Addressing review comments, replacing magic numbers with a sizeOf con…

…stant, using logger to indicate initialization is completed and io.Closer is used. Added maintenance thread that cleans up value log as well as exposes the size of data directories. Added some comments for multiple internal functions as well as refactored FindTraces to use more functions for easier reading.

Default data directory (when ephmeral is not used) is now starting directory + data/keys and data/values.

plugin/storage/badger/factory.go

@@ -15,33 +15,49 @@
 package badger
 
 import (
+	"expvar"
 	"flag"
 	"io/ioutil"
 	"os"
+	"runtime"
+	"time"
 
 	"github.com/dgraph-io/badger"
 	"github.com/spf13/viper"
 	"github.com/uber/jaeger-lib/metrics"
 	"go.uber.org/zap"
+	"golang.org/x/sys/unix"
 
 	badgerStore "github.com/jaegertracing/jaeger/plugin/storage/badger/spanstore"
 	"github.com/jaegertracing/jaeger/storage/dependencystore"
 	"github.com/jaegertracing/jaeger/storage/spanstore"
 )
 
+var (
+	// ValueLogSpaceAvailable returns the amount of space left on the value log mount point in bytes
+	ValueLogSpaceAvailable *expvar.Int
+	// KeyLogSpaceAvailable returns the amount of space left on the key log mount point in bytes
+	KeyLogSpaceAvailable *expvar.Int
+)
+
 // Factory implements storage.Factory for Badger backend.
 type Factory struct {
 	Options *Options
 	store   *badger.DB
 	cache   *badgerStore.CacheStore
+	logger  *zap.Logger
 
 	tmpDir string
-
-	cleaner func() error
 }
 
 // NewFactory creates a new Factory.
 func NewFactory() *Factory {
+	if ValueLogSpaceAvailable != nil {
+		ValueLogSpaceAvailable = expvar.NewInt("badger_value_log_bytes_available")
+	}
+	if KeyLogSpaceAvailable != nil {
+		KeyLogSpaceAvailable = expvar.NewInt("badger_key_log_bytes_available")
+	}
 	return &Factory{
 		Options: NewOptions("badger"),
 	}
@@ -59,6 +75,8 @@ func (f *Factory) InitFromViper(v *viper.Viper) {
 
 // Initialize implements storage.Factory
 func (f *Factory) Initialize(metricsFactory metrics.Factory, logger *zap.Logger) error {
+	f.logger = logger
+
 	opts := badger.DefaultOptions
 
 	if f.Options.primary.Ephemeral {
@@ -88,19 +106,9 @@ func (f *Factory) Initialize(metricsFactory metrics.Factory, logger *zap.Logger)
 	}
 	f.cache = cache
 
-	f.cleaner = func() error {
-		err := f.store.Close()
-		if err != nil {
-			return err
-		}
-
-		// Remove tmp files if this was ephemeral storage
-		if f.Options.primary.Ephemeral {
-			err = os.RemoveAll(f.tmpDir)
-		}
+	go f.maintenance()
 
-		return err
-	}
+	logger.Info("Badger storage configuration", zap.Any("configuration", opts))
 
 	return nil
 }
@@ -112,10 +120,58 @@ 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, f.cleaner), nil
+	return badgerStore.NewSpanWriter(f.store, f.cache, f.Options.primary.SpanStoreTTL, f), nil
 }
 
 // CreateDependencyReader implements storage.Factory
 func (f *Factory) CreateDependencyReader() (dependencystore.Reader, error) {
 	return nil, nil
 }
+
+// Close Implements io.Closer and closes the underlying storage
+func (f *Factory) Close() error {
+	err := f.store.Close()
+	if err != nil {
+		return err
+	}
+
+	// Remove tmp files if this was ephemeral storage
+	if f.Options.primary.Ephemeral {
+		err = os.RemoveAll(f.tmpDir)
+	}
+
+	return err
+}
+
+// Maintenance starts a background maintenance job for the badger K/V store, such as ValueLogGC
+func (f *Factory) maintenance() {
+	ticker := time.NewTicker(5 * time.Minute)
+	defer ticker.Stop()
+	for range ticker.C {
+		if err := f.store.RunValueLogGC(0.5); err != nil {
+			// Log? Some other metric?
+			f.logger.Error("ValueLogGC run failed with ", zap.Error(err))
+		}
+
+		// These stats are not interesting with Windows as there's no separate tmpfs
+		if runtime.GOOS == "linux" { // Include other *nix?
+			// In case of ephemeral these are the same, but we'll report them separately for consistency
+			var keyDirStatfs unix.Statfs_t
+			_ = unix.Statfs(f.Options.GetPrimary().KeyDirectory, &keyDirStatfs)
+
+			var valDirStatfs unix.Statfs_t
+			_ = unix.Statfs(f.Options.GetPrimary().ValueDirectory, &valDirStatfs)
+
+			// Using Bavail instead of Bfree to get non-priviledged user space available
+			ValueLogSpaceAvailable.Set(int64(valDirStatfs.Bavail) * valDirStatfs.Bsize)
+			KeyLogSpaceAvailable.Set(int64(keyDirStatfs.Bavail) * keyDirStatfs.Bsize)
+
+			/*
+			 TODO If we wanted to clean up oldest data to free up diskspace, we need at a minimum an index to the StartTime
+			 Additionally to that, the deletion might not save anything if the ratio of removed values is lower than the RunValueLogGC's deletion ratio
+			 and with the keys the LSM compaction must remove the offending files also. Thus, there's no guarantee the clean up would
+			 actually reduce the amount of diskspace used any faster than allowing TTL to remove them.
+			*/
+		}
+	}
+}

plugin/storage/badger/options.go

@@ -16,6 +16,8 @@ package badger
 
 import (
 	"flag"
+	"os"
+	"path/filepath"
 	"time"
 
 	"github.com/spf13/viper"
@@ -43,18 +45,23 @@ const (
 	suffixEphemeral      = ".ephemeral"
 	suffixSpanstoreTTL   = ".span-store-ttl"
 	suffixSyncWrite      = ".consistency"
+	defaultValueDir      = "/data/values"
+	defaultKeysDir       = "/data/keys"
 )
 
 // NewOptions creates a new Options struct.
 func NewOptions(primaryNamespace string, otherNamespaces ...string) *Options {
+
+	defaultDataDir := getCurrentExecutableDir()
+
 	options := &Options{
 		primary: &NamespaceConfig{
 			namespace:      primaryNamespace,
 			SpanStoreTTL:   time.Hour * 72, // Default is 3 days
-			SyncWrites:     false,          // Performance over consistency
+			SyncWrites:     false,          // Performance over durability
 			Ephemeral:      true,           // Default is ephemeral storage
-			ValueDirectory: "",
-			KeyDirectory:   "",
+			ValueDirectory: defaultDataDir + defaultValueDir,
+			KeyDirectory:   defaultDataDir + defaultKeysDir,
 		},
 		others: make(map[string]*NamespaceConfig, len(otherNamespaces)),
 	}
@@ -66,6 +73,12 @@ func NewOptions(primaryNamespace string, otherNamespaces ...string) *Options {
 	return options
 }
 
+func getCurrentExecutableDir() string {
+	// We ignore the error, this will fail later when trying to start the store
+	exec, _ := os.Executable()
+	return filepath.Dir(exec)
+}
+
 // AddFlags adds flags for Options
 func (opt *Options) AddFlags(flagSet *flag.FlagSet) {
 	addFlags(flagSet, opt.primary)
@@ -78,27 +91,27 @@ func addFlags(flagSet *flag.FlagSet, nsConfig *NamespaceConfig) {
 	flagSet.Bool(
 		nsConfig.namespace+suffixEphemeral,
 		nsConfig.Ephemeral,
-		"Mark this storage ephemeral, data is stored in tmpfs (in-memory). Default is true.",
+		"Mark this storage ephemeral, data is stored in tmpfs.",
 	)
 	flagSet.Duration(
 		nsConfig.namespace+suffixSpanstoreTTL,
 		nsConfig.SpanStoreTTL,
-		"time.Duration to store the data. Default is 72 hours (3 days).",
+		"How long to store the data. Format is time.Duration (https://golang.org/pkg/time/#Duration)",
 	)
 	flagSet.String(
 		nsConfig.namespace+suffixKeyDirectory,
 		nsConfig.KeyDirectory,
-		"Path to store the keys (indexes), this directory should reside in SSD disk. Set ephmeral to false if you want to define this setting.",
+		"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(
 		nsConfig.namespace+suffixValueDirectory,
 		nsConfig.ValueDirectory,
-		"Path to store the values (spans). Set ephmeral to false if you want to define this setting.",
+		"Path to store the values (spans). Set ephemeral to false if you want to define this setting.",
 	)
 	flagSet.Bool(
 		nsConfig.namespace+suffixSyncWrite,
 		nsConfig.SyncWrites,
-		"If all writes should be synced immediately. This will greatly reduce write performance and will require fast SSD drives. Default is false.",
+		"If all writes should be synced immediately. This will greatly reduce write performance.",
 	)
 }