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

Add ot-tracer-* propagator #562

Merged
merged 14 commits into from
Feb 5, 2021

Conversation

codeboten
Copy link
Contributor

This adds support for the semi-official OpenTracing (aka ot-tracer) header format. Supports propagation of trace context.

NOTE: The support for baggage is incomplete, as this requires iterating through all the keys in a carrier and prefix matching all keys starting with ot-baggage-{key}.

Other implementations:

@codecov
Copy link

codecov bot commented Jan 27, 2021

Codecov Report

Merging #562 (0682926) into main (bc8b54d) will increase coverage by 0.3%.
The diff coverage is 94.4%.

Impacted file tree graph

@@           Coverage Diff           @@
##            main    #562     +/-   ##
=======================================
+ Coverage   77.6%   77.9%   +0.3%     
=======================================
  Files         54      55      +1     
  Lines       2536    2590     +54     
=======================================
+ Hits        1969    2020     +51     
- Misses       437     439      +2     
- Partials     130     131      +1     
Impacted Files Coverage Δ
propagators/ot/ot_propagator.go 94.4% <94.4%> (ø)

carrier.Set(spanIDHeader, sc.SpanID.String())
}

if sc.IsSampled() {

Choose a reason for hiding this comment

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

Wondering whether there's any value on injecting the sampled part if the trace/span ids are invalid.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, probably this whole if/else can move inside the ids-validated branch above.

carrier.Set(spanIDHeader, sc.SpanID.String())
}

if sc.IsSampled() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, probably this whole if/else can move inside the ids-validated branch above.

if err != nil || !sc.IsValid() {
return ctx
}
// TODO: implement extracting baggage
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done!

@yurishkuro
Copy link
Member

semi-official OpenTracing (aka ot-tracer) header format

It's not "semi-official OpenTracing". OpenTracing does not define propagation formats. Is there a spec for this format? Who is asking to support it and why (no ticket linked to the PR)?

Base automatically changed from master to main February 1, 2021 17:09
@codeboten
Copy link
Contributor Author

Updated the name to "OT" propagator based on the conversation in: open-telemetry/opentelemetry-specification#1391

Co-authored-by: Yuri Shkuro <yurishkuro@users.noreply.github.com>
@yurishkuro yurishkuro changed the title Add OpenTracing propagator Add ot-tracer-* propagator Feb 1, 2021
propagators/ot/doc.go Outdated Show resolved Hide resolved
Copy link
Member

@Aneurysm9 Aneurysm9 left a comment

Choose a reason for hiding this comment

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

Please add a CHANGELOG.md entry for this addition.

propagators/ot/ot_propagator.go Outdated Show resolved Hide resolved
Co-authored-by: Anthony Mirabella <a9@aneurysm9.com>
@codeboten
Copy link
Contributor Author

Please add a CHANGELOG.md entry for this addition.

Thanks for the review @Aneurysm9! Added an entry to the changelog

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

5 participants