From 7537fe915563b4432a39597a6ac5cdbf4b23a3fb Mon Sep 17 00:00:00 2001 From: Mahad Zaryab <43658574+mahadzaryab1@users.noreply.github.com> Date: Wed, 18 Dec 2024 16:26:31 -0500 Subject: [PATCH] [v2][adjuster] Remove error return from adjuster interface (#6383) ## Which problem is this PR solving? - Towards #6344 ## Description of the changes - This PR removes the `error` return value from the v2 adjuster interface as none of the adjusters return any errors and errors instead recorded on the spans as warnings. ## 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/querysvc/adjuster/adjuster.go | 29 +++-------- .../app/querysvc/adjuster/adjuster_test.go | 51 ++++--------------- .../app/querysvc/adjuster/ipattribute.go | 3 +- .../app/querysvc/adjuster/ipattribute_test.go | 3 +- .../querysvc/adjuster/resourceattributes.go | 3 +- .../adjuster/resourceattributes_test.go | 8 +-- .../app/querysvc/adjuster/spaniduniquifier.go | 11 ++-- .../adjuster/spaniduniquifier_test.go | 6 +-- cmd/query/app/querysvc/adjuster/spanlinks.go | 3 +- .../app/querysvc/adjuster/spanlinks_test.go | 4 +- 10 files changed, 32 insertions(+), 89 deletions(-) diff --git a/cmd/query/app/querysvc/adjuster/adjuster.go b/cmd/query/app/querysvc/adjuster/adjuster.go index d03fe93103b..fbe9689ca96 100644 --- a/cmd/query/app/querysvc/adjuster/adjuster.go +++ b/cmd/query/app/querysvc/adjuster/adjuster.go @@ -4,8 +4,6 @@ package adjuster import ( - "errors" - "go.opentelemetry.io/collector/pdata/ptrace" ) @@ -14,15 +12,15 @@ import ( // The caller must ensure that all spans in the ptrace.Traces argument // belong to the same trace and represent the complete trace. type Adjuster interface { - Adjust(ptrace.Traces) error + Adjust(ptrace.Traces) } // Func is a type alias that wraps a function and makes an Adjuster from it. -type Func func(traces ptrace.Traces) error +type Func func(traces ptrace.Traces) // Adjust implements Adjuster interface for the Func alias. -func (f Func) Adjust(traces ptrace.Traces) error { - return f(traces) +func (f Func) Adjust(traces ptrace.Traces) { + f(traces) } // Sequence creates an adjuster that combines a series of adjusters @@ -33,27 +31,12 @@ 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(traces ptrace.Traces) error { - var errs []error +func (c sequence) Adjust(traces ptrace.Traces) { for _, adjuster := range c.adjusters { - err := adjuster.Adjust(traces) - if err != nil { - if c.failFast { - return err - } - errs = append(errs, err) - } + adjuster.Adjust(traces) } - return errors.Join(errs...) } diff --git a/cmd/query/app/querysvc/adjuster/adjuster_test.go b/cmd/query/app/querysvc/adjuster/adjuster_test.go index fbc5804ed9e..42cb3007261 100644 --- a/cmd/query/app/querysvc/adjuster/adjuster_test.go +++ b/cmd/query/app/querysvc/adjuster/adjuster_test.go @@ -4,12 +4,9 @@ package adjuster_test import ( - "fmt" "testing" "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/require" - "go.opentelemetry.io/collector/pdata/pcommon" "go.opentelemetry.io/collector/pdata/ptrace" "github.com/jaegertracing/jaeger/cmd/query/app/querysvc/adjuster" @@ -17,53 +14,23 @@ import ( type mockAdjuster struct{} -func (mockAdjuster) Adjust(traces ptrace.Traces) error { +func (mockAdjuster) Adjust(traces ptrace.Traces) { span := traces.ResourceSpans().At(0).ScopeSpans().At(0).Spans().At(0) spanId := span.SpanID() spanId[7]++ span.SetSpanID(spanId) - return nil -} - -type mockAdjusterError struct{} - -func (mockAdjusterError) Adjust(ptrace.Traces) error { - return assert.AnError } func TestSequences(t *testing.T) { - tests := []struct { - name string - adjuster adjuster.Adjuster - err string - lastSpanID pcommon.SpanID - }{ - { - name: "normal sequence", - adjuster: adjuster.Sequence(mockAdjuster{}, mockAdjusterError{}, mockAdjuster{}, mockAdjusterError{}), - err: fmt.Sprintf("%s\n%s", assert.AnError, assert.AnError), - lastSpanID: [8]byte{0, 0, 0, 0, 0, 0, 0, 2}, - }, - { - name: "fail fast sequence", - adjuster: adjuster.FailFastSequence(mockAdjuster{}, mockAdjusterError{}, mockAdjuster{}, mockAdjusterError{}), - err: assert.AnError.Error(), - lastSpanID: [8]byte{0, 0, 0, 0, 0, 0, 0, 1}, - }, - } + trace := ptrace.NewTraces() + span := trace.ResourceSpans().AppendEmpty().ScopeSpans().AppendEmpty().Spans().AppendEmpty() + span.SetSpanID([8]byte{0, 0, 0, 0, 0, 0, 0, 0}) - for _, test := range tests { - t.Run(test.name, func(t *testing.T) { - trace := ptrace.NewTraces() - span := trace.ResourceSpans().AppendEmpty().ScopeSpans().AppendEmpty().Spans().AppendEmpty() - span.SetSpanID([8]byte{0, 0, 0, 0, 0, 0, 0, 0}) + a := adjuster.Sequence(mockAdjuster{}, mockAdjuster{}) - err := test.adjuster.Adjust(trace) - require.EqualError(t, err, test.err) + a.Adjust(trace) - adjTraceSpan := trace.ResourceSpans().At(0).ScopeSpans().At(0).Spans().At(0) - assert.Equal(t, span, adjTraceSpan) - assert.EqualValues(t, test.lastSpanID, span.SpanID()) - }) - } + adjTraceSpan := trace.ResourceSpans().At(0).ScopeSpans().At(0).Spans().At(0) + assert.Equal(t, span, adjTraceSpan) + assert.EqualValues(t, [8]byte{0, 0, 0, 0, 0, 0, 0, 2}, span.SpanID()) } diff --git a/cmd/query/app/querysvc/adjuster/ipattribute.go b/cmd/query/app/querysvc/adjuster/ipattribute.go index cd3b9bb1340..a00f93b3254 100644 --- a/cmd/query/app/querysvc/adjuster/ipattribute.go +++ b/cmd/query/app/querysvc/adjuster/ipattribute.go @@ -28,7 +28,7 @@ func IPAttribute() IPAttributeAdjuster { type IPAttributeAdjuster struct{} -func (ia IPAttributeAdjuster) Adjust(traces ptrace.Traces) error { +func (ia IPAttributeAdjuster) Adjust(traces ptrace.Traces) { resourceSpans := traces.ResourceSpans() for i := 0; i < resourceSpans.Len(); i++ { rs := resourceSpans.At(i) @@ -43,7 +43,6 @@ func (ia IPAttributeAdjuster) Adjust(traces ptrace.Traces) error { } } } - return nil } func (IPAttributeAdjuster) adjustAttributes(attributes pcommon.Map) { diff --git a/cmd/query/app/querysvc/adjuster/ipattribute_test.go b/cmd/query/app/querysvc/adjuster/ipattribute_test.go index e55a9dd8bdc..d97f1bf35ed 100644 --- a/cmd/query/app/querysvc/adjuster/ipattribute_test.go +++ b/cmd/query/app/querysvc/adjuster/ipattribute_test.go @@ -58,8 +58,7 @@ func TestIPAttributeAdjuster(t *testing.T) { } } - err := IPAttribute().Adjust(traces) - require.NoError(t, err) + IPAttribute().Adjust(traces) resourceSpan := traces.ResourceSpans().At(0) assert.Equal(t, 3, resourceSpan.Resource().Attributes().Len()) diff --git a/cmd/query/app/querysvc/adjuster/resourceattributes.go b/cmd/query/app/querysvc/adjuster/resourceattributes.go index e995088d101..6bedd4ff4e9 100644 --- a/cmd/query/app/querysvc/adjuster/resourceattributes.go +++ b/cmd/query/app/querysvc/adjuster/resourceattributes.go @@ -31,7 +31,7 @@ func ResourceAttributes() ResourceAttributesAdjuster { type ResourceAttributesAdjuster struct{} -func (o ResourceAttributesAdjuster) Adjust(traces ptrace.Traces) error { +func (o ResourceAttributesAdjuster) Adjust(traces ptrace.Traces) { resourceSpans := traces.ResourceSpans() for i := 0; i < resourceSpans.Len(); i++ { rs := resourceSpans.At(i) @@ -46,7 +46,6 @@ func (o ResourceAttributesAdjuster) Adjust(traces ptrace.Traces) error { } } } - return nil } func (ResourceAttributesAdjuster) moveAttributes(span ptrace.Span, resource pcommon.Resource) { diff --git a/cmd/query/app/querysvc/adjuster/resourceattributes_test.go b/cmd/query/app/querysvc/adjuster/resourceattributes_test.go index 5dc5c438133..591820db4a2 100644 --- a/cmd/query/app/querysvc/adjuster/resourceattributes_test.go +++ b/cmd/query/app/querysvc/adjuster/resourceattributes_test.go @@ -26,7 +26,7 @@ func TestResourceAttributesAdjuster_SpanWithLibraryAttributes(t *testing.T) { span.Attributes().PutStr("another_key", "another_value") adjuster := ResourceAttributes() - require.NoError(t, adjuster.Adjust(traces)) + adjuster.Adjust(traces) resultSpanAttributes := traces.ResourceSpans().At(0).ScopeSpans().At(0).Spans().At(0).Attributes() require.Equal(t, 2, resultSpanAttributes.Len()) @@ -68,7 +68,7 @@ func TestResourceAttributesAdjuster_SpanWithoutLibraryAttributes(t *testing.T) { span.Attributes().PutStr("random_key", "random_value") adjuster := ResourceAttributes() - require.NoError(t, adjuster.Adjust(traces)) + adjuster.Adjust(traces) resultSpanAttributes := traces.ResourceSpans().At(0).ScopeSpans().At(0).Spans().At(0).Attributes() require.Equal(t, 1, resultSpanAttributes.Len()) @@ -86,7 +86,7 @@ func TestResourceAttributesAdjuster_SpanWithConflictingLibraryAttributes(t *test span.Attributes().PutStr(string(otelsemconv.TelemetrySDKLanguageKey), "Java") adjuster := ResourceAttributes() - require.NoError(t, adjuster.Adjust(traces)) + adjuster.Adjust(traces) resultSpan := traces.ResourceSpans().At(0).ScopeSpans().At(0).Spans().At(0) resultSpanAttributes := resultSpan.Attributes() @@ -120,7 +120,7 @@ func TestResourceAttributesAdjuster_SpanWithNonConflictingLibraryAttributes(t *t span.Attributes().PutStr(string(otelsemconv.TelemetrySDKLanguageKey), "Go") adjuster := ResourceAttributes() - require.NoError(t, adjuster.Adjust(traces)) + adjuster.Adjust(traces) resultSpanAttributes := traces.ResourceSpans().At(0).ScopeSpans().At(0).Spans().At(0).Attributes() require.Equal(t, 1, resultSpanAttributes.Len()) diff --git a/cmd/query/app/querysvc/adjuster/spaniduniquifier.go b/cmd/query/app/querysvc/adjuster/spaniduniquifier.go index 1fa05ca5f95..b21ba7eeb6d 100644 --- a/cmd/query/app/querysvc/adjuster/spaniduniquifier.go +++ b/cmd/query/app/querysvc/adjuster/spaniduniquifier.go @@ -21,15 +21,15 @@ var errTooManySpans = errors.New("cannot assign unique span ID, too many spans i // Zipkin-style clients that reuse the same span ID for both client and server // side of an RPC call. Jaeger UI expects all spans to have unique IDs. // -// This adjuster never returns any errors. Instead it records any issues -// it encounters in Span.Warnings. +// Any issues encountered during adjustment are recorded as warnings in the +// span. func SpanIDUniquifier() Adjuster { - return Func(func(traces ptrace.Traces) error { + return Func(func(traces ptrace.Traces) { adjuster := spanIDDeduper{ spansByID: make(map[pcommon.SpanID][]ptrace.Span), maxUsedID: pcommon.NewSpanIDEmpty(), } - return adjuster.adjust(traces) + adjuster.adjust(traces) }) } @@ -38,10 +38,9 @@ type spanIDDeduper struct { maxUsedID pcommon.SpanID } -func (d *spanIDDeduper) adjust(traces ptrace.Traces) error { +func (d *spanIDDeduper) adjust(traces ptrace.Traces) { d.groupSpansByID(traces) d.uniquifyServerSpanIDs(traces) - return nil } // groupSpansByID groups spans with the same ID returning a map id -> []Span diff --git a/cmd/query/app/querysvc/adjuster/spaniduniquifier_test.go b/cmd/query/app/querysvc/adjuster/spaniduniquifier_test.go index 564c8c85baa..5ad6868f443 100644 --- a/cmd/query/app/querysvc/adjuster/spaniduniquifier_test.go +++ b/cmd/query/app/querysvc/adjuster/spaniduniquifier_test.go @@ -47,7 +47,7 @@ func makeTraces() ptrace.Traces { func TestSpanIDUniquifierTriggered(t *testing.T) { traces := makeTraces() deduper := SpanIDUniquifier() - require.NoError(t, deduper.Adjust(traces)) + deduper.Adjust(traces) spans := traces.ResourceSpans().At(0).ScopeSpans().At(0).Spans() @@ -74,7 +74,7 @@ func TestSpanIDUniquifierNotTriggered(t *testing.T) { newSpans.CopyTo(spans) deduper := SpanIDUniquifier() - require.NoError(t, deduper.Adjust(traces)) + deduper.Adjust(traces) gotSpans := traces.ResourceSpans().At(0).ScopeSpans().At(0).Spans() @@ -96,7 +96,7 @@ func TestSpanIDUniquifierError(t *testing.T) { // instead of 0 start at the last possible value to cause an error maxUsedID: maxID, } - require.NoError(t, deduper.adjust(traces)) + deduper.adjust(traces) span := traces.ResourceSpans().At(0).ScopeSpans().At(0).Spans().At(1) warnings := jptrace.GetWarnings(span) diff --git a/cmd/query/app/querysvc/adjuster/spanlinks.go b/cmd/query/app/querysvc/adjuster/spanlinks.go index cac09ec592d..60730fa5ecf 100644 --- a/cmd/query/app/querysvc/adjuster/spanlinks.go +++ b/cmd/query/app/querysvc/adjuster/spanlinks.go @@ -22,7 +22,7 @@ func SpanLinks() LinksAdjuster { type LinksAdjuster struct{} -func (la LinksAdjuster) Adjust(traces ptrace.Traces) error { +func (la LinksAdjuster) Adjust(traces ptrace.Traces) { resourceSpans := traces.ResourceSpans() for i := 0; i < resourceSpans.Len(); i++ { rs := resourceSpans.At(i) @@ -36,7 +36,6 @@ func (la LinksAdjuster) Adjust(traces ptrace.Traces) error { } } } - return nil } // adjust removes invalid links from a span. diff --git a/cmd/query/app/querysvc/adjuster/spanlinks_test.go b/cmd/query/app/querysvc/adjuster/spanlinks_test.go index 7dfaf0ac6a6..33a8a5a78a6 100644 --- a/cmd/query/app/querysvc/adjuster/spanlinks_test.go +++ b/cmd/query/app/querysvc/adjuster/spanlinks_test.go @@ -7,7 +7,6 @@ import ( "testing" "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/require" "go.opentelemetry.io/collector/pdata/pcommon" "go.opentelemetry.io/collector/pdata/ptrace" @@ -32,9 +31,8 @@ func TestLinksAdjuster(t *testing.T) { spanC.Links().AppendEmpty().SetTraceID(pcommon.TraceID([]byte{0, 0, 0, 0, 0, 0, 0, 1, 0, 0, 0, 0, 0, 0, 0, 0})) spanC.Links().AppendEmpty().SetTraceID(pcommon.TraceID([]byte{0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0})) - err := SpanLinks().Adjust(traces) + SpanLinks().Adjust(traces) spans := traces.ResourceSpans().At(0).ScopeSpans().At(0).Spans() - require.NoError(t, err) gotSpansA := spans.At(0) assert.Equal(t, 0, gotSpansA.Links().Len())