[jaeger-v2] Streamline storage initialization (#5171)

## Which problem is this PR solving?
- There is duplicated code in the extension Start, which is being
copy/pasted in other PRs

## Description of the changes
- Refactor Start to avoid duplication and to make testing simpler (no
need to test all edge cases for every storage type)
- Clean-up tests

## How was this change tested?
- 

## Checklist
- [ ] I have read
https://github.com/jaegertracing/jaeger/blob/master/CONTRIBUTING_GUIDELINES.md
- [ ] I have signed all commits
- [ ] I have added unit tests for the new functionality
- [ ] I have run lint and test steps successfully
  - for `jaeger`: `make lint test`
  - for `jaeger-ui`: `yarn lint` and `yarn test`

Signed-off-by: Yuri Shkuro <github@ysh.us>

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

@@ -13,8 +13,10 @@ import (
 	"go.opentelemetry.io/collector/extension"
 	"go.uber.org/zap"
 
+	memoryCfg "github.com/jaegertracing/jaeger/pkg/memory/config"
 	"github.com/jaegertracing/jaeger/pkg/metrics"
 	"github.com/jaegertracing/jaeger/plugin/storage/badger"
+	badgerCfg "github.com/jaegertracing/jaeger/plugin/storage/badger"
 	"github.com/jaegertracing/jaeger/plugin/storage/memory"
 	"github.com/jaegertracing/jaeger/storage"
 )
@@ -60,35 +62,62 @@ func newStorageExt(config *Config, otel component.TelemetrySettings) *storageExt
 	}
 }
 
-func (s *storageExt) Start(ctx context.Context, host component.Host) error {
-	for name, mem := range s.config.Memory {
-		if _, ok := s.factories[name]; ok {
-			return fmt.Errorf("duplicate memory storage name %s", name)
-		}
-		s.factories[name] = memory.NewFactoryWithConfig(
-			mem,
-			metrics.NullFactory,
-			s.logger.With(zap.String("storage_name", name)),
-		)
-	}
+type starter[Config any, Factory storage.Factory] struct {
+	ext         *storageExt
+	storageKind string
+	cfg         map[string]Config
+	builder     func(Config, metrics.Factory, *zap.Logger) (Factory, error)
+}
 
-	for name, b := range s.config.Badger {
-		if _, ok := s.factories[name]; ok {
-			return fmt.Errorf("duplicate badger storage name %s", name)
+func (s *starter[Config, Factory]) build(ctx context.Context, host component.Host) error {
+	for name, cfg := range s.cfg {
+		if _, ok := s.ext.factories[name]; ok {
+			return fmt.Errorf("duplicate %s storage name %s", s.storageKind, name)
 		}
-		var err error
-		factory, err := badger.NewFactoryWithConfig(
-			b,
+		factory, err := s.builder(
+			cfg,
 			metrics.NullFactory,
-			s.logger.With(zap.String("storage_name", name)),
+			s.ext.logger.With(zap.String("storage_name", name)),
 		)
 		if err != nil {
-			return fmt.Errorf("failed to initialize badger storage: %w", err)
+			return fmt.Errorf("failed to initialize %s storage %s: %w", s.storageKind, name, err)
 		}
-		s.factories[name] = factory
+		s.ext.factories[name] = factory
 	}
+	return nil
+}
 
-	// TODO add support for other backends
+func (s *storageExt) Start(ctx context.Context, host component.Host) error {
+	memStarter := &starter[memoryCfg.Configuration, *memory.Factory]{
+		ext:         s,
+		storageKind: "memory",
+		cfg:         s.config.Memory,
+		// memory factory does not return an error, so need to wrap it
+		builder: func(
+			cfg memoryCfg.Configuration,
+			metricsFactory metrics.Factory,
+			logger *zap.Logger,
+		) (*memory.Factory, error) {
+			return memory.NewFactoryWithConfig(cfg, metricsFactory, logger), nil
+		},
+	}
+	badgerStarter := &starter[badgerCfg.NamespaceConfig, *badger.Factory]{
+		ext:         s,
+		storageKind: "badger",
+		cfg:         s.config.Badger,
+		builder:     badger.NewFactoryWithConfig,
+	}
+
+	builders := []func(ctx context.Context, host component.Host) error{
+		memStarter.build,
+		badgerStarter.build,
+		// TODO add support for other backends
+	}
+	for _, builder := range builders {
+		if err := builder(ctx, host); err != nil {
+			return err
+		}
+	}
 	return nil
 }
 

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

@@ -24,11 +24,6 @@ import (
 	"github.com/jaegertracing/jaeger/storage/spanstore"
 )
 
-const (
-	memstoreName = "memstore"
-	badgerName   = "badgerstore"
-)
-
 type storageHost struct {
 	t                *testing.T
 	storageExtension component.Component
@@ -79,143 +74,119 @@ func (e errorFactory) Close() error {
 func TestStorageExtensionConfigError(t *testing.T) {
 	config := createDefaultConfig().(*Config)
 	err := config.Validate()
-	require.EqualError(t, err, fmt.Sprintf("%s: no storage type present in config", ID))
+	require.ErrorContains(t, err, "no storage type present in config")
 }
 
-func TestStorageExtensionStartTwiceError(t *testing.T) {
-	ctx := context.Background()
-
-	storageExtension := makeStorageExtension(t, memstoreName)
-
-	host := componenttest.NewNopHost()
-	err := storageExtension.Start(ctx, host)
-	require.Error(t, err)
-	require.EqualError(t, err, fmt.Sprintf("duplicate memory storage name %s", memstoreName))
+func TestStorageExtensionNameConflict(t *testing.T) {
+	storageExtension := makeStorageExtenion(t, &Config{
+		Memory: map[string]memoryCfg.Configuration{
+			"foo": {MaxTraces: 10000},
+		},
+		Badger: map[string]badgerCfg.NamespaceConfig{
+			"foo": {},
+		},
+	})
+	err := storageExtension.Start(context.Background(), componenttest.NewNopHost())
+	require.ErrorContains(t, err, "duplicate")
 }
 
 func TestStorageFactoryBadHostError(t *testing.T) {
-	makeStorageExtension(t, memstoreName)
-
 	host := componenttest.NewNopHost()
-	_, err := GetStorageFactory(memstoreName, host)
-	require.Error(t, err)
-	require.EqualError(t, err, fmt.Sprintf("cannot find extension '%s' (make sure it's defined earlier in the config)", ID))
+	_, err := GetStorageFactory("something", host)
+	require.ErrorContains(t, err, "cannot find extension")
 }
 
 func TestStorageFactoryBadNameError(t *testing.T) {
-	storageExtension := makeStorageExtension(t, memstoreName)
-
-	host := storageHost{t: t, storageExtension: storageExtension}
-	const badMemstoreName = "test"
-
-	_, err := GetStorageFactory(badMemstoreName, host)
-	require.Error(t, err)
-	require.EqualError(t, err, fmt.Sprintf("cannot find storage '%s' declared with '%s' extension", badMemstoreName, ID))
+	host := storageHost{t: t, storageExtension: startStorageExtension(t, "foo")}
+	_, err := GetStorageFactory("bar", host)
+	require.ErrorContains(t, err, "cannot find storage 'bar'")
 }
 
 func TestStorageFactoryBadShutdownError(t *testing.T) {
+	shutdownError := fmt.Errorf("shutdown error")
 	storageExtension := storageExt{
-		factories: make(map[string]storage.Factory),
+		factories: map[string]storage.Factory{
+			"foo": errorFactory{closeErr: shutdownError},
+		},
 	}
-	badFactoryError := fmt.Errorf("error factory")
-	storageExtension.factories[memstoreName] = errorFactory{closeErr: badFactoryError}
-
 	err := storageExtension.Shutdown(context.Background())
-	require.ErrorIs(t, err, badFactoryError)
+	require.ErrorIs(t, err, shutdownError)
 }
 
 func TestStorageExtension(t *testing.T) {
-	storageExtension := makeStorageExtension(t, memstoreName)
-
-	host := storageHost{t: t, storageExtension: storageExtension}
-
-	_, err := GetStorageFactory(memstoreName, host)
+	const name = "foo"
+	host := storageHost{t: t, storageExtension: startStorageExtension(t, name)}
+	f, err := GetStorageFactory(name, host)
 	require.NoError(t, err)
+	require.NotNil(t, f)
 }
 
 func TestBadgerStorageExtension(t *testing.T) {
-	ctx := context.Background()
-	telemetrySettings := component.TelemetrySettings{
-		Logger:         zap.NewNop(),
-		TracerProvider: nooptrace.NewTracerProvider(),
-		MeterProvider:  noopmetric.NewMeterProvider(),
-	}
-
-	config := &Config{
+	storageExtension := makeStorageExtenion(t, &Config{
 		Badger: map[string]badgerCfg.NamespaceConfig{
-			badgerName: {
+			"foo": {
 				Ephemeral:             true,
 				MaintenanceInterval:   5,
 				MetricsUpdateInterval: 10,
 			},
 		},
-	}
-
-	require.NoError(t, config.Validate())
-
-	extensionFactory := NewFactory()
-	storageExtension, err := extensionFactory.CreateExtension(ctx, extension.CreateSettings{
-		ID:                ID,
-		TelemetrySettings: telemetrySettings,
-		BuildInfo:         component.NewDefaultBuildInfo(),
-	}, config)
-	require.NoError(t, err)
-
-	host := componenttest.NewNopHost()
-	err = storageExtension.Start(ctx, host)
+	})
+	ctx := context.Background()
+	err := storageExtension.Start(ctx, componenttest.NewNopHost())
 	require.NoError(t, err)
-
-	err = storageExtension.Start(ctx, host)
-	t.Cleanup(func() { require.NoError(t, storageExtension.Shutdown(ctx)) })
-	require.Error(t, err)
-	require.EqualError(t, err, fmt.Sprintf("duplicate badger storage name %s", badgerName))
+	require.NoError(t, storageExtension.Shutdown(ctx))
 }
 
 func TestBadgerStorageExtensionError(t *testing.T) {
-	ctx := context.Background()
-	factory, _ := badgerCfg.NewFactoryWithConfig(badgerCfg.NamespaceConfig{}, metrics.NullFactory, zap.NewNop())
-	ext := storageExt{
-		config: &Config{
-			Badger: map[string]badgerCfg.NamespaceConfig{
-				badgerName: {},
+	ext := makeStorageExtenion(t, &Config{
+		Badger: map[string]badgerCfg.NamespaceConfig{
+			"foo": {
+				KeyDirectory:   "/bad/path",
+				ValueDirectory: "/bad/path",
 			},
 		},
-		logger:    zap.NewNop(),
-		factories: map[string]storage.Factory{"badger": factory},
-	}
-	err := ext.Start(ctx, componenttest.NewNopHost())
-	require.Error(t, err)
-	require.EqualError(t, err, "failed to initialize badger storage: Error Creating Dir: \"\" error: mkdir : no such file or directory")
+	})
+	err := ext.Start(context.Background(), componenttest.NewNopHost())
+	require.ErrorContains(t, err, "failed to initialize badger storage")
+	require.ErrorContains(t, err, "/bad/path")
 }
 
-func makeStorageExtension(t *testing.T, memstoreName string) component.Component {
-	extensionFactory := NewFactory()
-
-	ctx := context.Background()
-	telemetrySettings := component.TelemetrySettings{
+func noopTelemetrySettings() component.TelemetrySettings {
+	return component.TelemetrySettings{
 		Logger:         zap.L(),
 		TracerProvider: nooptrace.NewTracerProvider(),
 		MeterProvider:  noopmetric.NewMeterProvider(),
 	}
+}
+
+func makeStorageExtenion(t *testing.T, config *Config) component.Component {
+	extensionFactory := NewFactory()
+	ctx := context.Background()
+	storageExtension, err := extensionFactory.CreateExtension(ctx,
+		extension.CreateSettings{
+			ID:                ID,
+			TelemetrySettings: noopTelemetrySettings(),
+			BuildInfo:         component.NewDefaultBuildInfo(),
+		},
+		config,
+	)
+	require.NoError(t, err)
+	return storageExtension
+}
+
+func startStorageExtension(t *testing.T, memstoreName string) component.Component {
 	config := &Config{
 		Memory: map[string]memoryCfg.Configuration{
 			memstoreName: {MaxTraces: 10000},
 		},
 	}
-	err := config.Validate()
-	require.NoError(t, err)
-
-	storageExtension, err := extensionFactory.CreateExtension(ctx, extension.CreateSettings{
-		ID:                ID,
-		TelemetrySettings: telemetrySettings,
-		BuildInfo:         component.NewDefaultBuildInfo(),
-	}, config)
-	require.NoError(t, err)
+	require.NoError(t, config.Validate())
 
-	host := componenttest.NewNopHost()
-	err = storageExtension.Start(ctx, host)
+	storageExtension := makeStorageExtenion(t, config)
+	err := storageExtension.Start(context.Background(), componenttest.NewNopHost())
 	require.NoError(t, err)
-	t.Cleanup(func() { require.NoError(t, storageExtension.Shutdown(ctx)) })
-
+	t.Cleanup(func() {
+		require.NoError(t, storageExtension.Shutdown(context.Background()))
+	})
 	return storageExtension
 }