fix(node-experimental): Ensure http breadcrumbs are correctly added #8937
Conversation
Due to the way OTEL handles context (=scope/hub), I noticed that breadcrumbs added in the http integration are not correctly captured. The reason is that OTEL forks the hub for the http request context, and runs the hooks inside of this forked context. In order to deal with this, we have to actually attach the breadcrumb to the _parent_ Hub in this specific case.
| } | ||
|
|
||
| const data = getRequestSpanData(span); | ||
| getParentHub().addBreadcrumb( |
There was a problem hiding this comment.
Technically, the most correct (?) would be to add the breadcrumb both to the current and the parent hub 🤔 does that make sense? We could also make it e.g.:
const hub = getCurrentHub();
const parentHub = getParentHub();
hub.addBreadcrumb(breadcrumb);
if (hub !== parentHub) {
parentHub.addBreadcrumb(breadcrumb);
}To be on the very safe side? The breadcrumb to the "actual current" hub would only really be relevant/visible if something happened inside of the OTEL internals, I believe (but it's a bit hard to say with all the callbacks/nested function scopes/etc.)
There was a problem hiding this comment.
Without being too deep into this, it sounds reasonable to me to add it to both.
size-limit report 📦
|
|
Note that one thing that does not solve is that if you have this function: function makeRequest(url) {
return new Promise((resolve) => {
http.request(url, (httpRes) => {
console.log('log inside of http callback');
httpRes.on('data', () => {});
httpRes.on('end', () => {
resolve();
});
}).end();
});
}The |
| } | ||
|
|
||
| const data = getRequestSpanData(span); | ||
| getParentHub().addBreadcrumb( |
There was a problem hiding this comment.
Without being too deep into this, it sounds reasonable to me to add it to both.
|
Closing this, as actually this needs more in-depth fixes to properly work 😬 |
#8966) This is a fork of #8937, with only the "uncontroversial" stuff, mainly fixing that we only create HTTP breadcrumbs for _outgoing_ requests. In addition, this also migrates to using `requestHook` and `responseHook` instead of `applyCustomAttributesOnSpan`. We may have to revisit this later, but these hooks seem to have a better context awareness (=they are called in a more reasonable OTEL context, which gives the callbacks there better access to scope data etc). However that means we cannot (easily) pass both request and response as breadcrumb hints - not sure how important that is to us... For now I'd say that's OK. Note that also `requestHook` is only called when the request finishes, so we already have all the response OTEL span attributes correctly set there.
Due to the way OTEL handles context (=scope/hub), I noticed that breadcrumbs added in the http integration are not correctly captured. The reason is that OTEL forks the hub for the http request context, and runs the hooks inside of this forked context. In order to deal with this, we have to actually attach the breadcrumb to the parent Hub in this specific case.
So this code:
Results in a correct transaction:
and a correct error
This is a bit hacky, but works as far as I can tell. Here is the code that "triggers" this: https://github.com/open-telemetry/opentelemetry-js/blob/main/experimental/packages/opentelemetry-instrumentation-http/src/http.ts#L532. note that the
context.with()above results in a hub fork for us.Not sure if there is a more elegant solution for this...? But I guess this is a fundamental challenge when we try to align our hub/scope usage with OTEL context.
One note on
requestHook&responseHookvs.applyCustomAttributesOnSpan: These differ slightly in when/where they are executed. I got less consistent/logical results of hub propagation on that hook, where thexxxHookcallbacks worked more reliably for me. I think the reason is thatapplyCustomAttributesOnSpanis called a bit later in the request lifecycle... However, the downside is that we do not easily have the request and response available in the callback, so we can't easily add it as a breadcrumb hint (without caching it somewhere, ...). Not sure how important that is to us...