Skip to content

Commit

Permalink
Address Feedback From PR Review
Browse files Browse the repository at this point in the history
Signed-off-by: Mahad Zaryab <[email protected]>
  • Loading branch information
mahadzaryab1 committed Sep 5, 2024
1 parent a9e5e47 commit 0a3822f
Show file tree
Hide file tree
Showing 11 changed files with 156 additions and 138 deletions.
2 changes: 1 addition & 1 deletion cmd/jaeger/internal/extension/jaegerstorage/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -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") {
Expand Down
83 changes: 4 additions & 79 deletions plugin/storage/badger/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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{
Expand All @@ -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
Expand Down
45 changes: 0 additions & 45 deletions plugin/storage/badger/config_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion plugin/storage/badger/dependencystore/storage_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down
10 changes: 5 additions & 5 deletions plugin/storage/badger/factory.go
Original file line number Diff line number Diff line change
Expand Up @@ -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),
}
}
Expand All @@ -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
Expand All @@ -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
}

Expand Down
4 changes: 2 additions & 2 deletions plugin/storage/badger/factory_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}

Expand Down
83 changes: 83 additions & 0 deletions plugin/storage/badger/options.go
Original file line number Diff line number Diff line change
@@ -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)
}
55 changes: 55 additions & 0 deletions plugin/storage/badger/options_test.go
Original file line number Diff line number Diff line change
@@ -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)
}
6 changes: 3 additions & 3 deletions plugin/storage/badger/spanstore/read_write_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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",
Expand Down Expand Up @@ -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")
Expand Down
Loading

0 comments on commit 0a3822f

Please sign in to comment.