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 b3 (zipkin) OpenTelemetry trace propagation headers #28

Merged
merged 5 commits into from
Apr 17, 2024

Conversation

daniel-chambers
Copy link
Collaborator

@daniel-chambers daniel-chambers commented Apr 17, 2024

This PR adds support for the b3 (zipkin) OpenTelemetry trace propagation headers. The old default was tracecontext and baggage, now we've extended that with b3 and b3-multi by manually configuring the propagators in instrumentation.ts.

Propagators are run in order and each contributes to the trace context. This means if multiple tracing header formats are provided in the request, the last configured propagator for the appropriate formats wins.

In addition:

  • I've added the parent span id to the stdout logs (we already log the trace id and span id); this can help debug trace propagation issues in production
  • I've added request headers to the debug-level tracing, which can also be useful in debugging trace propagation if all else fails. Debug logging is off by default.

This PR makes a similar change to the one in the Rust SDK.

@daniel-chambers daniel-chambers self-assigned this Apr 17, 2024
Copy link
Contributor

@sordina sordina left a comment

Choose a reason for hiding this comment

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

Looks great. I can't recall how the active propagator is selected, or if they're all just propagated, or something else... Might be worth having a quick description in the PR, or in a tracing section of the README. No blockers for merging.

CHANGELOG.md Outdated
Comment on lines 3 to 4
## 4.5.0
- Support b3 (zipkin) OpenTelemetry trace propagation headers ([#28](https://github.com/hasura/ndc-sdk-typescript/pull/28))
Copy link
Contributor

Choose a reason for hiding this comment

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

Neat, might be good to link to the impl or spec for this format.

Comment on lines +57 to +60
// This logs the parent span ID in the pino logs, useful for debugging propagation.
// parentSpanId is an internal property, hence the cast to any, because I can't
// seem to find a way to get at it through a supported API 😭
record["parent_span_id"] = (span as any).parentSpanId;
Copy link
Contributor

Choose a reason for hiding this comment

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

Neat and Tidy 👍

Comment on lines +64 to +71
textMapPropagator: new CompositePropagator({
propagators: [
new W3CTraceContextPropagator(),
new W3CBaggagePropagator(),
new B3Propagator(),
new B3Propagator({ injectEncoding: B3InjectEncoding.MULTI_HEADER }),
]
}),
Copy link
Contributor

Choose a reason for hiding this comment

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

How is the active propagator selected again?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

They're not "selected", they are run in sequence and effectively mutate the tracing context, so the last one wins.

@daniel-chambers daniel-chambers merged commit 322b5dc into main Apr 17, 2024
2 checks passed
@daniel-chambers daniel-chambers deleted the daniel/b3-tracing branch April 17, 2024 05:19
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.

2 participants