-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
tracing: Add support for B3 headers via the JAEGER_PROPAGATION env var #1456
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm very sorry for the late reply. I get so many notifications that sometimes things slip my radar. Thank you for the patch, I think it's on a good path but needs some changes before merging.
tracing/tracer.go
Outdated
var configs []jeagerConf.Option | ||
|
||
// This works in other jaeger clients, but is not part of jaeger-client-go | ||
if os.Getenv("JAEGER_PROPAGATION") == "b3" { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please, instead:
- Add
Propagation string
toJaegerConfig
- Use the
Propagation
value here (t.JaegerConfig
) - Set the value here
- Document the new value around here
To stay compatible it probably makes sense to do something like (assumingJAEGER_PROPAGATION
is a de-facto standard):
Propagation: stringsx.Coalesce(
viper.GetString("JAEGER_PROPAGATION")
viperx.GetString(v.l, "tracing.providers.jaeger.propagation", ""),
),
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pushed some new changes, let me know if that works. I also added TRACING_PROVIDER_JAEGER_PROPAGATION
just for consistency.
fc850a3
to
591d653
Compare
Needed for ory/hydra#1456
Your changes to ory/x are now released as v0.0.61 |
ps: If you rebase on master, the module changes should be up to 0.0.61 |
This will provide compatibility with istio.
591d653
to
2d290fc
Compare
@aeneasr all done, tests are passing now. |
Awesome, thank you! |
This will provide compatibility with istio.
Fixes #1447