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 B3 multi header propagation support #495

Merged
merged 4 commits into from
Dec 5, 2024
Merged

Add B3 multi header propagation support #495

merged 4 commits into from
Dec 5, 2024

Conversation

seemk
Copy link
Contributor

@seemk seemk commented Dec 4, 2024

Original PR: #103

I've brought it up to date and added tests, retained the original commits by @kuzaxak

kuzaxak and others added 4 commits September 8, 2022 18:00
Some services expect [B3 Multi][1] headers as input information.
To support that we need to be able to Inject them into upstream requests. Lowercase headers used to be [compatible][2] with Istio Envoy.

Tests will be added as a separate commit later on.

Solving #36

[1]: https://github.com/openzipkin/b3-propagation#multiple-headers
[2]: open-telemetry/opentelemetry-go#765
if (traceContext == nullptr || !traceContext->request_span) {
ngx_log_error(
NGX_LOG_ERR, req->connection->log, 0,
"Unable to get trace context when getting span id");
Copy link
Member

@lalitb lalitb Dec 4, 2024

Choose a reason for hiding this comment

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

nit, can be ignored if doesn't look good -

Suggested change
"Unable to get trace context when getting span id");
"Invalid trace context or request span when getting span id");

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've signed the CLA, but for some reason I'm getting an EasyCLA error when pushing to this branch, will change the wording in another PR.

Copy link
Member

@lalitb lalitb left a comment

Choose a reason for hiding this comment

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

Thanks

@seemk seemk merged commit c80826c into main Dec 5, 2024
63 checks passed
@seemk seemk mentioned this pull request Dec 5, 2024
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.

3 participants