Repository: jaegertracing/jaeger (go)

Issue:
Change jaegerstorage Extension To Hold v2 Storage Factories
The `jaegerstorage` extension currently holds v1 factories and casts them to a v2 factory via the v1adapter when a factory is requested via `GetTraceStoreFactory`. In this ticket, we want to change the extension such that it holds the v2 factories to unblock the path for a native v2 storage implementation. 

Pull request:
[storage] Change storage extension to hold v2 factories
## Which problem is this PR solving?
- Resolves #6697

## Description of the changes
- This PR refactors the `jaegerstorage` extension to hold the new `tracestore.Factory` instead of the v1 `storage.Factory`. Currently, all storage implementations are wrapped into the v1adapter. However, this PR unblocks the path for a native v2 storage implementation. 

## How was this change tested?
- CI

## Checklist
- [x] I have read https://github.com/jaegertracing/jaeger/blob/master/CONTRIBUTING_GUIDELINES.md
- [x] I have signed all commits
- [x] I have added unit tests for the new functionality
- [x] I have run lint and test steps successfully
  - for `jaeger`: `make lint test`
  - for `jaeger-ui`: `npm run lint` and `npm run test`


Patch:
diff --git a/cmd/jaeger/internal/extension/jaegerstorage/extension.go b/cmd/jaeger/internal/extension/jaegerstorage/extension.go
index 3a7439b3..a24a93ab 100644
--- a/cmd/jaeger/internal/extension/jaegerstorage/extension.go
+++ b/cmd/jaeger/internal/extension/jaegerstorage/extension.go
@@ -29,20 +29,20 @@ var _ Extension = (*storageExt)(nil)
 
 type Extension interface {
 	extension.Extension
-	TraceStorageFactory(name string) (storage.Factory, bool)
+	TraceStorageFactory(name string) (tracestore.Factory, bool)
 	MetricStorageFactory(name string) (storage.MetricStoreFactory, bool)
 }
 
 type storageExt struct {
 	config           *Config
 	telset           component.TelemetrySettings
-	factories        map[string]storage.Factory
+	factories        map[string]tracestore.Factory
 	metricsFactories map[string]storage.MetricStoreFactory
 }
 
 // getStorageFactory locates the extension in Host and retrieves
 // a trace storage factory from it with the given name.
-func getStorageFactory(name string, host component.Host) (storage.Factory, error) {
+func getStorageFactory(name string, host component.Host) (tracestore.Factory, error) {
 	ext, err := findExtension(host)
 	if err != nil {
 		return nil, err
@@ -80,7 +80,7 @@ func GetTraceStoreFactory(name string, host component.Host) (tracestore.Factory,
 		return nil, err
 	}
 
-	return v1adapter.NewFactory(f), nil
+	return f, nil
 }
 
 func GetSamplingStoreFactory(name string, host component.Host) (storage.SamplingStoreFactory, error) {
@@ -137,7 +137,7 @@ func newStorageExt(config *Config, telset component.TelemetrySettings) *storageE
 	return &storageExt{
 		config:           config,
 		telset:           telset,
-		factories:        make(map[string]storage.Factory),
+		factories:        make(map[string]tracestore.Factory),
 		metricsFactories: make(map[string]storage.MetricStoreFactory),
 	}
 }
@@ -157,17 +157,18 @@ func (s *storageExt) Start(_ context.Context, host component.Host) error {
 	}
 	for storageName, cfg := range s.config.TraceBackends {
 		s.telset.Logger.Sugar().Infof("Initializing storage '%s'", storageName)
-		var factory storage.Factory
+		var factory tracestore.Factory
+		var v1Factory storage.Factory
 		var err error = errors.New("empty configuration")
 		switch {
 		case cfg.Memory != nil:
-			factory, err = memory.NewFactoryWithConfig(
+			v1Factory, err = memory.NewFactoryWithConfig(
 				*cfg.Memory,
 				scopedMetricsFactory(storageName, "memory", "tracestore"),
 				s.telset.Logger,
 			), nil
 		case cfg.Badger != nil:
-			factory, err = badger.NewFactoryWithConfig(
+			v1Factory, err = badger.NewFactoryWithConfig(
 				*cfg.Badger,
 				scopedMetricsFactory(storageName, "badger", "tracestore"),
 				s.telset.Logger)
@@ -175,21 +176,21 @@ func (s *storageExt) Start(_ context.Context, host component.Host) error {
 			grpcTelset := telset
 			grpcTelset.Metrics = scopedMetricsFactory(storageName, "grpc", "tracestore")
 			//nolint: contextcheck
-			factory, err = grpc.NewFactoryWithConfig(*cfg.GRPC, grpcTelset)
+			v1Factory, err = grpc.NewFactoryWithConfig(*cfg.GRPC, grpcTelset)
 		case cfg.Cassandra != nil:
-			factory, err = cassandra.NewFactoryWithConfig(
+			v1Factory, err = cassandra.NewFactoryWithConfig(
 				*cfg.Cassandra,
 				scopedMetricsFactory(storageName, "cassandra", "tracestore"),
 				s.telset.Logger,
 			)
 		case cfg.Elasticsearch != nil:
-			factory, err = es.NewFactoryWithConfig(
+			v1Factory, err = es.NewFactoryWithConfig(
 				*cfg.Elasticsearch,
 				scopedMetricsFactory(storageName, "elasticsearch", "tracestore"),
 				s.telset.Logger,
 			)
 		case cfg.Opensearch != nil:
-			factory, err = es.NewFactoryWithConfig(
+			v1Factory, err = es.NewFactoryWithConfig(
 				*cfg.Opensearch,
 				scopedMetricsFactory(storageName, "opensearch", "tracestore"),
 				s.telset.Logger,
@@ -198,6 +199,11 @@ func (s *storageExt) Start(_ context.Context, host component.Host) error {
 		if err != nil {
 			return fmt.Errorf("failed to initialize storage '%s': %w", storageName, err)
 		}
+
+		if v1Factory != nil {
+			factory = v1adapter.NewFactory(v1Factory)
+		}
+
 		s.factories[storageName] = factory
 	}
 
@@ -234,7 +240,7 @@ func (s *storageExt) Shutdown(context.Context) error {
 	return errors.Join(errs...)
 }
 
-func (s *storageExt) TraceStorageFactory(name string) (storage.Factory, bool) {
+func (s *storageExt) TraceStorageFactory(name string) (tracestore.Factory, bool) {
 	f, ok := s.factories[name]
 	return f, ok
 }
diff --git a/internal/storage/v2/v1adapter/factory.go b/internal/storage/v2/v1adapter/factory.go
index b6ed9daf..fa2b7094 100644
--- a/internal/storage/v2/v1adapter/factory.go
+++ b/internal/storage/v2/v1adapter/factory.go
@@ -4,7 +4,6 @@
 package v1adapter
 
 import (
-	"context"
 	"io"
 
 	storage_v1 "github.com/jaegertracing/jaeger/internal/storage/v1"
@@ -49,7 +48,7 @@ func NewFactory(ss storage_v1.Factory) tracestore.Factory {
 }
 
 // Close implements tracestore.Factory.
-func (f *Factory) Close(_ context.Context) error {
+func (f *Factory) Close() error {
 	if closer, ok := f.ss.(io.Closer); ok {
 		return closer.Close()
 	}


Test patch:
diff --git a/cmd/jaeger/internal/exporters/storageexporter/exporter_test.go b/cmd/jaeger/internal/exporters/storageexporter/exporter_test.go
index 732f9f0c..98afc706 100644
--- a/cmd/jaeger/internal/exporters/storageexporter/exporter_test.go
+++ b/cmd/jaeger/internal/exporters/storageexporter/exporter_test.go
@@ -5,7 +5,6 @@ package storageexporter
 
 import (
 	"context"
-	"errors"
 	"testing"
 
 	"github.com/open-telemetry/opentelemetry-collector-contrib/extension/storage/storagetest"
@@ -25,13 +24,14 @@ import (
 	"github.com/jaegertracing/jaeger/internal/storage/v1/memory"
 	factoryMocks "github.com/jaegertracing/jaeger/internal/storage/v1/mocks"
 	"github.com/jaegertracing/jaeger/internal/storage/v2/api/tracestore"
+	tracestoreMocks "github.com/jaegertracing/jaeger/internal/storage/v2/api/tracestore/mocks"
 	"github.com/jaegertracing/jaeger/pkg/iter"
 	"github.com/jaegertracing/jaeger/pkg/otelsemconv"
 )
 
 type mockStorageExt struct {
 	name           string
-	factory        *factoryMocks.Factory
+	factory        *tracestoreMocks.Factory
 	metricsFactory *factoryMocks.MetricStoreFactory
 }
 
@@ -45,7 +45,7 @@ func (*mockStorageExt) Shutdown(context.Context) error {
 	panic("not implemented")
 }
 
-func (m *mockStorageExt) TraceStorageFactory(name string) (storage.Factory, bool) {
+func (m *mockStorageExt) TraceStorageFactory(name string) (tracestore.Factory, bool) {
 	if m.name == name {
 		return m.factory, true
 	}
@@ -79,8 +79,8 @@ func TestExporterStartBadNameError(t *testing.T) {
 }
 
 func TestExporterStartBadSpanstoreError(t *testing.T) {
-	factory := new(factoryMocks.Factory)
-	factory.On("CreateSpanWriter").Return(nil, errors.New("mocked error"))
+	factory := new(tracestoreMocks.Factory)
+	factory.On("CreateTraceWriter").Return(nil, assert.AnError)
 
 	host := storagetest.NewStorageHost()
 	host.WithExtension(jaegerstorage.ID, &mockStorageExt{
@@ -94,7 +94,7 @@ func TestExporterStartBadSpanstoreError(t *testing.T) {
 		},
 	}
 	err := exp.start(context.Background(), host)
-	require.ErrorContains(t, err, "mocked error")
+	require.ErrorIs(t, err, assert.AnError)
 }
 
 func TestExporter(t *testing.T) {
diff --git a/cmd/jaeger/internal/extension/jaegerquery/server_test.go b/cmd/jaeger/internal/extension/jaegerquery/server_test.go
index 95c2ee6e..4596a5c0 100644
--- a/cmd/jaeger/internal/extension/jaegerquery/server_test.go
+++ b/cmd/jaeger/internal/extension/jaegerquery/server_test.go
@@ -27,16 +27,15 @@ import (
 	v2querysvc "github.com/jaegertracing/jaeger/cmd/query/app/querysvc/v2/querysvc"
 	"github.com/jaegertracing/jaeger/internal/grpctest"
 	"github.com/jaegertracing/jaeger/internal/storage/v1"
-	"github.com/jaegertracing/jaeger/internal/storage/v1/api/dependencystore"
-	depsmocks "github.com/jaegertracing/jaeger/internal/storage/v1/api/dependencystore/mocks"
 	"github.com/jaegertracing/jaeger/internal/storage/v1/api/metricstore"
 	metricstoremocks "github.com/jaegertracing/jaeger/internal/storage/v1/api/metricstore/mocks"
 	"github.com/jaegertracing/jaeger/internal/storage/v1/api/spanstore"
 	spanstoremocks "github.com/jaegertracing/jaeger/internal/storage/v1/api/spanstore/mocks"
+	"github.com/jaegertracing/jaeger/internal/storage/v2/api/depstore"
+	depstoremocks "github.com/jaegertracing/jaeger/internal/storage/v2/api/depstore/mocks"
 	"github.com/jaegertracing/jaeger/internal/storage/v2/api/tracestore"
 	tracestoremocks "github.com/jaegertracing/jaeger/internal/storage/v2/api/tracestore/mocks"
 	"github.com/jaegertracing/jaeger/internal/storage/v2/v1adapter"
-	"github.com/jaegertracing/jaeger/pkg/metrics"
 	"github.com/jaegertracing/jaeger/pkg/telemetry"
 	"github.com/jaegertracing/jaeger/pkg/testutils"
 )
@@ -45,32 +44,25 @@ type fakeFactory struct {
 	name string
 }
 
-func (ff fakeFactory) CreateDependencyReader() (dependencystore.Reader, error) {
+func (ff fakeFactory) CreateDependencyReader() (depstore.Reader, error) {
 	if ff.name == "need-dependency-reader-error" {
 		return nil, errors.New("test-error")
 	}
-	return &depsmocks.Reader{}, nil
+	return &depstoremocks.Reader{}, nil
 }
 
-func (ff fakeFactory) CreateSpanReader() (spanstore.Reader, error) {
+func (ff fakeFactory) CreateTraceReader() (tracestore.Reader, error) {
 	if ff.name == "need-span-reader-error" {
 		return nil, errors.New("test-error")
 	}
-	return &spanstoremocks.Reader{}, nil
+	return &tracestoremocks.Reader{}, nil
 }
 
-func (ff fakeFactory) CreateSpanWriter() (spanstore.Writer, error) {
+func (ff fakeFactory) CreateTraceWriter() (tracestore.Writer, error) {
 	if ff.name == "need-span-writer-error" {
 		return nil, errors.New("test-error")
 	}
-	return &spanstoremocks.Writer{}, nil
-}
-
-func (ff fakeFactory) Initialize(metrics.Factory, *zap.Logger) error {
-	if ff.name == "need-initialize-error" {
-		return errors.New("test-error")
-	}
-	return nil
+	return &tracestoremocks.Writer{}, nil
 }
 
 type fakeMetricsFactory struct {
@@ -96,7 +88,7 @@ type fakeStorageExt struct{}
 
 var _ jaegerstorage.Extension = (*fakeStorageExt)(nil)
 
-func (fakeStorageExt) TraceStorageFactory(name string) (storage.Factory, bool) {
+func (fakeStorageExt) TraceStorageFactory(name string) (tracestore.Factory, bool) {
 	if name == "need-factory-error" {
 		return nil, false
 	}
diff --git a/cmd/jaeger/internal/extension/jaegerstorage/extension_test.go b/cmd/jaeger/internal/extension/jaegerstorage/extension_test.go
index caf963c9..c9d32c6d 100644
--- a/cmd/jaeger/internal/extension/jaegerstorage/extension_test.go
+++ b/cmd/jaeger/internal/extension/jaegerstorage/extension_test.go
@@ -21,15 +21,12 @@ import (
 	nooptrace "go.opentelemetry.io/otel/trace/noop"
 	"go.uber.org/zap"
 
-	"github.com/jaegertracing/jaeger/internal/storage/v1"
-	"github.com/jaegertracing/jaeger/internal/storage/v1/api/dependencystore"
-	"github.com/jaegertracing/jaeger/internal/storage/v1/api/spanstore"
 	"github.com/jaegertracing/jaeger/internal/storage/v1/badger"
 	"github.com/jaegertracing/jaeger/internal/storage/v1/cassandra"
 	"github.com/jaegertracing/jaeger/internal/storage/v1/grpc"
 	"github.com/jaegertracing/jaeger/internal/storage/v1/memory"
+	"github.com/jaegertracing/jaeger/internal/storage/v2/api/tracestore"
 	esCfg "github.com/jaegertracing/jaeger/pkg/es/config"
-	"github.com/jaegertracing/jaeger/pkg/metrics"
 	promCfg "github.com/jaegertracing/jaeger/pkg/prometheus/config"
 )
 
@@ -37,19 +34,11 @@ type errorFactory struct {
 	closeErr error
 }
 
-func (errorFactory) Initialize(metrics.Factory, *zap.Logger) error {
+func (errorFactory) CreateTraceReader() (tracestore.Reader, error) {
 	panic("not implemented")
 }
 
-func (errorFactory) CreateSpanReader() (spanstore.Reader, error) {
-	panic("not implemented")
-}
-
-func (errorFactory) CreateSpanWriter() (spanstore.Writer, error) {
-	panic("not implemented")
-}
-
-func (errorFactory) CreateDependencyReader() (dependencystore.Reader, error) {
+func (errorFactory) CreateTraceWriter() (tracestore.Writer, error) {
 	panic("not implemented")
 }
 
@@ -88,7 +77,7 @@ func TestStorageExtensionType(t *testing.T) {
 func TestStorageFactoryBadShutdownError(t *testing.T) {
 	shutdownError := errors.New("shutdown error")
 	ext := storageExt{
-		factories: map[string]storage.Factory{
+		factories: map[string]tracestore.Factory{
 			"foo": errorFactory{closeErr: shutdownError},
 		},
 	}
diff --git a/cmd/jaeger/internal/integration/storagecleaner/extension_test.go b/cmd/jaeger/internal/integration/storagecleaner/extension_test.go
index 430eb31f..75d6f1cf 100644
--- a/cmd/jaeger/internal/integration/storagecleaner/extension_test.go
+++ b/cmd/jaeger/internal/integration/storagecleaner/extension_test.go
@@ -21,16 +21,17 @@ import (
 
 	"github.com/jaegertracing/jaeger/cmd/jaeger/internal/extension/jaegerstorage"
 	"github.com/jaegertracing/jaeger/internal/storage/v1"
-	factoryMocks "github.com/jaegertracing/jaeger/internal/storage/v1/mocks"
+	"github.com/jaegertracing/jaeger/internal/storage/v2/api/tracestore"
+	tracestoremocks "github.com/jaegertracing/jaeger/internal/storage/v2/api/tracestore/mocks"
 )
 
 var (
 	_ jaegerstorage.Extension = (*mockStorageExt)(nil)
-	_ storage.Factory         = (*PurgerFactory)(nil)
+	_ tracestore.Factory      = (*PurgerFactory)(nil)
 )
 
 type PurgerFactory struct {
-	factoryMocks.Factory
+	tracestoremocks.Factory
 	err error
 }
 
@@ -40,7 +41,7 @@ func (f *PurgerFactory) Purge(_ context.Context) error {
 
 type mockStorageExt struct {
 	name           string
-	factory        storage.Factory
+	factory        tracestore.Factory
 	metricsFactory storage.MetricStoreFactory
 }
 
@@ -52,7 +53,7 @@ func (*mockStorageExt) Shutdown(context.Context) error {
 	panic("not implemented")
 }
 
-func (m *mockStorageExt) TraceStorageFactory(name string) (storage.Factory, bool) {
+func (m *mockStorageExt) TraceStorageFactory(name string) (tracestore.Factory, bool) {
 	if m.name == name {
 		return m.factory, true
 	}
@@ -69,7 +70,7 @@ func (m *mockStorageExt) MetricStorageFactory(name string) (storage.MetricStoreF
 func TestStorageCleanerExtension(t *testing.T) {
 	tests := []struct {
 		name    string
-		factory storage.Factory
+		factory tracestore.Factory
 		status  int
 	}{
 		{
@@ -119,11 +120,11 @@ func TestStorageCleanerExtension(t *testing.T) {
 func TestGetStorageFactoryError(t *testing.T) {
 	tests := []struct {
 		name    string
-		factory storage.Factory
+		factory tracestore.Factory
 	}{
 		{
 			name:    "NoFactory",
-			factory: &factoryMocks.Factory{},
+			factory: &tracestoremocks.Factory{},
 		},
 		{
 			name:    "BadFactory",
diff --git a/internal/storage/v2/v1adapter/factory_test.go b/internal/storage/v2/v1adapter/factory_test.go
index e2caea0e..136a9633 100644
--- a/internal/storage/v2/v1adapter/factory_test.go
+++ b/internal/storage/v2/v1adapter/factory_test.go
@@ -5,6 +5,7 @@ package v1adapter
 
 import (
 	"errors"
+	"io"
 	"testing"
 
 	"github.com/stretchr/testify/require"
@@ -12,12 +13,13 @@ import (
 	storage_v1 "github.com/jaegertracing/jaeger/internal/storage/v1"
 	dependencyStoreMocks "github.com/jaegertracing/jaeger/internal/storage/v1/api/dependencystore/mocks"
 	spanstoreMocks "github.com/jaegertracing/jaeger/internal/storage/v1/api/spanstore/mocks"
+	"github.com/jaegertracing/jaeger/internal/storage/v1/grpc"
 	factoryMocks "github.com/jaegertracing/jaeger/internal/storage/v1/mocks"
 	"github.com/jaegertracing/jaeger/internal/storage/v2/api/depstore"
 	"github.com/jaegertracing/jaeger/internal/storage/v2/api/tracestore"
 )
 
-func TestNewTraceReader(t *testing.T) {
+func TestNewFactory(t *testing.T) {
 	mockFactory := new(factoryMocks.Factory)
 	mockPurger := new(factoryMocks.Purger)
 	mockSamplingStoreFactory := new(factoryMocks.SamplingStoreFactory)
@@ -28,9 +30,13 @@ func TestNewTraceReader(t *testing.T) {
 		expectedInterfaces []any
 	}{
 		{
-			name:               "No extra interfaces",
-			factory:            mockFactory,
-			expectedInterfaces: []any{(*tracestore.Factory)(nil), (*depstore.Factory)(nil)},
+			name:    "No extra interfaces",
+			factory: mockFactory,
+			expectedInterfaces: []any{
+				(*tracestore.Factory)(nil),
+				(*depstore.Factory)(nil),
+				(*io.Closer)(nil),
+			},
 		},
 		{
 			name: "Implements Purger",
@@ -41,6 +47,7 @@ func TestNewTraceReader(t *testing.T) {
 			expectedInterfaces: []any{
 				(*tracestore.Factory)(nil),
 				(*depstore.Factory)(nil),
+				(*io.Closer)(nil),
 				(*storage_v1.Purger)(nil),
 			},
 		},
@@ -53,6 +60,7 @@ func TestNewTraceReader(t *testing.T) {
 			expectedInterfaces: []any{
 				(*tracestore.Factory)(nil),
 				(*depstore.Factory)(nil),
+				(*io.Closer)(nil),
 				(*storage_v1.SamplingStoreFactory)(nil),
 			},
 		},
@@ -66,6 +74,7 @@ func TestNewTraceReader(t *testing.T) {
 			expectedInterfaces: []any{
 				(*tracestore.Factory)(nil),
 				(*depstore.Factory)(nil),
+				(*io.Closer)(nil),
 				(*storage_v1.Purger)(nil),
 				(*storage_v1.SamplingStoreFactory)(nil),
 			},
@@ -82,6 +91,20 @@ func TestNewTraceReader(t *testing.T) {
 	}
 }
 
+func TestAdapterCloseNotOk(t *testing.T) {
+	f := NewFactory(&factoryMocks.Factory{})
+	closer, ok := f.(io.Closer)
+	require.True(t, ok)
+	require.NoError(t, closer.Close())
+}
+
+func TestAdapterClose(t *testing.T) {
+	f := NewFactory(grpc.NewFactory())
+	closer, ok := f.(io.Closer)
+	require.True(t, ok)
+	require.NoError(t, closer.Close())
+}
+
 func TestAdapterCreateTraceReader(t *testing.T) {
 	f1 := new(factoryMocks.Factory)
 	f1.On("CreateSpanReader").Return(new(spanstoreMocks.Reader), nil)