code-review-fixes

Signed-off-by: Yuri Shkuro <[email protected]>

storage_v2/factoryadapter/reader.go

@@ -49,7 +49,10 @@ func (tr *TraceReader) GetTraces(
 			// TODO start/end times are not supported by v1 reader
 			// https://github.com/jaegertracing/jaeger/pull/6242
 			t, err := tr.spanReader.GetTrace(ctx, model.TraceIDFromOTEL(idParams.TraceID))
-			if err != nil && !errors.Is(err, spanstore.ErrTraceNotFound) {
+			if err != nil {
+				if errors.Is(err, spanstore.ErrTraceNotFound) {
+					continue
+				}
 				yield(nil, err)
 				return
 			}

storage_v2/factoryadapter/reader_test.go

@@ -81,20 +81,51 @@ func TestTraceReader_GetTracesDelegatesSuccessResponse(t *testing.T) {
 }
 
 func TestTraceReader_GetTracesErrorResponse(t *testing.T) {
-	sr := new(spanStoreMocks.Reader)
-	testErr := errors.New("test error")
-	sr.On("GetTrace", mock.Anything, mock.Anything).Return(nil, testErr)
-	traceReader := &TraceReader{
-		spanReader: sr,
-	}
-	traces, err := iter.FlattenWithErrors(traceReader.GetTraces(
-		context.Background(),
-		tracestore.GetTraceParams{
-			TraceID: pcommon.TraceID([]byte{0, 0, 0, 0, 0, 0, 0, 2, 0, 0, 0, 0, 0, 0, 0, 3}),
+	testCases := []struct {
+		name          string
+		firstErr      error
+		expectedErr   error
+		expectedIters int
+	}{
+		{
+			name:          "real error aborts iterator",
+			firstErr:      assert.AnError,
+			expectedErr:   assert.AnError,
+			expectedIters: 0, // technically 1 but FlattenWithErrors makes it 0
 		},
-	))
-	require.ErrorIs(t, err, testErr)
-	require.Empty(t, traces)
+		{
+			name:          "trace not found error skips iteration",
+			firstErr:      spanstore.ErrTraceNotFound,
+			expectedErr:   nil,
+			expectedIters: 1,
+		},
+		{
+			name:          "no error produces two iterations",
+			firstErr:      nil,
+			expectedErr:   nil,
+			expectedIters: 2,
+		},
+	}
+	traceID := func(i byte) tracestore.GetTraceParams {
+		return tracestore.GetTraceParams{
+			TraceID: pcommon.TraceID([]byte{0, 0, 0, 0, 0, 0, 0, 2, 0, 0, 0, 0, 0, 0, 0, i}),
+		}
+	}
+	for _, test := range testCases {
+		t.Run(test.name, func(t *testing.T) {
+			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{
+				spanReader: sr,
+			}
+			traces, err := iter.FlattenWithErrors(traceReader.GetTraces(
+				context.Background(), traceID(1), traceID(2),
+			))
+			assert.ErrorIs(t, err, test.expectedErr)
+			assert.Len(t, traces, test.expectedIters)
+		})
+	}
 }
 
 func TestTraceReader_GetTracesEarlyStop(t *testing.T) {

storage_v2/tracestore/reader.go

@@ -63,13 +63,16 @@ type Reader interface {
 	FindTraceIDs(ctx context.Context, query TraceQueryParams) iter.Seq2[[]pcommon.TraceID, error]
 }
 
-// GetTraceParams contains single-trace parameters for a GetTraces.
+// GetTraceParams contains single-trace parameters for a GetTraces request.
 // Some storage backends (e.g. Tempo) perform GetTraces much more efficiently
 // if they know the approximate time range of the trace.
 type GetTraceParams struct {
-	TraceID pcommon.TraceID // required
-	Start   time.Time       // optional
-	End     time.Time       // optional
+	// TraceID is the ID of the trace to retrieve. Required.
+	TraceID pcommon.TraceID
+	// Start of the time interval to search for trace ID. Optional.
+	Start time.Time
+	// End of the time interval to search for trace ID. Optional.
+	End time.Time
 }
 
 // TraceQueryParams contains parameters of a trace query.