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

Propagate baggage through opentracing bridge #4021

Closed

Conversation

shubham-bansal96
Copy link

@shubham-bansal96 shubham-bansal96 commented Apr 19, 2023

Resolves #2793

These changes will allow to propagate baggage through opentracing bridge.

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Apr 19, 2023

CLA Signed

The committers listed above are authorized under a signed CLA.

  • ✅ login: shubham-bansal96 / name: Shubham Bansal (470c154)

@shubham-bansal96 shubham-bansal96 changed the title Propagate baggage through opentracing bridge [WIP] : Propagate baggage through opentracing bridge Apr 19, 2023
@shubham-bansal96 shubham-bansal96 changed the title [WIP] : Propagate baggage through opentracing bridge Propagate baggage through opentracing bridge Apr 20, 2023
@codecov
Copy link

codecov bot commented Apr 20, 2023

Codecov Report

Merging #4021 (2ec2755) into main (eb2b89f) will increase coverage by 0.0%.
The diff coverage is 50.0%.

Additional details and impacted files

Impacted file tree graph

@@          Coverage Diff           @@
##            main   #4021    +/-   ##
======================================
  Coverage   82.1%   82.1%            
======================================
  Files        175     175            
  Lines      12977   13082   +105     
======================================
+ Hits       10655   10752    +97     
- Misses      2102    2108     +6     
- Partials     220     222     +2     
Impacted Files Coverage Δ
propagation/trace_context.go 62.7% <50.0%> (-2.8%) ⬇️

... and 12 files with indirect coverage changes

@@ -63,6 +64,11 @@ func (tc TraceContext) Inject(ctx context.Context, carrier TextMapCarrier) {
sc.SpanID(),
flags)
carrier.Set(traceparentHeader, h)

bStr := baggage.FromContext(ctx).String()
Copy link
Member

Choose a reason for hiding this comment

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

There is a separate baggage propagator that should be used to propagate baggage. This propagator is only for propagating trace context in the w3c tracecontext format. A composite propagator should be used if you want to propagate both trace context and baggage and should not require any changes.

Copy link
Author

@shubham-bansal96 shubham-bansal96 Apr 25, 2023

Choose a reason for hiding this comment

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

Thanks @Aneurysm9 for letting me know, if no changes are required then could you please close this issue ?
#2793

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure that the issue is invalid. This attempt to fix it, however, is.

Copy link
Author

Choose a reason for hiding this comment

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

@Aneurysm9
I wanted you to close the issue, but i think you have mistakenly closed my PR. If my changes doesn't looks good to you then i can try different approach to work on this issue.

I would request you to please open this PR, so i could start working on this issue again. And it will be really helpful for me, if you could please confirm whether this is a valid issue(#2793) or not ??

Copy link
Member

Choose a reason for hiding this comment

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

I intended to close this PR. This approach is not viable. Please open a new PR with a new approach if you would like to try again.

The linked issue appears to be valid from the user reports, but I have not personally validated it. If you present a PR that fixes it in an appropriate manner and includes tests that demonstrate it has been fixed then the issue can be resolved when that PR is merged. Alternately, a PR with tests demonstrating that the functionality is working as expected would also suffice.

@Aneurysm9 Aneurysm9 closed this Apr 25, 2023
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.

Baggage is not propagated by OpenTracing bridge
2 participants