From e6caacb25843c3c974cc1ce46a11c4a0d900819a Mon Sep 17 00:00:00 2001 From: Mahad Zaryab <43658574+mahadzaryab1@users.noreply.github.com> Date: Fri, 27 Dec 2024 14:31:04 -0500 Subject: [PATCH] [v1][adjuster] Change v1 adjuster interface to not return error and modify 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 --- cmd/query/app/http_handler.go | 23 ++-------- cmd/query/app/http_handler_test.go | 40 ----------------- cmd/query/app/querysvc/query_service.go | 4 +- cmd/query/app/querysvc/query_service_test.go | 20 --------- model/adjuster/adjuster.go | 39 ++++------------ model/adjuster/adjuster_test.go | 45 ++++--------------- model/adjuster/bad_span_references.go | 3 +- model/adjuster/bad_span_references_test.go | 4 +- model/adjuster/clockskew.go | 6 +-- model/adjuster/clockskew_test.go | 4 +- model/adjuster/ip_tag.go | 3 +- model/adjuster/ip_tag_test.go | 4 +- model/adjuster/otel_tag.go | 3 +- model/adjuster/otel_tag_test.go | 4 +- model/adjuster/parent_reference.go | 4 +- model/adjuster/parent_reference_test.go | 4 +- model/adjuster/sort_tags_and_log_fields.go | 3 +- .../adjuster/sort_tags_and_log_fields_test.go | 28 ++++++------ model/adjuster/span_hash_deduper.go | 3 +- model/adjuster/span_hash_deduper_test.go | 13 ++---- model/adjuster/zipkin_span_id_uniquify.go | 3 +- .../adjuster/zipkin_span_id_uniquify_test.go | 7 +-- plugin/storage/memory/memory.go | 5 +-- 23 files changed, 60 insertions(+), 212 deletions(-) diff --git a/cmd/query/app/http_handler.go b/cmd/query/app/http_handler.go index 02ed05c059f..06c97c94dbf 100644 --- a/cmd/query/app/http_handler.go +++ b/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 { diff --git a/cmd/query/app/http_handler_test.go b/cmd/query/app/http_handler_test.go index 479466d0da0..015c8e0fb7a 100644 --- a/cmd/query/app/http_handler_test.go +++ b/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")). diff --git a/cmd/query/app/querysvc/query_service.go b/cmd/query/app/querysvc/query_service.go index 724a2536583..3f40b2d44cb 100644 --- a/cmd/query/app/querysvc/query_service.go +++ b/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 diff --git a/cmd/query/app/querysvc/query_service_test.go b/cmd/query/app/querysvc/query_service_test.go index 2b1176cf96f..1d9217ade29 100644 --- a/cmd/query/app/querysvc/query_service_test.go +++ b/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() diff --git a/model/adjuster/adjuster.go b/model/adjuster/adjuster.go index ac808866326..5f97741398d 100644 --- a/model/adjuster/adjuster.go +++ b/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...) } diff --git a/model/adjuster/adjuster_test.go b/model/adjuster/adjuster_test.go index 0a0960920b5..ddd7b26c744 100644 --- a/model/adjuster/adjuster_test.go +++ b/model/adjuster/adjuster_test.go @@ -5,12 +5,9 @@ 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" @@ -18,41 +15,17 @@ import ( 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") } diff --git a/model/adjuster/bad_span_references.go b/model/adjuster/bad_span_references.go index 3720be424b3..8f30ec3938b 100644 --- a/model/adjuster/bad_span_references.go +++ b/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 }) } diff --git a/model/adjuster/bad_span_references_test.go b/model/adjuster/bad_span_references_test.go index 72952c30f40..4c671704e98 100644 --- a/model/adjuster/bad_span_references_test.go +++ b/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) diff --git a/model/adjuster/clockskew.go b/model/adjuster/clockskew.go index 30f922307dc..27cfc0851d5 100644 --- a/model/adjuster/clockskew.go +++ b/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 }) } diff --git a/model/adjuster/clockskew_test.go b/model/adjuster/clockskew_test.go index 5d5023e4d49..1e0d2d8a62a 100644 --- a/model/adjuster/clockskew_test.go +++ b/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 { diff --git a/model/adjuster/ip_tag.go b/model/adjuster/ip_tag.go index b3105fe6b5d..2f11e304022 100644 --- a/model/adjuster/ip_tag.go +++ b/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 }) } diff --git a/model/adjuster/ip_tag_test.go b/model/adjuster/ip_tag_test.go index 716efb020b5..ae8ec352d38 100644 --- a/model/adjuster/ip_tag_test.go +++ b/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), diff --git a/model/adjuster/otel_tag.go b/model/adjuster/otel_tag.go index e579856726d..7811e8ebb19 100644 --- a/model/adjuster/otel_tag.go +++ b/model/adjuster/otel_tag.go @@ -32,11 +32,10 @@ func OTelTagAdjuster() Adjuster { span.Tags = span.Tags[:newI] } - return Func(func(trace *model.Trace) (*model.Trace, error) { + return Func(func(trace *model.Trace) { for _, span := range trace.Spans { adjustSpanTags(span) model.KeyValues(span.Process.Tags).Sort() } - return trace, nil }) } diff --git a/model/adjuster/otel_tag_test.go b/model/adjuster/otel_tag_test.go index 7e0b802d56c..c87708a43ce 100644 --- a/model/adjuster/otel_tag_test.go +++ b/model/adjuster/otel_tag_test.go @@ -7,7 +7,6 @@ import ( "testing" "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/require" "github.com/jaegertracing/jaeger/model" "github.com/jaegertracing/jaeger/pkg/otelsemconv" @@ -79,8 +78,7 @@ func TestOTelTagAdjuster(t *testing.T) { trace := &model.Trace{ Spans: []*model.Span{testCase.span}, } - trace, err := OTelTagAdjuster().Adjust(trace) - require.NoError(t, err) + OTelTagAdjuster().Adjust(trace) assert.Equal(t, testCase.expected.Tags, trace.Spans[0].Tags) assert.Equal(t, testCase.expected.Process.Tags, trace.Spans[0].Process.Tags) diff --git a/model/adjuster/parent_reference.go b/model/adjuster/parent_reference.go index 00e763d9b2b..253243ad53a 100644 --- a/model/adjuster/parent_reference.go +++ b/model/adjuster/parent_reference.go @@ -11,7 +11,7 @@ import ( // This is necessary to match jaeger-ui expectations: // * https://github.com/jaegertracing/jaeger-ui/issues/966 func ParentReference() Adjuster { - return Func(func(trace *model.Trace) (*model.Trace, error) { + return Func(func(trace *model.Trace) { for _, span := range trace.Spans { firstChildOfRef := -1 firstOtherRef := -1 @@ -33,7 +33,5 @@ func ParentReference() Adjuster { span.References[swap], span.References[0] = span.References[0], span.References[swap] } } - - return trace, nil }) } diff --git a/model/adjuster/parent_reference_test.go b/model/adjuster/parent_reference_test.go index fc9ba39b50f..58447dc5bf9 100644 --- a/model/adjuster/parent_reference_test.go +++ b/model/adjuster/parent_reference_test.go @@ -7,7 +7,6 @@ import ( "testing" "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/require" "github.com/jaegertracing/jaeger/model" ) @@ -96,8 +95,7 @@ func TestParentReference(t *testing.T) { }, }, } - trace, err := ParentReference().Adjust(trace) - require.NoError(t, err) + ParentReference().Adjust(trace) assert.Equal(t, testCase.expected, trace.Spans[0].References) }) } diff --git a/model/adjuster/sort_tags_and_log_fields.go b/model/adjuster/sort_tags_and_log_fields.go index 0d6028a2d0c..88891e507fb 100644 --- a/model/adjuster/sort_tags_and_log_fields.go +++ b/model/adjuster/sort_tags_and_log_fields.go @@ -20,7 +20,7 @@ import ( // place in the list. This adjuster needs some sort of config describing predefined // field names/types and their relative order. func SortTagsAndLogFields() Adjuster { - return Func(func(trace *model.Trace) (*model.Trace, error) { + return Func(func(trace *model.Trace) { for _, span := range trace.Spans { model.KeyValues(span.Tags).Sort() if span.Process != nil { @@ -45,6 +45,5 @@ func SortTagsAndLogFields() Adjuster { } } } - return trace, nil }) } diff --git a/model/adjuster/sort_tags_and_log_fields_test.go b/model/adjuster/sort_tags_and_log_fields_test.go index afd1983cac0..e87d3748267 100644 --- a/model/adjuster/sort_tags_and_log_fields_test.go +++ b/model/adjuster/sort_tags_and_log_fields_test.go @@ -80,8 +80,7 @@ func TestSortTagsAndLogFieldsDoesSortFields(t *testing.T) { }, }, } - trace, err := SortTagsAndLogFields().Adjust(trace) - require.NoError(t, err) + SortTagsAndLogFields().Adjust(trace) assert.Equal(t, testCase.expected, model.KeyValues(trace.Spans[0].Logs[0].Fields)) } } @@ -109,11 +108,10 @@ func TestSortTagsAndLogFieldsDoesSortTags(t *testing.T) { }, }, } - sorted, err := SortTagsAndLogFields().Adjust(trace) - require.NoError(t, err) - assert.ElementsMatch(t, tags, sorted.Spans[0].Tags) - adjustedKeys := make([]string, len(sorted.Spans[0].Tags)) - for i, kv := range sorted.Spans[0].Tags { + SortTagsAndLogFields().Adjust(trace) + assert.ElementsMatch(t, tags, trace.Spans[0].Tags) + adjustedKeys := make([]string, len(trace.Spans[0].Tags)) + for i, kv := range trace.Spans[0].Tags { adjustedKeys[i] = kv.Key } assert.IsIncreasing(t, adjustedKeys) @@ -135,11 +133,9 @@ func TestSortTagsAndLogFieldsDoesSortProcessTags(t *testing.T) { }, }, } - sorted, err := SortTagsAndLogFields().Adjust(trace) - require.NoError(t, err) - assert.ElementsMatch(t, trace.Spans[0].Process.Tags, sorted.Spans[0].Process.Tags) - adjustedKeys := make([]string, len(sorted.Spans[0].Process.Tags)) - for i, kv := range sorted.Spans[0].Process.Tags { + SortTagsAndLogFields().Adjust(trace) + adjustedKeys := make([]string, len(trace.Spans[0].Process.Tags)) + for i, kv := range trace.Spans[0].Process.Tags { adjustedKeys[i] = kv.Key } assert.IsIncreasing(t, adjustedKeys) @@ -151,6 +147,10 @@ func TestSortTagsAndLogFieldsHandlesNilProcessTags(t *testing.T) { {}, }, } - _, err := SortTagsAndLogFields().Adjust(trace) - require.NoError(t, err) + SortTagsAndLogFields().Adjust(trace) + require.Equal(t, &model.Trace{ + Spans: []*model.Span{ + {}, + }, + }, trace) } diff --git a/model/adjuster/span_hash_deduper.go b/model/adjuster/span_hash_deduper.go index 5feb79e1769..e0d7a8df6db 100644 --- a/model/adjuster/span_hash_deduper.go +++ b/model/adjuster/span_hash_deduper.go @@ -11,10 +11,9 @@ import ( // This is useful for when spans are duplicated in archival storage, as happens with // ElasticSearch archival. func DedupeBySpanHash() Adjuster { - return Func(func(trace *model.Trace) (*model.Trace, error) { + return Func(func(trace *model.Trace) { deduper := &spanHashDeduper{trace: trace} deduper.groupSpansByHash() - return deduper.trace, nil }) } diff --git a/model/adjuster/span_hash_deduper_test.go b/model/adjuster/span_hash_deduper_test.go index d8bffd5bf82..3660e9eb253 100644 --- a/model/adjuster/span_hash_deduper_test.go +++ b/model/adjuster/span_hash_deduper_test.go @@ -7,7 +7,6 @@ import ( "testing" "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/require" "github.com/jaegertracing/jaeger/model" ) @@ -74,8 +73,7 @@ func getSpanIDs(spans []*model.Span) []int { func TestDedupeBySpanHashTriggers(t *testing.T) { trc := newDuplicatedSpansTrace() deduper := DedupeBySpanHash() - trc, err := deduper.Adjust(trc) - require.NoError(t, err) + deduper.Adjust(trc) assert.Len(t, trc.Spans, 2, "should dedupe spans") @@ -86,8 +84,7 @@ func TestDedupeBySpanHashTriggers(t *testing.T) { func TestDedupeBySpanHashNotTriggered(t *testing.T) { trc := newUniqueSpansTrace() deduper := DedupeBySpanHash() - trc, err := deduper.Adjust(trc) - require.NoError(t, err) + deduper.Adjust(trc) assert.Len(t, trc.Spans, 2, "should not dedupe spans") @@ -99,8 +96,7 @@ func TestDedupeBySpanHashNotTriggered(t *testing.T) { func TestDedupeBySpanHashEmpty(t *testing.T) { trc := &model.Trace{} deduper := DedupeBySpanHash() - trc, err := deduper.Adjust(trc) - require.NoError(t, err) + deduper.Adjust(trc) assert.Empty(t, trc.Spans, "should be empty") } @@ -117,8 +113,7 @@ func TestDedupeBySpanHashManyManySpans(t *testing.T) { } trc := &model.Trace{Spans: spans} deduper := DedupeBySpanHash() - trc, err := deduper.Adjust(trc) - require.NoError(t, err) + deduper.Adjust(trc) assert.Len(t, trc.Spans, distinctSpanIDs, "should dedupe spans") diff --git a/model/adjuster/zipkin_span_id_uniquify.go b/model/adjuster/zipkin_span_id_uniquify.go index 5f8a86a32cc..3de8f20d77f 100644 --- a/model/adjuster/zipkin_span_id_uniquify.go +++ b/model/adjuster/zipkin_span_id_uniquify.go @@ -20,11 +20,10 @@ import ( // This adjuster never returns any errors. Instead it records any issues // it encounters in Span.Warnings. func ZipkinSpanIDUniquifier() Adjuster { - return Func(func(trace *model.Trace) (*model.Trace, error) { + return Func(func(trace *model.Trace) { deduper := &spanIDDeduper{trace: trace} deduper.groupSpansByID() deduper.uniquifyServerSpanIDs() - return deduper.trace, nil }) } diff --git a/model/adjuster/zipkin_span_id_uniquify_test.go b/model/adjuster/zipkin_span_id_uniquify_test.go index c0b92799c21..8b65cad68aa 100644 --- a/model/adjuster/zipkin_span_id_uniquify_test.go +++ b/model/adjuster/zipkin_span_id_uniquify_test.go @@ -8,7 +8,6 @@ import ( "testing" "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/require" "github.com/jaegertracing/jaeger/model" ) @@ -53,8 +52,7 @@ func newZipkinTrace() *model.Trace { func TestZipkinSpanIDUniquifierTriggered(t *testing.T) { trc := newZipkinTrace() deduper := ZipkinSpanIDUniquifier() - trc, err := deduper.Adjust(trc) - require.NoError(t, err) + deduper.Adjust(trc) clientSpan := trc.Spans[0] assert.Equal(t, clientSpanID, clientSpan.SpanID, "client span ID should not change") @@ -73,8 +71,7 @@ func TestZipkinSpanIDUniquifierNotTriggered(t *testing.T) { trc.Spans = trc.Spans[1:] // remove client span deduper := ZipkinSpanIDUniquifier() - trc, err := deduper.Adjust(trc) - require.NoError(t, err) + deduper.Adjust(trc) serverSpanID := clientSpanID // for better readability serverSpan := trc.Spans[0] diff --git a/plugin/storage/memory/memory.go b/plugin/storage/memory/memory.go index 23ec0bd2d39..02dff26a584 100644 --- a/plugin/storage/memory/memory.go +++ b/plugin/storage/memory/memory.go @@ -89,9 +89,8 @@ func (st *Store) GetDependencies(ctx context.Context, endTs time.Time, lookback defer m.Unlock() deps := map[string]*model.DependencyLink{} startTs := endTs.Add(-1 * lookback) - for _, orig := range m.traces { - // ZipkinSpanIDUniquifier never returns an err - trace, _ := m.deduper.Adjust(orig) + for _, trace := range m.traces { + m.deduper.Adjust(trace) if traceIsBetweenStartAndEnd(startTs, endTs, trace) { for _, s := range trace.Spans { parentSpan := findSpan(trace, s.ParentSpanID())