From 0a3822fc89b383b63be7ba46d3d20cd539449b73 Mon Sep 17 00:00:00 2001 From: Mahad Zaryab Date: Wed, 4 Sep 2024 23:08:19 -0400 Subject: [PATCH] Address Feedback From PR Review Signed-off-by: Mahad Zaryab --- .../extension/jaegerstorage/config.go | 2 +- plugin/storage/badger/config.go | 83 +------------------ plugin/storage/badger/config_test.go | 45 ---------- .../badger/dependencystore/storage_test.go | 2 +- plugin/storage/badger/factory.go | 10 +-- plugin/storage/badger/factory_test.go | 4 +- plugin/storage/badger/options.go | 83 +++++++++++++++++++ plugin/storage/badger/options_test.go | 55 ++++++++++++ .../badger/spanstore/read_write_test.go | 6 +- plugin/storage/badger/stats_linux_test.go | 2 +- plugin/storage/badger/stats_test.go | 2 +- 11 files changed, 156 insertions(+), 138 deletions(-) create mode 100644 plugin/storage/badger/options.go create mode 100644 plugin/storage/badger/options_test.go diff --git a/cmd/jaeger/internal/extension/jaegerstorage/config.go b/cmd/jaeger/internal/extension/jaegerstorage/config.go index 34c33d2cbcb..d297bea5b3d 100644 --- a/cmd/jaeger/internal/extension/jaegerstorage/config.go +++ b/cmd/jaeger/internal/extension/jaegerstorage/config.go @@ -62,7 +62,7 @@ func (cfg *Backend) Unmarshal(conf *confmap.Conf) error { } } if conf.IsSet("badger") { - v := badger.NewConfig() + v := badger.DefaultConfig() cfg.Badger = v } if conf.IsSet("grpc") { diff --git a/plugin/storage/badger/config.go b/plugin/storage/badger/config.go index 24e7e27b6b5..73e95a7dbb4 100644 --- a/plugin/storage/badger/config.go +++ b/plugin/storage/badger/config.go @@ -4,14 +4,11 @@ package badger import ( - "flag" "os" "path/filepath" "time" "github.com/asaskevich/govalidator" - "github.com/spf13/viper" - "go.uber.org/zap" ) // Config is badger's internal configuration data. @@ -56,24 +53,12 @@ const ( defaultMaintenanceInterval time.Duration = 5 * time.Minute defaultMetricsUpdateInterval time.Duration = 10 * time.Second defaultTTL time.Duration = time.Hour * 72 + defaultDataDir string = string(os.PathSeparator) + "data" + defaultValueDir string = defaultDataDir + string(os.PathSeparator) + "values" + defaultKeysDir string = defaultDataDir + string(os.PathSeparator) + "keys" ) -const ( - prefix = "badger" - suffixKeyDirectory = ".directory-key" - suffixValueDirectory = ".directory-value" - suffixEphemeral = ".ephemeral" - suffixSpanstoreTTL = ".span-store-ttl" - suffixSyncWrite = ".consistency" - suffixMaintenanceInterval = ".maintenance-interval" - suffixMetricsInterval = ".metrics-update-interval" // Intended only for testing purposes - suffixReadOnly = ".read-only" - defaultDataDir = string(os.PathSeparator) + "data" - defaultValueDir = defaultDataDir + string(os.PathSeparator) + "values" - defaultKeysDir = defaultDataDir + string(os.PathSeparator) + "keys" -) - -func NewConfig() *Config { +func DefaultConfig() *Config { defaultBadgerDataDir := getCurrentExecutableDir() return &Config{ TTL: TTL{ @@ -96,66 +81,6 @@ func getCurrentExecutableDir() string { return filepath.Dir(exec) } -// AddFlags adds flags for Config. -func (c *Config) AddFlags(flagSet *flag.FlagSet) { - flagSet.Bool( - prefix+suffixEphemeral, - c.Ephemeral, - "Mark this storage ephemeral, data is stored in tmpfs.", - ) - flagSet.Duration( - prefix+suffixSpanstoreTTL, - c.TTL.Spans, - "How long to store the data. Format is time.Duration (https://golang.org/pkg/time/#Duration)", - ) - flagSet.String( - prefix+suffixKeyDirectory, - c.Directories.Keys, - "Path to store the keys (indexes), this directory should reside in SSD disk. Set ephemeral to false if you want to define this setting.", - ) - flagSet.String( - prefix+suffixValueDirectory, - c.Directories.Values, - "Path to store the values (spans). Set ephemeral to false if you want to define this setting.", - ) - flagSet.Bool( - prefix+suffixSyncWrite, - c.SyncWrites, - "If all writes should be synced immediately to physical disk. This will impact write performance.", - ) - flagSet.Duration( - prefix+suffixMaintenanceInterval, - c.MaintenanceInterval, - "How often the maintenance thread for values is ran. Format is time.Duration (https://golang.org/pkg/time/#Duration)", - ) - flagSet.Duration( - prefix+suffixMetricsInterval, - c.MetricsUpdateInterval, - "How often the badger metrics are collected by Jaeger. Format is time.Duration (https://golang.org/pkg/time/#Duration)", - ) - flagSet.Bool( - prefix+suffixReadOnly, - c.ReadOnly, - "Allows to open badger database in read only mode. Multiple instances can open same database in read-only mode. Values still in the write-ahead-log must be replayed before opening.", - ) -} - -// InitFromViper initializes Config with properties from viper. -func (c *Config) InitFromViper(v *viper.Viper, logger *zap.Logger) { - initFromViper(c, v, logger) -} - -func initFromViper(config *Config, v *viper.Viper, _ *zap.Logger) { - config.Ephemeral = v.GetBool(prefix + suffixEphemeral) - config.Directories.Keys = v.GetString(prefix + suffixKeyDirectory) - config.Directories.Values = v.GetString(prefix + suffixValueDirectory) - config.SyncWrites = v.GetBool(prefix + suffixSyncWrite) - config.TTL.Spans = v.GetDuration(prefix + suffixSpanstoreTTL) - config.MaintenanceInterval = v.GetDuration(prefix + suffixMaintenanceInterval) - config.MetricsUpdateInterval = v.GetDuration(prefix + suffixMetricsInterval) - config.ReadOnly = v.GetBool(prefix + suffixReadOnly) -} - func (c *Config) Validate() error { _, err := govalidator.ValidateStruct(c) return err diff --git a/plugin/storage/badger/config_test.go b/plugin/storage/badger/config_test.go index 43c7f1a1e54..ec3b363c2b4 100644 --- a/plugin/storage/badger/config_test.go +++ b/plugin/storage/badger/config_test.go @@ -7,54 +7,9 @@ import ( "testing" "time" - "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" - "go.uber.org/zap" - - "github.com/jaegertracing/jaeger/pkg/config" ) -func TestDefaultConfigParsing(t *testing.T) { - cfg := NewConfig() - v, command := config.Viperize(cfg.AddFlags) - command.ParseFlags([]string{}) - cfg.InitFromViper(v, zap.NewNop()) - - assert.True(t, cfg.Ephemeral) - assert.False(t, cfg.SyncWrites) - assert.Equal(t, time.Duration(72*time.Hour), cfg.TTL.Spans) -} - -func TestParseConfig(t *testing.T) { - cfg := NewConfig() - v, command := config.Viperize(cfg.AddFlags) - command.ParseFlags([]string{ - "--badger.ephemeral=false", - "--badger.consistency=true", - "--badger.directory-key=/var/lib/badger", - "--badger.directory-value=/mnt/slow/badger", - "--badger.span-store-ttl=168h", - }) - cfg.InitFromViper(v, zap.NewNop()) - - assert.False(t, cfg.Ephemeral) - assert.True(t, cfg.SyncWrites) - assert.Equal(t, time.Duration(168*time.Hour), cfg.TTL.Spans) - assert.Equal(t, "/var/lib/badger", cfg.Directories.Keys) - assert.Equal(t, "/mnt/slow/badger", cfg.Directories.Values) - assert.False(t, cfg.ReadOnly) -} - -func TestReadOnlyConfig(t *testing.T) { - cfg := NewConfig() - v, command := config.Viperize(cfg.AddFlags) - command.ParseFlags([]string{ - "--badger.read-only=true", - }) - cfg.InitFromViper(v, zap.NewNop()) - assert.True(t, cfg.ReadOnly) -} - func TestValidate_DoesNotReturnErrorWhenValid(t *testing.T) { tests := []struct { name string diff --git a/plugin/storage/badger/dependencystore/storage_test.go b/plugin/storage/badger/dependencystore/storage_test.go index 001ac7e417d..187e524a0b9 100644 --- a/plugin/storage/badger/dependencystore/storage_test.go +++ b/plugin/storage/badger/dependencystore/storage_test.go @@ -28,7 +28,7 @@ func runFactoryTest(tb testing.TB, test func(tb testing.TB, sw spanstore.Writer, require.NoError(tb, f.Close()) }() - cfg := badger.NewConfig() + cfg := badger.DefaultConfig() v, command := config.Viperize(cfg.AddFlags) command.ParseFlags([]string{ "--badger.ephemeral=true", diff --git a/plugin/storage/badger/factory.go b/plugin/storage/badger/factory.go index 02e888c67c3..63d05612c22 100644 --- a/plugin/storage/badger/factory.go +++ b/plugin/storage/badger/factory.go @@ -77,7 +77,7 @@ type Factory struct { // NewFactory creates a new Factory. func NewFactory() *Factory { return &Factory{ - Config: NewConfig(), + Config: DefaultConfig(), maintenanceDone: make(chan bool), } } @@ -88,7 +88,7 @@ func NewFactoryWithConfig( logger *zap.Logger, ) (*Factory, error) { f := NewFactory() - f.configureFromConfig(&cfg) + f.configure(&cfg) err := f.Initialize(metricsFactory, logger) if err != nil { return nil, err @@ -104,11 +104,11 @@ func (f *Factory) AddFlags(flagSet *flag.FlagSet) { // InitFromViper implements plugin.Configurable func (f *Factory) InitFromViper(v *viper.Viper, logger *zap.Logger) { f.Config.InitFromViper(v, logger) - f.configureFromConfig(f.Config) + f.configure(f.Config) } -// configureFromConfig initializes Factory from supplied Config. -func (f *Factory) configureFromConfig(config *Config) { +// configure initializes Factory from supplied Config. +func (f *Factory) configure(config *Config) { f.Config = config } diff --git a/plugin/storage/badger/factory_test.go b/plugin/storage/badger/factory_test.go index b5f53cb2417..6190f1510b6 100644 --- a/plugin/storage/badger/factory_test.go +++ b/plugin/storage/badger/factory_test.go @@ -183,12 +183,12 @@ func TestBadgerMetrics(t *testing.T) { require.NoError(t, err) } -func TestConfigureFromConfig(t *testing.T) { +func TestConfigure(t *testing.T) { f := NewFactory() config := &Config{ MaintenanceInterval: 42 * time.Second, } - f.configureFromConfig(config) + f.configure(config) assert.Equal(t, config, f.Config) } diff --git a/plugin/storage/badger/options.go b/plugin/storage/badger/options.go new file mode 100644 index 00000000000..c9f34f40e0f --- /dev/null +++ b/plugin/storage/badger/options.go @@ -0,0 +1,83 @@ +// Copyright (c) 2024 The Jaeger Authors. +// SPDX-License-Identifier: Apache-2.0 + +package badger + +import ( + "flag" + + "github.com/spf13/viper" + "go.uber.org/zap" +) + +const ( + prefix = "badger" + suffixKeyDirectory = ".directory-key" + suffixValueDirectory = ".directory-value" + suffixEphemeral = ".ephemeral" + suffixSpanstoreTTL = ".span-store-ttl" + suffixSyncWrite = ".consistency" + suffixMaintenanceInterval = ".maintenance-interval" + suffixMetricsInterval = ".metrics-update-interval" // Intended only for testing purposes + suffixReadOnly = ".read-only" +) + +// AddFlags adds flags for Config. +func (c *Config) AddFlags(flagSet *flag.FlagSet) { + flagSet.Bool( + prefix+suffixEphemeral, + c.Ephemeral, + "Mark this storage ephemeral, data is stored in tmpfs.", + ) + flagSet.Duration( + prefix+suffixSpanstoreTTL, + c.TTL.Spans, + "How long to store the data. Format is time.Duration (https://golang.org/pkg/time/#Duration)", + ) + flagSet.String( + prefix+suffixKeyDirectory, + c.Directories.Keys, + "Path to store the keys (indexes), this directory should reside in SSD disk. Set ephemeral to false if you want to define this setting.", + ) + flagSet.String( + prefix+suffixValueDirectory, + c.Directories.Values, + "Path to store the values (spans). Set ephemeral to false if you want to define this setting.", + ) + flagSet.Bool( + prefix+suffixSyncWrite, + c.SyncWrites, + "If all writes should be synced immediately to physical disk. This will impact write performance.", + ) + flagSet.Duration( + prefix+suffixMaintenanceInterval, + c.MaintenanceInterval, + "How often the maintenance thread for values is ran. Format is time.Duration (https://golang.org/pkg/time/#Duration)", + ) + flagSet.Duration( + prefix+suffixMetricsInterval, + c.MetricsUpdateInterval, + "How often the badger metrics are collected by Jaeger. Format is time.Duration (https://golang.org/pkg/time/#Duration)", + ) + flagSet.Bool( + prefix+suffixReadOnly, + c.ReadOnly, + "Allows to open badger database in read only mode. Multiple instances can open same database in read-only mode. Values still in the write-ahead-log must be replayed before opening.", + ) +} + +// InitFromViper initializes Config with properties from viper. +func (c *Config) InitFromViper(v *viper.Viper, logger *zap.Logger) { + initFromViper(c, v, logger) +} + +func initFromViper(config *Config, v *viper.Viper, _ *zap.Logger) { + config.Ephemeral = v.GetBool(prefix + suffixEphemeral) + config.Directories.Keys = v.GetString(prefix + suffixKeyDirectory) + config.Directories.Values = v.GetString(prefix + suffixValueDirectory) + config.SyncWrites = v.GetBool(prefix + suffixSyncWrite) + config.TTL.Spans = v.GetDuration(prefix + suffixSpanstoreTTL) + config.MaintenanceInterval = v.GetDuration(prefix + suffixMaintenanceInterval) + config.MetricsUpdateInterval = v.GetDuration(prefix + suffixMetricsInterval) + config.ReadOnly = v.GetBool(prefix + suffixReadOnly) +} diff --git a/plugin/storage/badger/options_test.go b/plugin/storage/badger/options_test.go new file mode 100644 index 00000000000..e397391427c --- /dev/null +++ b/plugin/storage/badger/options_test.go @@ -0,0 +1,55 @@ +// Copyright (c) 2024 The Jaeger Authors. +// SPDX-License-Identifier: Apache-2.0 + +package badger + +import ( + "testing" + "time" + + "github.com/stretchr/testify/assert" + "go.uber.org/zap" + + "github.com/jaegertracing/jaeger/pkg/config" +) + +func TestDefaultConfigParsing(t *testing.T) { + cfg := DefaultConfig() + v, command := config.Viperize(cfg.AddFlags) + command.ParseFlags([]string{}) + cfg.InitFromViper(v, zap.NewNop()) + + assert.True(t, cfg.Ephemeral) + assert.False(t, cfg.SyncWrites) + assert.Equal(t, time.Duration(72*time.Hour), cfg.TTL.Spans) +} + +func TestParseConfig(t *testing.T) { + cfg := DefaultConfig() + v, command := config.Viperize(cfg.AddFlags) + command.ParseFlags([]string{ + "--badger.ephemeral=false", + "--badger.consistency=true", + "--badger.directory-key=/var/lib/badger", + "--badger.directory-value=/mnt/slow/badger", + "--badger.span-store-ttl=168h", + }) + cfg.InitFromViper(v, zap.NewNop()) + + assert.False(t, cfg.Ephemeral) + assert.True(t, cfg.SyncWrites) + assert.Equal(t, time.Duration(168*time.Hour), cfg.TTL.Spans) + assert.Equal(t, "/var/lib/badger", cfg.Directories.Keys) + assert.Equal(t, "/mnt/slow/badger", cfg.Directories.Values) + assert.False(t, cfg.ReadOnly) +} + +func TestReadOnlyConfig(t *testing.T) { + cfg := DefaultConfig() + v, command := config.Viperize(cfg.AddFlags) + command.ParseFlags([]string{ + "--badger.read-only=true", + }) + cfg.InitFromViper(v, zap.NewNop()) + assert.True(t, cfg.ReadOnly) +} diff --git a/plugin/storage/badger/spanstore/read_write_test.go b/plugin/storage/badger/spanstore/read_write_test.go index 5571275fd3d..6fa17c497a8 100644 --- a/plugin/storage/badger/spanstore/read_write_test.go +++ b/plugin/storage/badger/spanstore/read_write_test.go @@ -373,7 +373,7 @@ func TestPersist(t *testing.T) { require.NoError(t, f.Close()) }() - cfg := badger.NewConfig() + cfg := badger.DefaultConfig() v, command := config.Viperize(cfg.AddFlags) keyParam := fmt.Sprintf("--badger.directory-key=%s", dir) @@ -438,7 +438,7 @@ func runFactoryTest(tb testing.TB, test func(tb testing.TB, sw spanstore.Writer, require.NoError(tb, f.Close()) }() - cfg := badger.NewConfig() + cfg := badger.DefaultConfig() v, command := config.Viperize(cfg.AddFlags) command.ParseFlags([]string{ "--badger.ephemeral=true", @@ -598,7 +598,7 @@ func BenchmarkServiceIndexLimitFetch(b *testing.B) { func runLargeFactoryTest(tb testing.TB, test func(tb testing.TB, sw spanstore.Writer, sr spanstore.Reader)) { assertion := require.New(tb) f := badger.NewFactory() - cfg := badger.NewConfig() + cfg := badger.DefaultConfig() v, command := config.Viperize(cfg.AddFlags) dir := filepath.Join(tb.TempDir(), "badger-testRun") diff --git a/plugin/storage/badger/stats_linux_test.go b/plugin/storage/badger/stats_linux_test.go index 82b539462a3..d8af2aa4bcd 100644 --- a/plugin/storage/badger/stats_linux_test.go +++ b/plugin/storage/badger/stats_linux_test.go @@ -16,7 +16,7 @@ import ( func TestDiskStatisticsUpdate(t *testing.T) { f := NewFactory() - cfg := NewConfig() + cfg := DefaultConfig() v, command := config.Viperize(cfg.AddFlags) command.ParseFlags([]string{ "--badger.ephemeral=true", diff --git a/plugin/storage/badger/stats_test.go b/plugin/storage/badger/stats_test.go index c4648f64d8d..b98c3e645cb 100644 --- a/plugin/storage/badger/stats_test.go +++ b/plugin/storage/badger/stats_test.go @@ -18,7 +18,7 @@ import ( func TestDiskStatisticsUpdate(t *testing.T) { f := NewFactory() - cfg := NewConfig() + cfg := DefaultConfig() v, command := config.Viperize(cfg.AddFlags) command.ParseFlags([]string{ "--badger.ephemeral=true",