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

Support for OTEL_PROPAGATORS environment variable #5147

Merged
merged 5 commits into from Mar 9, 2023

Conversation

krak3n
Copy link
Contributor

@krak3n krak3n commented Oct 14, 2022

Currently the propagators are hard coded to W3C TraceContext and Baggage within the tracing module here.

This makes it difficult to integrate Caddy into existing systems that do not use W3C TraceContext or Baggage for propagating traces.

This PR adds support to the caddyhttp/tracing module to support the OpenTelemetry OTEL_PROPAGATORS environment variable which allows users to configure the propagators used to extract and inject trace data from incoming and outgoing requests.

For example, users will now be able to define their propagators like this:

export OTEL_PROPAGATORS=b3,b3multi,baggage

As defined here on the OpenTelemetry SDK configuration document.

This is achieved by using the go.opentelemetry.io/contrib/propagators/autoprop propagator which also uses the W3C TraceContext and Baggage as the default if no propagators are set on the OTEL_PROPAGATORS environment variable, see here

@CLAassistant
Copy link

CLAassistant commented Oct 14, 2022

CLA assistant check
All committers have signed the CLA.

@mholt mholt added the under review 🧐 Review is pending before merging label Oct 15, 2022
@mholt
Copy link
Member

mholt commented Oct 15, 2022

Thanks for the PR. I will need to check on something before we merge this, as the otel dependency is a bit tricky for some of our users.

@starkers
Copy link

starkers commented Mar 9, 2023

Thanks for the PR. I will need to check on something before we merge this, as the otel dependency is a bit tricky for some of our users.

Any progress on this @mholt ? support for b3 (or others) is quite important

@mholt
Copy link
Member

mholt commented Mar 9, 2023

We can merge this, but the lint errors need fixing.

Edit: Nevermind -- just looked and for some reason the linter has started complaining about code unrelated to this change. 🤔

I'll look into this...

@mholt
Copy link
Member

mholt commented Mar 9, 2023

I don't think those linter errors make any sense. That's got to be a bug. The code shouldn't even be compiling if some of those are true.

I'll merge this and cross fingers. Thanks @krak3n !

@mholt mholt merged commit b420561 into caddyserver:master Mar 9, 2023
19 of 22 checks passed
@mholt mholt removed the under review 🧐 Review is pending before merging label Mar 9, 2023
@mholt mholt added this to the v2.6.5 milestone Mar 9, 2023
@starkers
Copy link

starkers commented Mar 9, 2023

I'll be testing this today, maybe prod next week ;-)

@krak3n
Copy link
Contributor Author

krak3n commented Mar 9, 2023

Awesome thank you @mholt 🎉

@francislavoie francislavoie added the feature ⚙️ New feature or request label Mar 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature ⚙️ New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants