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

Replace usage of OpenTracing constants with OpenTelemetry in the domain model #4614

Merged

Conversation

afzal442
Copy link
Contributor

@afzal442 afzal442 commented Aug 1, 2023

Which problem is this PR solving?

Short description of the changes

  • Replace OT trace with otel trace spans type i.e. ext.SpanKind.. with trace.SpanKind..

@afzal442 afzal442 requested a review from a team as a code owner August 1, 2023 18:33
model/span.go Outdated Show resolved Hide resolved
model/span.go Outdated Show resolved Hide resolved
Copy link
Contributor

@albertteoh albertteoh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks pretty good to me, just a couple of nits.

model/span.go Outdated Show resolved Hide resolved
model/span_test.go Outdated Show resolved Hide resolved
@afzalbin64
Copy link
Contributor

afzalbin64 commented Aug 2, 2023

but the concern is why the unit test is still failing. sorry!

@albertteoh
Copy link
Contributor

albertteoh commented Aug 2, 2023

but the concern is why the unit test is still failing. sorry!

Ah, based on what I see here:

integration.go:184: 
        	Error Trace:	/home/runner/work/jaeger/jaeger/plugin/storage/integration/integration.go:184
        	Error:      	Should be true
        	Test:       	TestMemoryStorage/GetOperations
    integration.go:185: 	 Expected: [{example-operation-1 } {example-operation-3 server} {example-operation-4 client}]
    integration.go:186: 	 Actual  : [{example-operation-1 unspecified} {example-operation-3 server} {example-operation-4 client}]

EDIT: The assertion is in the integration test here

Also, my suggestion was incorrect, and should be:

assert.Equal(t, "unspecified", spanKind.String())

unless if we don't want to change the original behaviour, which means mapping "unspecified" back to "".

@codecov
Copy link

codecov bot commented Aug 2, 2023

Codecov Report

Patch coverage: 100.00% and project coverage change: -0.01% ⚠️

Comparison is base (82e426c) 97.07% compared to head (79428b7) 97.06%.
Report is 5 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4614      +/-   ##
==========================================
- Coverage   97.07%   97.06%   -0.01%     
==========================================
  Files         301      301              
  Lines       17876    17877       +1     
==========================================
  Hits        17353    17353              
- Misses        419      420       +1     
  Partials      104      104              
Flag Coverage Δ
unittests 97.06% <100.00%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Changed Coverage Δ
cmd/query/app/apiv3/otlp_translator.go 100.00% <100.00%> (ø)
model/span.go 100.00% <100.00%> (ø)
plugin/storage/cassandra/spanstore/writer.go 100.00% <100.00%> (ø)
plugin/storage/integration/integration.go 75.95% <100.00%> (-0.39%) ⬇️
plugin/storage/memory/memory.go 100.00% <100.00%> (ø)

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Signed-off-by: Afzal Ansari <[email protected]>
albertteoh
albertteoh previously approved these changes Aug 2, 2023
Copy link
Contributor

@albertteoh albertteoh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with a tiny nit.

model/span.go Outdated Show resolved Hide resolved
model/span_test.go Outdated Show resolved Hide resolved
Signed-off-by: Afzal <[email protected]>
model/span.go Outdated Show resolved Hide resolved
@yurishkuro yurishkuro changed the title [WIP] Replace OT trace with otel trace spans type to span model Replace usage of OpenTracing constants with OpenTelemetry in the domain model Aug 3, 2023
@yurishkuro yurishkuro merged commit 312746b into jaegertracing:main Aug 3, 2023
31 checks passed
@afzalbin64 afzalbin64 deleted the add-otel-trace-span-model branch August 4, 2023 01:25
@afzal442 afzal442 restored the add-otel-trace-span-model branch August 14, 2023 04:13
@afzal442 afzal442 deleted the add-otel-trace-span-model branch August 14, 2023 14:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants