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

Implemented logic for Opentracing baggage propagation #1880

Merged

Conversation

jaronoff97
Copy link
Contributor

What

This PR implements the logic for extracting baggage in the opentracing propagator.

Why

Users of the opentracing baggage propagator want to extract baggage so that they can effectively migrate from opentracing to opentelemetry.

Other Implementations:

Java
Python
Javascript

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Mar 3, 2022

CLA Signed

The committers are authorized under a signed CLA.

@jaronoff97
Copy link
Contributor Author

Pending CLA authorization from Lightstep right now.

@codecov
Copy link

codecov bot commented Mar 3, 2022

Codecov Report

Merging #1880 (170e137) into main (d1b7211) will increase coverage by 0.1%.
The diff coverage is 100.0%.

Impacted file tree graph

@@           Coverage Diff           @@
##            main   #1880     +/-   ##
=======================================
+ Coverage   70.5%   70.6%   +0.1%     
=======================================
  Files        129     129             
  Lines       5564    5587     +23     
=======================================
+ Hits        3925    3948     +23     
  Misses      1500    1500             
  Partials     139     139             
Impacted Files Coverage Δ
propagators/ot/ot_propagator.go 94.5% <100.0%> (+1.8%) ⬆️

@carlosalberto
Copy link

Looks great, thanks!

@jaronoff97
Copy link
Contributor Author

@carlosalberto one result of this behavior is that if baggage from the opentracing side is invalid, we don't create a context from remote span. Should we instead change this block to be something like:

         bags, err := extractBags(carrier)
	if err != nil {
		return trace.ContextWithRemoteSpanContext(ctx, sc)
	}
	ctx = baggage.ContextWithBaggage(ctx, bags)
	return trace.ContextWithRemoteSpanContext(ctx, sc)

@carlosalberto
Copy link

@jaronoff97 Oh, good call - indeed, invalid Baggage shouldn't prevent a valid SpanContext from being returned.

propagators/ot/ot_propagator.go Outdated Show resolved Hide resolved
propagators/ot/ot_propagator.go Outdated Show resolved Hide resolved
Copy link
Member

@hanyuancheung hanyuancheung left a comment

Choose a reason for hiding this comment

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

lgtm

propagators/ot/ot_propagator.go Outdated Show resolved Hide resolved
Co-authored-by: Chester Cheung <cheung.zhy.csu@gmail.com>
@Aneurysm9 Aneurysm9 merged commit fcd5fb0 into open-telemetry:main Mar 9, 2022
@MrAlias MrAlias mentioned this pull request Mar 16, 2022
@jaronoff97 jaronoff97 deleted the jaronoff97/ot-baggage-props branch June 30, 2022 14:50
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.

None yet

6 participants