Skip to content

Conversation

@tombruijn
Copy link
Member

@tombruijn tombruijn commented Aug 18, 2025

Prevent OpenTelemetry from sending tracing headers to other applications through HTTP requests.

Use an unconfigured CompositePropagator to disable HTTP propagation headers while maintaining local trace correlation. It is not configured with any other propagators, so it will not add the headers to the outgoing requests.

I hope this fixes the issue with app data being reported to the wrong app, if one app makes a request to another app and they're both instrumented with OpenTelemetry.

To do

Resources

Prevent OpenTelemetry from sending tracing headers to other applications
through HTTP requests.

Use an unconfigured `CompositePropagator` to disable HTTP
propagation headers while maintaining local trace correlation.
It is not configured with any other propagators, so it will not add the
headers to the outgoing requests.

I hope this fixes the issue with app data being reported to the wrong
app, if one app makes a request to another app and they're both
instrumented with OpenTelemetry.
@tombruijn tombruijn self-assigned this Aug 18, 2025
@tombruijn tombruijn added the bug Confirmed and unconfirmed bugs reported by us and customers. label Aug 18, 2025
@tombruijn tombruijn requested a review from unflxw August 18, 2025 15:51
@tombruijn tombruijn marked this pull request as ready for review August 18, 2025 15:51
Copy link
Contributor

@unflxw unflxw left a comment

Choose a reason for hiding this comment

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

I wonder/worry this might break instrumentations that use propagation for other reasons -- for example, our own BullMQ instrumentation, but also others.

It would be worth running the BullMQ instrumentation's tests (for older BullMQ versions, as the instrumentation is broken on latest BullMQ) with this propagator configuration and seeing if it works.

@backlog-helper

This comment has been minimized.

@unflxw
Copy link
Contributor

unflxw commented Aug 21, 2025

It would seem this breaks propagation at large: adding provider.register({propagator: new CompositePropagator()}); (which is what passing a textMapPropagator to the NodeSDK ultimately does) at the top of the beforeEach in the BullMQ instrumentation's tests causes the tests that rely on propagation to fail.

This by itself could be an acceptable breakage, but other instrumentations might rely on propagation as well. We may wish to disable propagation specifically for the HTTP integration, if that's possible.

@backlog-helper
Copy link


This is a message from the daily scheduled checks.

New issue guide | Backlog management | Rules | Feedback

@tombruijn
Copy link
Member Author

It would seem this breaks propagation at large: adding provider.register({propagator: new CompositePropagator()}); (which is what passing a textMapPropagator to the NodeSDK ultimately does) at the top of the beforeEach in the BullMQ instrumentation's tests causes the tests that rely on propagation to fail.

This by itself could be an acceptable breakage, but other instrumentations might rely on propagation as well. We may wish to disable propagation specifically for the HTTP integration, if that's possible.

That's weird. If I run the test app we have it does work with the changes in this PR.

Request timeline:

2025-08-22 at 11 20 20 - Incident #2 - Sample - AppSignal

Worker timeline:

2025-08-22 at 11 20 36 - Incident #1 - Sample - AppSignal

@tombruijn
Copy link
Member Author

I don't see a way to disable the trace headers to be disabled in the HTTP instrumentation. Do you think the suggested change is still acceptable?

@unflxw
Copy link
Contributor

unflxw commented Aug 22, 2025

That's weird. If I run the test app we have it does work with the changes in this PR.

Ah, yes, for our purposes, the BullMQ instrumentation will work just fine with propagation disabled. It will fail to create a link between the process.bullmq and the publish.bullmq spans, but this is fine, because we don't send span links to the agent.

Actually, I can't think of an use case for propagation that we wouldn't be okay with breaking -- its whole use case is having different processes share the same trace, which is what we want to avoid.

So it's likely okay to merge this as-is, and we'll deal with issues if they arise.

The previous commit removed all default OpenTelemetry propagators.
This may break things we don't want, where our customers make use of the
baggage for example.

So, lets add back the default propagators, to just disable the
`tracecontext` propagator.

https://github.com/open-telemetry/opentelemetry-js/blob/e5c02e7cd266597e8658fcf3c1525b48f924082f/experimental/packages/opentelemetry-sdk-node/src/utils.ts#L203-L212
@tombruijn tombruijn merged commit 8f7bc29 into main Aug 22, 2025
33 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Confirmed and unconfirmed bugs reported by us and customers.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants