[v1][adjuster] Change v1 adjuster interface to not return error and m…

…odify trace in place (#6426)

## Which problem is this PR solving?
- Towards #6417

## Description of the changes
- The v1 adjuster interface returns an error even though none of the
adjusters ever return an error. This was leading to handling errors that
would never be thrown. This PR simplifies the interface by removing the
error return.
- The v1 adjuster interface was also returning the trace that it
modified. However, all the adjusters currently just take the trace in as
a pointer and modify it in place so this return was not necessary.

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

cmd/query/app/http_handler.go

@@ -254,10 +254,7 @@ func (aH *APIHandler) search(w http.ResponseWriter, r *http.Request) {
 func (aH *APIHandler) tracesToResponse(traces []*model.Trace, adjust bool, uiErrors []structuredError) *structuredResponse {
 	uiTraces := make([]*ui.Trace, len(traces))
 	for i, v := range traces {
-		uiTrace, uiErr := aH.convertModelToUI(v, adjust)
-		if uiErr != nil {
-			uiErrors = append(uiErrors, *uiErr)
-		}
+		uiTrace := aH.convertModelToUI(v, adjust)
 		uiTraces[i] = uiTrace
 	}
 
@@ -364,24 +361,12 @@ func (aH *APIHandler) metrics(w http.ResponseWriter, r *http.Request, getMetrics
 	aH.writeJSON(w, r, m)
 }
 
-func (aH *APIHandler) convertModelToUI(trc *model.Trace, adjust bool) (*ui.Trace, *structuredError) {
-	var errs []error
+func (aH *APIHandler) convertModelToUI(trc *model.Trace, adjust bool) *ui.Trace {
 	if adjust {
-		var err error
-		trc, err = aH.queryService.Adjust(trc)
-		if err != nil {
-			errs = append(errs, err)
-		}
+		aH.queryService.Adjust(trc)
 	}
 	uiTrace := uiconv.FromDomain(trc)
-	var uiError *structuredError
-	if err := errors.Join(errs...); err != nil {
-		uiError = &structuredError{
-			Msg:     err.Error(),
-			TraceID: uiTrace.TraceID,
-		}
-	}
-	return uiTrace, uiError
+	return uiTrace
 }
 
 func (*APIHandler) deduplicateDependencies(dependencies []model.DependencyLink) []ui.DependencyLink {

cmd/query/app/http_handler_test.go

@@ -32,7 +32,6 @@ import (
 
 	"github.com/jaegertracing/jaeger/cmd/query/app/querysvc"
 	"github.com/jaegertracing/jaeger/model"
-	"github.com/jaegertracing/jaeger/model/adjuster"
 	ui "github.com/jaegertracing/jaeger/model/json"
 	"github.com/jaegertracing/jaeger/pkg/jtracer"
 	"github.com/jaegertracing/jaeger/pkg/tenancy"
@@ -59,7 +58,6 @@ func (m *IoReaderMock) Read(b []byte) (int, error) {
 var (
 	errStorageMsg = "storage error"
 	errStorage    = errors.New(errStorageMsg)
-	errAdjustment = errors.New("adjustment error")
 
 	httpClient = &http.Client{
 		Timeout: 2 * time.Second,
@@ -375,25 +373,6 @@ func TestGetTraceNotFound(t *testing.T) {
 	require.EqualError(t, err, parsedError(404, "trace not found"))
 }
 
-func TestGetTraceAdjustmentFailure(t *testing.T) {
-	ts := initializeTestServerWithHandler(
-		t,
-		querysvc.QueryServiceOptions{
-			Adjuster: adjuster.Func(func(trace *model.Trace) (*model.Trace, error) {
-				return trace, errAdjustment
-			}),
-		},
-	)
-	ts.spanReader.On("GetTrace", mock.AnythingOfType("*context.valueCtx"), mock.AnythingOfType("spanstore.GetTraceParameters")).
-		Return(mockTrace, nil).Once()
-
-	var response structuredResponse
-	err := getJSON(ts.server.URL+`/api/traces/123456`, &response)
-	require.NoError(t, err)
-	assert.Len(t, response.Errors, 1)
-	assert.EqualValues(t, errAdjustment.Error(), response.Errors[0].Msg)
-}
-
 func TestGetTraceBadTraceID(t *testing.T) {
 	ts := initializeTestServer(t)
 
@@ -564,25 +543,6 @@ func TestSearchByTraceIDFailure(t *testing.T) {
 	require.EqualError(t, err, parsedError(500, whatsamattayou))
 }
 
-func TestSearchModelConversionFailure(t *testing.T) {
-	ts := initializeTestServerWithOptions(
-		t,
-		&tenancy.Manager{},
-		querysvc.QueryServiceOptions{
-			Adjuster: adjuster.Func(func(trace *model.Trace) (*model.Trace, error) {
-				return trace, errAdjustment
-			}),
-		},
-	)
-	ts.spanReader.On("FindTraces", mock.AnythingOfType("*context.valueCtx"), mock.AnythingOfType("*spanstore.TraceQueryParameters")).
-		Return([]*model.Trace{mockTrace}, nil).Once()
-	var response structuredResponse
-	err := getJSON(ts.server.URL+`/api/traces?service=service&start=0&end=0&operation=operation&limit=200&minDuration=20ms`, &response)
-	require.NoError(t, err)
-	assert.Len(t, response.Errors, 1)
-	assert.EqualValues(t, errAdjustment.Error(), response.Errors[0].Msg)
-}
-
 func TestSearchDBFailure(t *testing.T) {
 	ts := initializeTestServer(t)
 	ts.spanReader.On("FindTraces", mock.AnythingOfType("*context.valueCtx"), mock.AnythingOfType("*spanstore.TraceQueryParameters")).

cmd/query/app/querysvc/query_service.go

@@ -128,8 +128,8 @@ func (qs QueryService) ArchiveTrace(ctx context.Context, query spanstore.GetTrac
 }
 
 // Adjust applies adjusters to the trace.
-func (qs QueryService) Adjust(trace *model.Trace) (*model.Trace, error) {
-	return qs.options.Adjuster.Adjust(trace)
+func (qs QueryService) Adjust(trace *model.Trace) {
+	qs.options.Adjuster.Adjust(trace)
 }
 
 // GetDependencies implements dependencystore.Reader.GetDependencies

cmd/query/app/querysvc/query_service_test.go

@@ -15,7 +15,6 @@ import (
 	"go.uber.org/zap"
 
 	"github.com/jaegertracing/jaeger/model"
-	"github.com/jaegertracing/jaeger/model/adjuster"
 	"github.com/jaegertracing/jaeger/pkg/metrics"
 	"github.com/jaegertracing/jaeger/pkg/testutils"
 	"github.com/jaegertracing/jaeger/storage"
@@ -31,8 +30,6 @@ import (
 const millisToNanosMultiplier = int64(time.Millisecond / time.Nanosecond)
 
 var (
-	errAdjustment = errors.New("adjustment error")
-
 	defaultDependencyLookbackDuration = time.Hour * 24
 
 	mockTraceID = model.NewTraceID(0, 123456)
@@ -80,14 +77,6 @@ func withArchiveSpanWriter() testOption {
 	}
 }
 
-func withAdjuster() testOption {
-	return func(_ *testQueryService, options *QueryServiceOptions) {
-		options.Adjuster = adjuster.Func(func(trace *model.Trace) (*model.Trace, error) {
-			return trace, errAdjustment
-		})
-	}
-}
-
 func initializeTestService(optionAppliers ...testOption) *testQueryService {
 	readStorage := &spanstoremocks.Reader{}
 	traceReader := v1adapter.NewTraceReader(readStorage)
@@ -307,15 +296,6 @@ func TestArchiveTraceSuccess(t *testing.T) {
 	require.NoError(t, err)
 }
 
-// Test QueryService.Adjust()
-func TestTraceAdjustmentFailure(t *testing.T) {
-	tqs := initializeTestService(withAdjuster())
-
-	_, err := tqs.queryService.Adjust(mockTrace)
-	require.Error(t, err)
-	assert.EqualValues(t, errAdjustment.Error(), err.Error())
-}
-
 // Test QueryService.GetDependencies()
 func TestGetDependencies(t *testing.T) {
 	tqs := initializeTestService()

model/adjuster/adjuster.go

@@ -5,57 +5,34 @@
 package adjuster
 
 import (
-	"errors"
-
 	"github.com/jaegertracing/jaeger/model"
 )
 
-// Adjuster applies certain modifications to a Trace object.
-// It returns adjusted Trace, which can be the same Trace updated in place.
-// If it detects a problem with the trace that prevents it from applying
-// adjustments, it must still return the original trace, and the error.
+// Adjuster is an interface for modifying a trace object in place.
 type Adjuster interface {
-	Adjust(trace *model.Trace) (*model.Trace, error)
+	Adjust(trace *model.Trace)
 }
 
 // Func wraps a function of appropriate signature and makes an Adjuster from it.
-type Func func(trace *model.Trace) (*model.Trace, error)
+type Func func(trace *model.Trace)
 
 // Adjust implements Adjuster interface.
-func (f Func) Adjust(trace *model.Trace) (*model.Trace, error) {
-	return f(trace)
+func (f Func) Adjust(trace *model.Trace) {
+	f(trace)
 }
 
 // Sequence creates an adjuster that combines a series of adjusters
-// applied in order. Errors from each step are accumulated and returned
-// in the end as a single wrapper error. Errors do not interrupt the
-// sequence of adapters.
+// applied in order.
 func Sequence(adjusters ...Adjuster) Adjuster {
 	return sequence{adjusters: adjusters}
 }
 
-// FailFastSequence is similar to Sequence() but returns immediately
-// if any adjuster returns an error.
-func FailFastSequence(adjusters ...Adjuster) Adjuster {
-	return sequence{adjusters: adjusters, failFast: true}
-}
-
 type sequence struct {
 	adjusters []Adjuster
-	failFast  bool
 }
 
-func (c sequence) Adjust(trace *model.Trace) (*model.Trace, error) {
-	var errs []error
+func (c sequence) Adjust(trace *model.Trace) {
 	for _, adjuster := range c.adjusters {
-		var err error
-		trace, err = adjuster.Adjust(trace)
-		if err != nil {
-			if c.failFast {
-				return trace, err
-			}
-			errs = append(errs, err)
-		}
+		adjuster.Adjust(trace)
 	}
-	return trace, errors.Join(errs...)
 }

model/adjuster/adjuster_test.go

@@ -5,54 +5,27 @@
 package adjuster_test
 
 import (
-	"errors"
-	"fmt"
 	"testing"
 
 	"github.com/stretchr/testify/assert"
-	"github.com/stretchr/testify/require"
 
 	"github.com/jaegertracing/jaeger/model"
 	"github.com/jaegertracing/jaeger/model/adjuster"
 )
 
 func TestSequences(t *testing.T) {
 	// mock adjuster that increments span ID
-	adj := adjuster.Func(func(trace *model.Trace) (*model.Trace, error) {
+	adj := adjuster.Func(func(trace *model.Trace) {
 		trace.Spans[0].SpanID++
-		return trace, nil
 	})
 
-	adjErr := errors.New("mock adjuster error")
-	failingAdj := adjuster.Func(func(trace *model.Trace) (*model.Trace, error) {
-		return trace, adjErr
-	})
+	seqAdjuster := adjuster.Sequence(adj, adj)
+
+	span := &model.Span{}
+	trace := model.Trace{Spans: []*model.Span{span}}
+
+	seqAdjuster.Adjust(&trace)
 
-	testCases := []struct {
-		adjuster   adjuster.Adjuster
-		err        string
-		lastSpanID int
-	}{
-		{
-			adjuster:   adjuster.Sequence(adj, failingAdj, adj, failingAdj),
-			err:        fmt.Sprintf("%s\n%s", adjErr, adjErr),
-			lastSpanID: 2,
-		},
-		{
-			adjuster:   adjuster.FailFastSequence(adj, failingAdj, adj, failingAdj),
-			err:        adjErr.Error(),
-			lastSpanID: 1,
-		},
-	}
-
-	for _, testCase := range testCases {
-		span := &model.Span{}
-		trace := model.Trace{Spans: []*model.Span{span}}
-
-		adjTrace, err := testCase.adjuster.Adjust(&trace)
-
-		assert.Equal(t, span, adjTrace.Spans[0], "same trace & span returned")
-		assert.EqualValues(t, testCase.lastSpanID, span.SpanID, "expect span ID to be incremented")
-		require.EqualError(t, err, testCase.err)
-	}
+	assert.Equal(t, span, trace.Spans[0], "same trace & span returned")
+	assert.EqualValues(t, 2, span.SpanID, "expect span ID to be incremented")
 }

model/adjuster/bad_span_references.go

@@ -12,12 +12,11 @@ import (
 
 // SpanReferences creates an adjuster that removes invalid span references, e.g. with traceID==0
 func SpanReferences() Adjuster {
-	return Func(func(trace *model.Trace) (*model.Trace, error) {
+	return Func(func(trace *model.Trace) {
 		adjuster := spanReferenceAdjuster{}
 		for _, span := range trace.Spans {
 			adjuster.adjust(span)
 		}
-		return trace, nil
 	})
 }
 

model/adjuster/bad_span_references_test.go

@@ -8,7 +8,6 @@ import (
 	"testing"
 
 	"github.com/stretchr/testify/assert"
-	"github.com/stretchr/testify/require"
 
 	"github.com/jaegertracing/jaeger/model"
 )
@@ -29,8 +28,7 @@ func TestSpanReferencesAdjuster(t *testing.T) {
 			},
 		},
 	}
-	trace, err := SpanReferences().Adjust(trace)
-	require.NoError(t, err)
+	SpanReferences().Adjust(trace)
 	assert.Empty(t, trace.Spans[0].References)
 	assert.Empty(t, trace.Spans[1].References)
 	assert.Len(t, trace.Spans[2].References, 2)

model/adjuster/clockskew.go

@@ -21,10 +21,9 @@ import (
 // The algorithm assumes that all spans have unique IDs, so the trace may need
 // to go through another adjuster first, such as ZipkinSpanIDUniquifier.
 //
-// This adjuster never returns any errors. Instead it records any issues
-// it encounters in Span.Warnings.
+// Any issues encountered by the adjuster are recorded in Span.Warnings.
 func ClockSkew(maxDelta time.Duration) Adjuster {
-	return Func(func(trace *model.Trace) (*model.Trace, error) {
+	return Func(func(trace *model.Trace) {
 		adjuster := &clockSkewAdjuster{
 			trace:    trace,
 			maxDelta: maxDelta,
@@ -35,7 +34,6 @@ func ClockSkew(maxDelta time.Duration) Adjuster {
 			skew := clockSkew{hostKey: n.hostKey}
 			adjuster.adjustNode(n, nil, skew)
 		}
-		return adjuster.trace, nil
 	})
 }
 

model/adjuster/clockskew_test.go

@@ -187,8 +187,8 @@ func TestClockSkewAdjuster(t *testing.T) {
 		testCase := tt // capture loop var
 		t.Run(testCase.description, func(t *testing.T) {
 			adjuster := ClockSkew(tt.maxAdjust)
-			trace, err := adjuster.Adjust(makeTrace(testCase.trace))
-			require.NoError(t, err)
+			trace := makeTrace(testCase.trace)
+			adjuster.Adjust(trace)
 			if testCase.err != "" {
 				var err string
 				for _, span := range trace.Spans {

model/adjuster/ip_tag.go

@@ -49,12 +49,11 @@ func IPTagAdjuster() Adjuster {
 		}
 	}
 
-	return Func(func(trace *model.Trace) (*model.Trace, error) {
+	return Func(func(trace *model.Trace) {
 		for _, span := range trace.Spans {
 			adjustTags(span.Tags)
 			adjustTags(span.Process.Tags)
 			model.KeyValues(span.Process.Tags).Sort()
 		}
-		return trace, nil
 	})
 }

model/adjuster/ip_tag_test.go

@@ -8,7 +8,6 @@ import (
 	"testing"
 
 	"github.com/stretchr/testify/assert"
-	"github.com/stretchr/testify/require"
 
 	"github.com/jaegertracing/jaeger/model"
 )
@@ -36,8 +35,7 @@ func TestIPTagAdjuster(t *testing.T) {
 			},
 		},
 	}
-	trace, err := IPTagAdjuster().Adjust(trace)
-	require.NoError(t, err)
+	IPTagAdjuster().Adjust(trace)
 
 	expectedSpanTags := model.KeyValues{
 		model.Int64("a", 42),