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

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

mydea
Copy link

@mydea mydea commented Apr 17, 2024

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.

@mydea mydea requested a review from a team as a code owner April 17, 2024 12:03
@david-luna
Copy link
Contributor

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
Copy link
Author

mydea commented Apr 29, 2024

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);
});

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.

None yet

2 participants