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

ddtrace/tracer/textmap: Added none as a supported propagator for trace context extraction and injection #1610

Merged
merged 5 commits into from Dec 12, 2022

Conversation

dianashevchenko
Copy link
Contributor

What does this PR do?

Following Implementing W3C Trace Context Propagation, which adds none as a supported propagator for trace context extraction and injection.

  • When none is the only propagator listed in the inject or extract list, the corresponding trace context operation is disabled.
  • If there are other propagators in the inject or extract list, the none propagator has no effect.
  • none propagator means no headers will be applied

Motivation

W3C Trace Context Propagation RFC

Describe how to test/QA your changes

  • none applies no headers
  • none,b3 or any other combination of none with other propagators doesn't change existing functionality and tests

Reviewer's Checklist

  • If known, an appropriate milestone has been selected; otherwise the Triage milestone is set.
  • Changed code has unit tests for its functionality.
  • If this interacts with the agent in a new way, a system test has been added.

@dianashevchenko dianashevchenko requested a review from a team December 7, 2022 15:47
@dianashevchenko dianashevchenko modified the milestones: Triage, v1.45.0 Dec 7, 2022
@ajgajg1134 ajgajg1134 changed the title Added none as a supported propagator for trace context extraction and injection ddtrace/tracer/textmap: Added none as a supported propagator for trace context extraction and injection Dec 7, 2022
@@ -177,6 +177,9 @@ func getPropagators(cfg *PropagatorConfig, env string) []Propagator {
if ps == "" {
return defaultPs
}
if ps == "none" {
Copy link
Contributor

Choose a reason for hiding this comment

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

Looking at like 197 here it seems like setting a value of none,b3 would result in a message "unrecognized propagator: none". Can we add a special case for none that instead warns the user something like Propagator "none" has no effect when combined with other propagators. To disable propagator set only "none" (Feel free to change the message). (And a quick little unit test update to make sure that future users don't accidentally break that message)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point 👍 Updated

@pr-commenter
Copy link

pr-commenter bot commented Dec 8, 2022

Benchmarks

Comparing candidate commit 87bd5e0 in PR branch shevchenko/none-ppropagator with baseline commit 4b12722 in branch main.

Found 0 performance improvements and 0 performance regressions! Performance is the same for 6 cases.

@dianashevchenko dianashevchenko merged commit 2790627 into main Dec 12, 2022
@dianashevchenko dianashevchenko deleted the shevchenko/none-ppropagator branch December 12, 2022 20:09
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

3 participants