feat(http): Allow to opt-out of instrumenting incoming/outgoing requests

[mydea]

trafficstars

This PR introduces two new options for the HTTP instrumentation: instrumentOutgoingRequests & instrumentIncomingRequests.

If these are set to false, the instrumentation will completely skip instrumenting the respective methods on the http(s) modules.

Why is this useful

This can be useful/necessary if there is other instrumentation that handles incoming or outgoing requests. For example, Next.js handles incoming requests but not outgoing http requests - today, this leaves the user in a weird state where either they add the http instrumentation and get duplicated incoming request spans, or they do not add it and get no outgoing request spans at all.

Note that the existing options like ignoreIncomingRequestHook cannot be used for this, because they actually suppress tracing, which is not necessarily what we want there.

[david-luna]

Hi @mydea

This is an interesting feature. I think having more control over the instrumentation is good :)

Note that the existing options like ignoreIncomingRequestHook cannot be used for this, because they actually suppress tracing, which is not necessarily what we want there.

I didn't get that. Not instrumenting the incoming requests is actually suppressing tracing and metrics for all incoming requests.

Also I'm not an expert but I wonder if there would be trace context issues if one of the instrumentations is disabled. I guess there will be no issues if both instrumentations are using the OTEL API but it would be an issue if one of them does not.

[mydea]

Hi @mydea

This is an interesting feature. I think having more control over the instrumentation is good :)

Note that the existing options like ignoreIncomingRequestHook cannot be used for this, because they actually suppress tracing, which is not necessarily what we want there.

I didn't get that. Not instrumenting the incoming requests is actually suppressing tracing and metrics for all incoming requests.

It's not the same, because if we suppress tracing, any other auto-instrumentation inside of this will also be suppressed, vs. not suppressing it at all just does nothing. It's something like this:

function incomingHttpRequest() {
  // this does something, imagine this is inside of `http`
}

suppressTracing(context.active(), () => {
  return nextjsInstrumentHttp(incomingHttpRequest);
});

[github-actions[bot]]

This PR is stale because it has been open 60 days with no activity. Remove stale label or comment or this will be closed in 14 days.

[mydea]

Any thoughts about merging this? I think it would be valuable, but also happy to close otherwise...

[trentm]

^^ I merged from main, dealing with the http.ts conflict resulting from my merge of https://github.com/open-telemetry/opentelemetry-js/pull/4866