[refactor] Consolidate v1/v2 reader factory adapter functionality (jaegertracing#7019)

## Which problem is this PR solving?
- Towards jaegertracing#6979

## Description of the changes
- This PR consolidates the v1/v2 reader adapter functionality by
combining `GetV1Reader` with `NewSpanReader`

## 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`

---------

Signed-off-by: Mahad Zaryab <[email protected]>

Author: mahadzaryab1

cmd/jaeger/internal/extension/jaegerquery/server.go

@@ -185,11 +185,7 @@ func getV1Adapters(
 	reader tracestore.Reader,
 	writer tracestore.Writer,
 ) (spanstore.Reader, spanstore.Writer) {
-	v1Reader, ok := v1adapter.GetV1Reader(reader)
-	if !ok {
-		v1Reader = v1adapter.NewSpanReader(reader)
-	}
-
+	v1Reader := v1adapter.GetV1Reader(reader)
 	v1Writer, ok := v1adapter.GetV1Writer(writer)
 	if !ok {
 		v1Writer = v1adapter.NewSpanWriter(writer)

cmd/jaeger/internal/extension/jaegerquery/server_test.go

@@ -356,7 +356,7 @@ func TestGetV1Adapters(t *testing.T) {
 			name:           "native tracestore.Reader and tracestore.Writer",
 			reader:         &tracestoremocks.Reader{},
 			writer:         &tracestoremocks.Writer{},
-			expectedReader: v1adapter.NewSpanReader(&tracestoremocks.Reader{}),
+			expectedReader: &v1adapter.SpanReader{},
 			expectedWriter: v1adapter.NewSpanWriter(&tracestoremocks.Writer{}),
 		},
 		{
@@ -371,7 +371,7 @@ func TestGetV1Adapters(t *testing.T) {
 	for _, test := range tests {
 		t.Run(test.name, func(t *testing.T) {
 			gotReader, gotWriter := getV1Adapters(test.reader, test.writer)
-			require.Equal(t, test.expectedReader, gotReader)
+			require.IsType(t, test.expectedReader, gotReader)
 			require.Equal(t, test.expectedWriter, gotWriter)
 		})
 	}

cmd/query/app/querysvc/query_service.go

@@ -55,12 +55,7 @@ type TraceQueryParameters struct {
 
 // NewQueryService returns a new QueryService.
 func NewQueryService(traceReader tracestore.Reader, dependencyReader depstore.Reader, options QueryServiceOptions) *QueryService {
-	spanReader, ok := v1adapter.GetV1Reader(traceReader)
-	if !ok {
-		// if the spanstore.Reader is not available, downgrade the native tracestore.Reader to
-		// a spanstore.Reader
-		spanReader = v1adapter.NewSpanReader(traceReader)
-	}
+	spanReader := v1adapter.GetV1Reader(traceReader)
 	qsvc := &QueryService{
 		spanReader:       spanReader,
 		dependencyReader: dependencyReader,

internal/storage/v2/v1adapter/spanreader.go

@@ -23,12 +23,6 @@ type SpanReader struct {
 	traceReader tracestore.Reader
 }
 
-func NewSpanReader(traceReader tracestore.Reader) *SpanReader {
-	return &SpanReader{
-		traceReader: traceReader,
-	}
-}
-
 func (sr *SpanReader) GetTrace(ctx context.Context, query spanstore.GetTraceParameters) (*model.Trace, error) {
 	getTracesIter := sr.traceReader.GetTraces(ctx, tracestore.GetTraceParams{
 		TraceID: FromV1TraceID(query.TraceID),

internal/storage/v2/v1adapter/spanreader_test.go

@@ -122,7 +122,9 @@ func TestSpanReader_GetTrace(t *testing.T) {
 				yield(test.traces, test.err)
 			})).Once()
 
-		sr := NewSpanReader(&tr)
+		sr := SpanReader{
+			traceReader: &tr,
+		}
 		trace, err := sr.GetTrace(context.Background(), test.query)
 		require.ErrorIs(t, err, test.expectedErr)
 		require.Equal(t, test.expectedTrace, trace)
@@ -159,7 +161,9 @@ func TestSpanReader_GetServices(t *testing.T) {
 		tr.On("GetServices", mock.Anything).
 			Return(test.services, test.err).Once()
 
-		sr := NewSpanReader(&tr)
+		sr := SpanReader{
+			traceReader: &tr,
+		}
 		services, err := sr.GetServices(context.Background())
 		require.ErrorIs(t, err, test.expectedErr)
 		require.Equal(t, test.expectedServices, services)
@@ -222,7 +226,9 @@ func TestSpanReader_GetOperations(t *testing.T) {
 		tr.On("GetOperations", mock.Anything, test.expectedQuery).
 			Return(test.operations, test.err).Once()
 
-		sr := NewSpanReader(&tr)
+		sr := SpanReader{
+			traceReader: &tr,
+		}
 		ops, err := sr.GetOperations(context.Background(), test.query)
 		require.ErrorIs(t, err, test.expectedErr)
 		require.Equal(t, test.expectedOperations, ops)
@@ -333,7 +339,9 @@ func TestSpanReader_FindTraces(t *testing.T) {
 				yield(test.traces, test.err)
 			})).Once()
 
-		sr := NewSpanReader(&tr)
+		sr := SpanReader{
+			traceReader: &tr,
+		}
 		traces, err := sr.FindTraces(context.Background(), test.query)
 		require.ErrorIs(t, err, test.expectedErr)
 		require.Equal(t, test.expectedTraces, traces)
@@ -405,7 +413,9 @@ func TestSpanReader_FindTraceIDs(t *testing.T) {
 				yield(test.traceIDs, test.err)
 			})).Once()
 
-		sr := NewSpanReader(&tr)
+		sr := SpanReader{
+			traceReader: &tr,
+		}
 		traceIDs, err := sr.FindTraceIDs(context.Background(), test.query)
 		require.ErrorIs(t, err, test.expectedErr)
 		require.Equal(t, test.expectedTraceIDs, traceIDs)

internal/storage/v2/v1adapter/tracereader.go

@@ -23,11 +23,13 @@ type TraceReader struct {
 	spanReader spanstore.Reader
 }
 
-func GetV1Reader(reader tracestore.Reader) (spanstore.Reader, bool) {
+func GetV1Reader(reader tracestore.Reader) spanstore.Reader {
 	if tr, ok := reader.(*TraceReader); ok {
-		return tr.spanReader, ok
+		return tr.spanReader
+	}
+	return &SpanReader{
+		traceReader: reader,
 	}
-	return nil, false
 }
 
 func NewTraceReader(spanReader spanstore.Reader) *TraceReader {

internal/storage/v2/v1adapter/tracereader_test.go

@@ -17,33 +17,34 @@ import (
 
 	"github.com/jaegertracing/jaeger-idl/model/v1"
 	"github.com/jaegertracing/jaeger/internal/jiter"
-	dependencyStoreMocks "github.com/jaegertracing/jaeger/internal/storage/v1/api/dependencystore/mocks"
+	dependencystoremocks "github.com/jaegertracing/jaeger/internal/storage/v1/api/dependencystore/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/v1/memory"
+	spanstoremocks "github.com/jaegertracing/jaeger/internal/storage/v1/api/spanstore/mocks"
 	"github.com/jaegertracing/jaeger/internal/storage/v2/api/depstore"
 	"github.com/jaegertracing/jaeger/internal/storage/v2/api/tracestore"
 	tracestoremocks "github.com/jaegertracing/jaeger/internal/storage/v2/api/tracestore/mocks"
 )
 
-func TestGetV1Reader_NoError(t *testing.T) {
-	memstore := memory.NewStore()
-	traceReader := &TraceReader{
-		spanReader: memstore,
-	}
-	v1Reader, ok := GetV1Reader(traceReader)
-	require.True(t, ok)
-	require.Equal(t, memstore, v1Reader)
-}
+func TestGetV1Reader(t *testing.T) {
+	t.Run("wrapped v1 reader", func(t *testing.T) {
+		reader := new(spanstoremocks.Reader)
+		traceReader := &TraceReader{
+			spanReader: reader,
+		}
+		v1Reader := GetV1Reader(traceReader)
+		require.Equal(t, reader, v1Reader)
+	})
 
-func TestGetV1Reader_Error(t *testing.T) {
-	fr := new(tracestoremocks.Reader)
-	_, ok := GetV1Reader(fr)
-	require.False(t, ok)
+	t.Run("native v2 reader", func(t *testing.T) {
+		reader := new(tracestoremocks.Reader)
+		v1Reader := GetV1Reader(reader)
+		require.IsType(t, &SpanReader{}, v1Reader)
+		require.Equal(t, reader, v1Reader.(*SpanReader).traceReader)
+	})
 }
 
 func TestTraceReader_GetTracesDelegatesSuccessResponse(t *testing.T) {
-	sr := new(spanStoreMocks.Reader)
+	sr := new(spanstoremocks.Reader)
 	modelTrace := &model.Trace{
 		Spans: []*model.Span{
 			{
@@ -114,7 +115,7 @@ func TestTraceReader_GetTracesErrorResponse(t *testing.T) {
 	}
 	for _, test := range testCases {
 		t.Run(test.name, func(t *testing.T) {
-			sr := new(spanStoreMocks.Reader)
+			sr := new(spanstoremocks.Reader)
 			sr.On("GetTrace", mock.Anything, mock.Anything).Return(&model.Trace{}, test.firstErr).Once()
 			sr.On("GetTrace", mock.Anything, mock.Anything).Return(&model.Trace{}, nil).Once()
 			traceReader := &TraceReader{
@@ -130,7 +131,7 @@ func TestTraceReader_GetTracesErrorResponse(t *testing.T) {
 }
 
 func TestTraceReader_GetTracesEarlyStop(t *testing.T) {
-	sr := new(spanStoreMocks.Reader)
+	sr := new(spanstoremocks.Reader)
 	sr.On(
 		"GetTrace",
 		mock.Anything,
@@ -167,7 +168,7 @@ func TestTraceReader_GetTracesEarlyStop(t *testing.T) {
 }
 
 func TestTraceReader_GetServicesDelegatesToSpanReader(t *testing.T) {
-	sr := new(spanStoreMocks.Reader)
+	sr := new(spanstoremocks.Reader)
 	expectedServices := []string{"service-a", "service-b"}
 	sr.On("GetServices", mock.Anything).Return(expectedServices, nil)
 	traceReader := &TraceReader{
@@ -228,7 +229,7 @@ func TestTraceReader_GetOperationsDelegatesResponse(t *testing.T) {
 
 	for _, test := range tests {
 		t.Run(test.name, func(t *testing.T) {
-			sr := new(spanStoreMocks.Reader)
+			sr := new(spanstoremocks.Reader)
 			sr.On("GetOperations",
 				mock.Anything,
 				spanstore.OperationQueryParameters{
@@ -276,7 +277,7 @@ func TestTraceReader_FindTracesDelegatesSuccessResponse(t *testing.T) {
 			},
 		},
 	}
-	sr := new(spanStoreMocks.Reader)
+	sr := new(spanstoremocks.Reader)
 	now := time.Now()
 	sr.On(
 		"FindTraces",
@@ -351,7 +352,7 @@ func TestTraceReader_FindTracesEdgeCases(t *testing.T) {
 	}
 	for _, test := range tests {
 		t.Run(test.name, func(t *testing.T) {
-			sr := new(spanStoreMocks.Reader)
+			sr := new(spanstoremocks.Reader)
 			sr.On(
 				"FindTraces",
 				mock.Anything,
@@ -373,7 +374,7 @@ func TestTraceReader_FindTracesEdgeCases(t *testing.T) {
 }
 
 func TestTraceReader_FindTracesEarlyStop(t *testing.T) {
-	sr := new(spanStoreMocks.Reader)
+	sr := new(spanstoremocks.Reader)
 	sr.On(
 		"FindTraces",
 		mock.Anything,
@@ -449,7 +450,7 @@ func TestTraceReader_FindTraceIDsDelegatesResponse(t *testing.T) {
 	}
 	for _, test := range tests {
 		t.Run(test.name, func(t *testing.T) {
-			sr := new(spanStoreMocks.Reader)
+			sr := new(spanstoremocks.Reader)
 			now := time.Now()
 			sr.On(
 				"FindTraceIDs",
@@ -497,7 +498,7 @@ func TestDependencyReader_GetDependencies(t *testing.T) {
 		EndTime:   end,
 	}
 	expectedDeps := []model.DependencyLink{{Parent: "parent", Child: "child", CallCount: 12}}
-	mr := new(dependencyStoreMocks.Reader)
+	mr := new(dependencystoremocks.Reader)
 	mr.On("GetDependencies", mock.Anything, end, time.Minute).Return(expectedDeps, nil)
 	dr := NewDependencyReader(mr)
 	deps, err := dr.GetDependencies(context.Background(), query)