[jaeger-v2] Add Validation And Comments to Badger Storage Config (#5927)

cmd/jaeger/config-badger.yaml

@@ -20,13 +20,15 @@ extensions:
     backends:
       some_store:
         badger:
-          directory_key: "/tmp/jaeger/"
-          directory_value: "/tmp/jaeger/"
+          directories:
+            keys: "/tmp/jaeger/"
+            values: "/tmp/jaeger/"
           ephemeral: false
       another_store:
         badger:
-          directory_key: "/tmp/jaeger_archive/"
-          directory_value: "/tmp/jaeger_archive/"
+          directories:
+            keys: "/tmp/jaeger_archive/"
+            values: "/tmp/jaeger_archive/"
           ephemeral: false
 
 receivers:

cmd/jaeger/internal/extension/jaegerstorage/config_test.go

@@ -61,7 +61,7 @@ backends:
 `)
 	cfg := createDefaultConfig().(*Config)
 	require.NoError(t, conf.Unmarshal(cfg))
-	assert.NotEmpty(t, cfg.Backends["some_storage"].Badger.SpanStoreTTL)
+	assert.NotEmpty(t, cfg.Backends["some_storage"].Badger.TTL.Spans)
 }
 
 func TestConfigDefaultGRPC(t *testing.T) {

plugin/storage/badger/factory.go

@@ -126,16 +126,16 @@ func (f *Factory) Initialize(metricsFactory metrics.Factory, logger *zap.Logger)
 		opts.Dir = f.tmpDir
 		opts.ValueDir = f.tmpDir
 
-		f.Options.Primary.KeyDirectory = f.tmpDir
-		f.Options.Primary.ValueDirectory = f.tmpDir
+		f.Options.Primary.Directories.Keys = f.tmpDir
+		f.Options.Primary.Directories.Values = f.tmpDir
 	} else {
 		// Errors are ignored as they're caught in the Open call
-		initializeDir(f.Options.Primary.KeyDirectory)
-		initializeDir(f.Options.Primary.ValueDirectory)
+		initializeDir(f.Options.Primary.Directories.Keys)
+		initializeDir(f.Options.Primary.Directories.Values)
 
 		opts.SyncWrites = f.Options.Primary.SyncWrites
-		opts.Dir = f.Options.Primary.KeyDirectory
-		opts.ValueDir = f.Options.Primary.ValueDirectory
+		opts.Dir = f.Options.Primary.Directories.Keys
+		opts.ValueDir = f.Options.Primary.Directories.Values
 
 		// These options make no sense with ephemeral data
 		opts.ReadOnly = f.Options.Primary.ReadOnly
@@ -147,7 +147,7 @@ func (f *Factory) Initialize(metricsFactory metrics.Factory, logger *zap.Logger)
 	}
 	f.store = store
 
-	f.cache = badgerStore.NewCacheStore(f.store, f.Options.Primary.SpanStoreTTL, true)
+	f.cache = badgerStore.NewCacheStore(f.store, f.Options.Primary.TTL.Spans, true)
 
 	f.metrics.ValueLogSpaceAvailable = metricsFactory.Gauge(metrics.Options{Name: valueLogSpaceAvailableName})
 	f.metrics.KeyLogSpaceAvailable = metricsFactory.Gauge(metrics.Options{Name: keyLogSpaceAvailableName})
@@ -178,7 +178,7 @@ func (f *Factory) CreateSpanReader() (spanstore.Reader, error) {
 
 // CreateSpanWriter implements storage.Factory
 func (f *Factory) CreateSpanWriter() (spanstore.Writer, error) {
-	return badgerStore.NewSpanWriter(f.store, f.cache, f.Options.Primary.SpanStoreTTL), nil
+	return badgerStore.NewSpanWriter(f.store, f.cache, f.Options.Primary.TTL.Spans), nil
 }
 
 // CreateDependencyReader implements storage.Factory

plugin/storage/badger/factory_test.go

@@ -203,8 +203,10 @@ func TestBadgerStorageFactoryWithConfig(t *testing.T) {
 	tmp := os.TempDir()
 	defer os.Remove(tmp)
 	cfg = NamespaceConfig{
-		ValueDirectory:        tmp,
-		KeyDirectory:          tmp,
+		Directories: Directories{
+			Keys:   tmp,
+			Values: tmp,
+		},
 		Ephemeral:             false,
 		MaintenanceInterval:   5,
 		MetricsUpdateInterval: 10,

plugin/storage/badger/options.go

@@ -9,6 +9,7 @@ import (
 	"path/filepath"
 	"time"
 
+	"github.com/asaskevich/govalidator"
 	"github.com/spf13/viper"
 	"go.uber.org/zap"
 )
@@ -19,17 +20,42 @@ type Options struct {
 	// This storage plugin does not support additional namespaces
 }
 
-// NamespaceConfig is badger's internal configuration data
+// NamespaceConfig is badger's internal configuration data.
 type NamespaceConfig struct {
-	SpanStoreTTL   time.Duration `mapstructure:"span_store_ttl"`
-	ValueDirectory string        `mapstructure:"directory_value"`
-	KeyDirectory   string        `mapstructure:"directory_key"`
-	// Setting this to true will ignore ValueDirectory and KeyDirectory
-	Ephemeral             bool          `mapstructure:"ephemeral"`
-	SyncWrites            bool          `mapstructure:"consistency"`
-	MaintenanceInterval   time.Duration `mapstructure:"maintenance_interval"`
+	// TTL holds time-to-live configuration for the badger store.
+	TTL TTL `mapstructure:"ttl"`
+	// Directories contains the configuration for where items are stored. Ephemeral must be
+	// set to false for this configuration to take effect.
+	Directories Directories `mapstructure:"directories"`
+	// Ephemeral, if set to true, will store data in a temporary file system.
+	// If set to true, the configuration in Directories is ignored.
+	Ephemeral bool `mapstructure:"ephemeral"`
+	// SyncWrites, if set to true, will immediately sync all writes to disk. Note that
+	// setting this field to true will affect write performance.
+	SyncWrites bool `mapstructure:"consistency"`
+	// MaintenanceInterval is the regular interval after which a maintenance job is
+	// run on the values in the store.
+	MaintenanceInterval time.Duration `mapstructure:"maintenance_interval"`
+	// MetricsUpdateInterval is the regular interval after which metrics are collected
+	// by Jaeger.
 	MetricsUpdateInterval time.Duration `mapstructure:"metrics_update_interval"`
-	ReadOnly              bool          `mapstructure:"read_only"`
+	// ReadOnly opens the data store in read-only mode. Multiple instances can open the same
+	// store in read-only mode. Values still in the write-ahead-log must be replayed before opening.
+	ReadOnly bool `mapstructure:"read_only"`
+}
+
+type TTL struct {
+	// SpanStore holds the amount of time that the span store data is stored.
+	// Once this duration has passed for a given key, span store data will
+	// no longer be accessible.
+	Spans time.Duration `mapstructure:"spans"`
+}
+
+type Directories struct {
+	// Keys contains the directory in which the keys are stored.
+	Keys string `mapstructure:"keys"`
+	// Values contains the directory in which the values are stored.
+	Values string `mapstructure:"values"`
 }
 
 const (
@@ -56,11 +82,15 @@ const (
 func DefaultNamespaceConfig() NamespaceConfig {
 	defaultBadgerDataDir := getCurrentExecutableDir()
 	return NamespaceConfig{
-		SpanStoreTTL:          defaultTTL,
-		SyncWrites:            false, // Performance over durability
-		Ephemeral:             true,  // Default is ephemeral storage
-		ValueDirectory:        defaultBadgerDataDir + defaultValueDir,
-		KeyDirectory:          defaultBadgerDataDir + defaultKeysDir,
+		TTL: TTL{
+			Spans: defaultTTL,
+		},
+		SyncWrites: false, // Performance over durability
+		Ephemeral:  true,  // Default is ephemeral storage
+		Directories: Directories{
+			Keys:   defaultBadgerDataDir + defaultKeysDir,
+			Values: defaultBadgerDataDir + defaultValueDir,
+		},
 		MaintenanceInterval:   defaultMaintenanceInterval,
 		MetricsUpdateInterval: defaultMetricsUpdateInterval,
 	}
@@ -97,17 +127,17 @@ func addFlags(flagSet *flag.FlagSet, nsConfig NamespaceConfig) {
 	)
 	flagSet.Duration(
 		prefix+suffixSpanstoreTTL,
-		nsConfig.SpanStoreTTL,
+		nsConfig.TTL.Spans,
 		"How long to store the data. Format is time.Duration (https://golang.org/pkg/time/#Duration)",
 	)
 	flagSet.String(
 		prefix+suffixKeyDirectory,
-		nsConfig.KeyDirectory,
+		nsConfig.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,
-		nsConfig.ValueDirectory,
+		nsConfig.Directories.Values,
 		"Path to store the values (spans). Set ephemeral to false if you want to define this setting.",
 	)
 	flagSet.Bool(
@@ -139,10 +169,10 @@ func (opt *Options) InitFromViper(v *viper.Viper, logger *zap.Logger) {
 
 func initFromViper(cfg *NamespaceConfig, v *viper.Viper, _ *zap.Logger) {
 	cfg.Ephemeral = v.GetBool(prefix + suffixEphemeral)
-	cfg.KeyDirectory = v.GetString(prefix + suffixKeyDirectory)
-	cfg.ValueDirectory = v.GetString(prefix + suffixValueDirectory)
+	cfg.Directories.Keys = v.GetString(prefix + suffixKeyDirectory)
+	cfg.Directories.Values = v.GetString(prefix + suffixValueDirectory)
 	cfg.SyncWrites = v.GetBool(prefix + suffixSyncWrite)
-	cfg.SpanStoreTTL = v.GetDuration(prefix + suffixSpanstoreTTL)
+	cfg.TTL.Spans = v.GetDuration(prefix + suffixSpanstoreTTL)
 	cfg.MaintenanceInterval = v.GetDuration(prefix + suffixMaintenanceInterval)
 	cfg.MetricsUpdateInterval = v.GetDuration(prefix + suffixMetricsInterval)
 	cfg.ReadOnly = v.GetBool(prefix + suffixReadOnly)
@@ -152,3 +182,8 @@ func initFromViper(cfg *NamespaceConfig, v *viper.Viper, _ *zap.Logger) {
 func (opt *Options) GetPrimary() NamespaceConfig {
 	return opt.Primary
 }
+
+func (c *NamespaceConfig) Validate() error {
+	_, err := govalidator.ValidateStruct(c)
+	return err
+}

plugin/storage/badger/options_test.go

@@ -8,6 +8,7 @@ import (
 	"time"
 
 	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
 	"go.uber.org/zap"
 
 	"github.com/jaegertracing/jaeger/pkg/config"
@@ -21,7 +22,7 @@ func TestDefaultOptionsParsing(t *testing.T) {
 
 	assert.True(t, opts.GetPrimary().Ephemeral)
 	assert.False(t, opts.GetPrimary().SyncWrites)
-	assert.Equal(t, time.Duration(72*time.Hour), opts.GetPrimary().SpanStoreTTL)
+	assert.Equal(t, time.Duration(72*time.Hour), opts.GetPrimary().TTL.Spans)
 }
 
 func TestParseOptions(t *testing.T) {
@@ -38,9 +39,9 @@ func TestParseOptions(t *testing.T) {
 
 	assert.False(t, opts.GetPrimary().Ephemeral)
 	assert.True(t, opts.GetPrimary().SyncWrites)
-	assert.Equal(t, time.Duration(168*time.Hour), opts.GetPrimary().SpanStoreTTL)
-	assert.Equal(t, "/var/lib/badger", opts.GetPrimary().KeyDirectory)
-	assert.Equal(t, "/mnt/slow/badger", opts.GetPrimary().ValueDirectory)
+	assert.Equal(t, time.Duration(168*time.Hour), opts.GetPrimary().TTL.Spans)
+	assert.Equal(t, "/var/lib/badger", opts.GetPrimary().Directories.Keys)
+	assert.Equal(t, "/mnt/slow/badger", opts.GetPrimary().Directories.Values)
 	assert.False(t, opts.GetPrimary().ReadOnly)
 }
 
@@ -53,3 +54,39 @@ func TestReadOnlyOptions(t *testing.T) {
 	opts.InitFromViper(v, zap.NewNop())
 	assert.True(t, opts.GetPrimary().ReadOnly)
 }
+
+func TestValidate_DoesNotReturnErrorWhenValid(t *testing.T) {
+	tests := []struct {
+		name   string
+		config *NamespaceConfig
+	}{
+		{
+			name:   "non-required fields not set",
+			config: &NamespaceConfig{},
+		},
+		{
+			name: "all fields are set",
+			config: &NamespaceConfig{
+				TTL: TTL{
+					Spans: time.Second,
+				},
+				Directories: Directories{
+					Keys:   "some-key-directory",
+					Values: "some-values-directory",
+				},
+				Ephemeral:             false,
+				SyncWrites:            false,
+				MaintenanceInterval:   time.Second,
+				MetricsUpdateInterval: time.Second,
+				ReadOnly:              false,
+			},
+		},
+	}
+
+	for _, test := range tests {
+		t.Run(test.name, func(t *testing.T) {
+			err := test.config.Validate()
+			require.NoError(t, err)
+		})
+	}
+}

plugin/storage/badger/stats_linux.go

@@ -12,11 +12,11 @@ func (f *Factory) diskStatisticsUpdate() error {
 	// In case of ephemeral these are the same, but we'll report them separately for consistency
 	var keyDirStatfs unix.Statfs_t
 	// Error ignored to satisfy Codecov
-	_ = unix.Statfs(f.Options.GetPrimary().KeyDirectory, &keyDirStatfs)
+	_ = unix.Statfs(f.Options.GetPrimary().Directories.Keys, &keyDirStatfs)
 
 	var valDirStatfs unix.Statfs_t
 	// Error ignored to satisfy Codecov
-	_ = unix.Statfs(f.Options.GetPrimary().ValueDirectory, &valDirStatfs)
+	_ = unix.Statfs(f.Options.GetPrimary().Directories.Values, &valDirStatfs)
 
 	// Using Bavail instead of Bfree to get non-priviledged user space available
 	//nolint: gosec // G115