Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[chore] update jaeger dep and remove koanf 1.5 dep #28881

Merged

Conversation

codeboten
Copy link
Contributor

@codeboten codeboten commented Nov 3, 2023

Fixes #28647

After this is merged contributors can finally use go workspaces in this repo.

Fixes #26567

Copy link
Contributor

This PR was marked stale due to lack of activity. It will be closed in 14 days.

@github-actions github-actions bot added the Stale label Nov 18, 2023
@codeboten codeboten removed the Stale label Nov 20, 2023
@codeboten
Copy link
Contributor Author

not stale, waiting on PR

Copy link
Contributor

github-actions bot commented Dec 5, 2023

This PR was marked stale due to lack of activity. It will be closed in 14 days.

@github-actions github-actions bot added the Stale label Dec 5, 2023
@codeboten codeboten force-pushed the codeboten/update-jaeger-dep3 branch from d85f2a5 to b76ea36 Compare December 5, 2023 21:54
@codeboten
Copy link
Contributor Author

There's a change in jaeger v1.45.0 that causes the pkg/translator/jaeger test to fail:

--- FAIL: TestProtoToTraces (0.00s)
    --- FAIL: TestProtoToTraces/two-spans-with-follower (0.00s)
        jaegerproto_to_traces_test.go:313: 
            	Error Trace:	/home/runner/work/opentelemetry-collector-contrib/opentelemetry-collector-contrib/pkg/translator/jaeger/jaegerproto_to_traces_test.go:313
            	Error:      	Not equal: 
            	            	expected: ptrace.Traces{orig:(*v1.ExportTraceServiceRequest)(0xc000013470), state:(*internal.State)(0xc0001f2e2c)}
            	            	actual  : ptrace.Traces{orig:(*v1.ExportTraceServiceRequest)(0xc000013668), state:(*internal.State)(0xc0001f318c)}
            	            	
            	            	Diff:
            	            	--- Expected
            	            	+++ Actual
            	            	@@ -70,3 +70,3 @@
            	            	         ParentSpanId: (data.SpanID) (len=8) {
            	            	-         00000000  00 00 00 00 00 00 00 00                           |........|
            	            	+         00000000  af ae ad ac ab aa a9 a8                           |........|
            	            	         },
            	Test:       	TestProtoToTraces/two-spans-with-follower
    --- FAIL: TestProtoToTraces/a-spans-with-two-parent (0.00s)
        jaegerproto_to_traces_test.go:313: 
            	Error Trace:	/home/runner/work/opentelemetry-collector-contrib/opentelemetry-collector-contrib/pkg/translator/jaeger/jaegerproto_to_traces_test.go:313
            	Error:      	Not equal: 
            	            	expected: ptrace.Traces{orig:(*v1.ExportTraceServiceRequest)(0xc000013488), state:(*internal.State)(0xc0001f2e4c)}
            	            	actual  : ptrace.Traces{orig:(*v1.ExportTraceServiceRequest)(0xc0000138a8), state:(*internal.State)(0xc00022a84c)}
            	            	
            	            	Diff:
            	            	--- Expected
            	            	+++ Actual
            	            	@@ -70,3 +70,3 @@
            	            	         ParentSpanId: (data.SpanID) (len=8) {
            	            	-         00000000  00 00 00 00 00 00 00 00                           |........|
            	            	+         00000000  af ae ad ac ab aa a9 a8                           |........|
            	            	         },
            	Test:       	TestProtoToTraces/a-spans-with-two-parent
FAIL
	github.com/open-telemetry/opentelemetry-collector-contrib/pkg/translator/jaeger	coverage: 95.4% of statements
FAIL	github.com/open-telemetry/opentelemetry-collector-contrib/pkg/translator/jaeger	0.378s
FAIL

@frzifus @yurishkuro it looks like maybe this change: jaegertracing/jaeger@0ee275a, any thoughts on how this test should change to address the failure?

@yurishkuro
Copy link
Member

@codeboten yes, that's the change. It allows a lone follows-from reference to be treated as a parent. However, the OTLP span that the Jaeger span is compared against after translation does not define parent ID. The tests can be fixed with this change:

diff --git a/pkg/translator/jaeger/jaegerproto_to_traces_test.go b/pkg/translator/jaeger/jaegerproto_to_traces_test.go
index 3ea43570d8..a75be85c87 100644
--- a/pkg/translator/jaeger/jaegerproto_to_traces_test.go
+++ b/pkg/translator/jaeger/jaegerproto_to_traces_test.go
@@ -919,6 +919,7 @@ func generateTracesTwoSpansWithFollower() ptrace.Traces {
        span.SetName("operationC")
        span.SetSpanID([8]byte{0x1F, 0x1E, 0x1D, 0x1C, 0x1B, 0x1A, 0x19, 0x18})
        span.SetTraceID(spans.At(0).TraceID())
+       span.SetParentSpanID(spans.At(0).SpanID())
        span.SetStartTimestamp(spans.At(0).EndTimestamp())
        span.SetEndTimestamp(spans.At(0).EndTimestamp() + 1000000)
        span.SetKind(ptrace.SpanKindConsumer)

However, it would cause breaks in some other conversion tests, for the same reason - one side does not include the reference and/or ParentID.

@github-actions github-actions bot removed the Stale label Dec 6, 2023
@codeboten
Copy link
Contributor Author

@yurishkuro after making the suggested change, the following error occurs, will fixing this require a change in ThriftToTraces to support this as well?

                Error Trace:	opentelemetry-collector-contrib/pkg/translator/jaeger/jaegerthrift_to_traces_test.go:135
        	Error:      	Not equal: 
        	            	expected: ptrace.Traces{orig:(*v1.ExportTraceServiceRequest)(0x1400000f0c8), state:(*internal.State)(0x14000011d68)}
        	            	actual  : ptrace.Traces{orig:(*v1.ExportTraceServiceRequest)(0x1400000f290), state:(*internal.State)(0x14000011e98)}
        	            	
        	            	Diff:
        	            	--- Expected
        	            	+++ Actual
        	            	@@ -70,3 +70,3 @@
        	            	         ParentSpanId: (data.SpanID) (len=8) {
        	            	-         00000000  af ae ad ac ab aa a9 a8                           |........|
        	            	+         00000000  00 00 00 00 00 00 00 00                           |........|
        	            	         },
        	Test:       	TestThriftBatchToInternalTraces/a-spans-with-two-parent

Alex Boten added 2 commits December 7, 2023 10:46
Fixes open-telemetry#28647

After this is merged contributors can finally use go workspaces in this repo.

Signed-off-by: Alex Boten <aboten@lightstep.com>
Signed-off-by: Alex Boten <aboten@lightstep.com>
@codeboten codeboten force-pushed the codeboten/update-jaeger-dep3 branch from 1cd5d86 to e18eb5d Compare December 7, 2023 18:46
@yurishkuro
Copy link
Member

Yes, I believe translation code is incorrect. For instance, in makeJaegerProtoReferences, the parentID is unconditionally translated into CHILD_OF reference, but it should be conditional on not having a link with the same parentID, i.e.

  1. process all links and convert them to references
  2. if neither of them mentioned parentID, then prepend a CHILD_OF reference with that ID

Probably a similar issue in Thrift translation.

Signed-off-by: Yuri Shkuro <github@ysh.us>
@codeboten codeboten marked this pull request as ready for review December 8, 2023 00:43
@codeboten codeboten merged commit 7d62ad9 into open-telemetry:main Dec 8, 2023
85 checks passed
@codeboten codeboten deleted the codeboten/update-jaeger-dep3 branch December 8, 2023 00:46
@github-actions github-actions bot added this to the next release milestone Dec 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants