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

[mydea]

This PR introduces two new options for the HTTP instrumentation: disableIncomingRequestInstrumentation & disableOutgoingRequestInstrumentation.

If these are set to true, 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]

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]

@mydea Thanks for the PR and sorry for the long delay.

We discussed this in the OTel JS SIG meeting last week: https://docs.google.com/document/d/1tCyoQK49WVcE-x8oryZOTTToFm7sIeUhxFPm9g-qL1k/edit#heading=h.a8usr2qdq202

The carrying argument, IMO, was that while the instrumentations are "experimental" (i.e. pre-1.0), we should experiment with them more. IOW, this PR generally sounds good. Some specifics below.

Also note the link from the SIG meeting notes: https://opentelemetry.io/docs/zero-code/java/agent/configuration/#instrumentation-span-suppression-behavior
What the OTel Java SDK is offering is interesting. IIUC, the default span-suppression-strategy=semconv setting would handle the use case you are describing. From the Java code implementing that semconv strategy:

   * <p>For example, nested HTTP client spans will be suppressed; but an RPC client span will not
   * suppress an HTTP client span, if the instrumented RPC client uses HTTP as transport.

Of course, this PR isn't expected to implement something so general.

---

Regarding your use case: Are you getting an HTTP spec from Next.js with spanKind=SERVER for an incoming request, and then a child span of that from instrumentation-http that is also spanKind=SERVER?

[trentm]

Some issues here:

1. Generally the JS SDK, at least, tries to use config vars that are false-y by default. This is because then a false value is treated the same as an undefined value. The <Instrumentation>._config object on the various instrumentations in this and the opentelemetry-js-contrib repo are often just a sparse object where undefined config vars implies the default value. (The huge exception here is the enabled: true config var on all instrumentations. :sadface:) It would potentially be error prone to have a config var where false and undefined are different behaviours.
2. From the config vars and documentation, it isn't clear which of instrumentOutgoingRequests and ignoreOutgoingRequestHook should take precedence.

Without complicating this PR by getting too general, one possible option:

• Change the verb from "instrument" to "disable", e.g. disableIncomingRequestInstrumentation. This flips the boolean sense to deal with issue 1. The term "disable" is closer to the enable/disable state on the an entire Instrumentation, which helps imply that this take precedence over the ignore*Hook vars -- though the docs should still spell that out.

What do you think?

3. It isn't clear from the docs here that instrument* vars are just skipping this one span and the ignore* are supressing. What exactly the existing ignore*RequestHook vars were doing already wasn't clear -- which is an existing issue. Even the term "suppress" here isn't clear ... given that the OTel Java SDK is using (https://opentelemetry.io/docs/zero-code/java/agent/configuration/#instrumentation-span-suppression-behavior) "suppression" to mean just dropping a span and not suppressing the full trace.

• I think this could be solved by expanding on the description (or in a [1] ...-note below the options table) in this section of the README. Personally, I think mentioning the particular use case could be helpful for other users to understand why they might want to use this, and to inform a possible future generalization.

[mydea]

Changing the name to disableIncomingRequestInstrumentation makes a lot of sense to me, that's def. better! I'll update the PR accordingly. I'll also update the docs part to explain this better, thanks for the feedback!

[trentm]

Since your PR, there has been a change from _getConfig() to getConfig() that you'll need to update for. Let me know if you need help with that.

[mydea]

all good I believe, I rebased on master and incorporated the changes, should be OK!

[trentm]

Thanks!

I think it would still be worthwhile to have a paragraph in the README options section mentioning that the disable... settings effectively take precedence over others. However, that could be done in a separate PR -- and I'm happy to do that README update.

I'll leave this for a little bit and probably merge it tomorrow.

[trentm]

^^ I merged from main, dealing with the http.ts conflict resulting from my merge of #4866

[peterabbott]

wouldn't this be better using the isWrapped check ?

if (isWrapped(module.request)) {
this._unwrap(module, 'request')
}