node http integration: ignoreIncomingRequests() option does not work

[Bruno-DaSilva]

Is there an existing issue for this?

• I have checked for existing issues https://github.com/getsentry/sentry-javascript/issues
• I have reviewed the documentation https://docs.sentry.io/
• I am using the latest SDK release https://github.com/getsentry/sentry-javascript/releases

How do you use Sentry?

Sentry Saas (sentry.io)

Which SDK are you using?

@sentry/node

SDK Version

8.17.0

Framework Version

No response

Link to Sentry event

No response

SDK Setup/Reproduction Example

  Sentry.init({
    dsn:
      '...',
    integrations: [
      Sentry.httpIntegration({
        ignoreIncomingRequests: (url: string) => {
          console.log(url); // This is always '///'.
          const shouldIgnore =
            url.includes('/liveness') ||
            url.includes('/readiness') ||
            url.includes('/metrics');

          return shouldIgnore;
        },
      }),
    ],
  });

Steps to Reproduce

1. init sentry as above
2. send request to service
3. observe all incoming http urls are passed as '///' in the logs

Expected Result

all incoming http urls are a string matching the request, usable to filter ignore true/false

Actual Result

all incoming http urls are passed as '///' in the logs.

[Bruno-DaSilva]

After some digging I found that the issue was introduced in this PR: #10443
Basically, the code assumes that ignoreOutgoingRequestHook and ignoreIncomingRequestHook otel hooks supply the same request object type: RequestOptions. However, ignoreIncomingRequestHook actually passes an IncomingMessage object which has a totally different signature:
https://github.com/open-telemetry/opentelemetry-js/blob/54b14fbbe33e0cf38115ee8c5c934a4e27786186/experimental/packages/opentelemetry-instrumentation-http/src/types.ts#L59-L65

As a result, when we call getRequestUrl(request):

sentry-javascript/packages/node/src/integrations/http.ts

Line 106 in eaf6055

const url = getRequestUrl(request);

which expects a RequestOptions object:

sentry-javascript/packages/node/src/utils/getRequestUrl.ts

Line 4 in eaf6055

export function getRequestUrl(requestOptions: RequestOptions): string {

all of the params it tries to access are undefined:

sentry-javascript/packages/node/src/utils/getRequestUrl.ts

Lines 4 to 15 in eaf6055

	export function getRequestUrl(requestOptions: RequestOptions): string {
	const protocol = requestOptions.protocol || '';
	const hostname = requestOptions.hostname || requestOptions.host || '';
	// Don't log standard :80 (http) and :443 (https) ports to reduce the noise
	// Also don't add port if the hostname already includes a port
	const port =
	!requestOptions.port || requestOptions.port === 80 || requestOptions.port === 443 || /^(.*):(\d+)$/.test(hostname)
	? ''
	: `:${requestOptions.port}`;
	const path = requestOptions.path ? requestOptions.path : '/';
	return `${protocol}//${hostname}${port}${path}`;
	}

leading to the method returning "///".

[Bruno-DaSilva]

One option: can we return the full request object back to the user in the ignoreIncomingRequests() user supplied hook?
This lets the user decide what criteria they want to use for requests that isn't just URL (eg. they could check the headers or body for something and ignore a request that way).

[Bruno-DaSilva]

Alternatively: is it acceptable to pass just request.url (it does not include protocol, hostname, etc. - just the path and maybe params) instead of trying to parse more info out?
Then you could do const url = request.url; instead of getRequestUrl(request).

I don't see anywhere to get the extra bits of protocol, hostname, etc. in the IncomingMessage type.

[Lms24]

Hey @Bruno-DaSilva thanks for writing in and for the excellent detective work! I'll take a look at this today and respond to your suggestions a bit later.

[Lms24]

I can confirm that currently, ignoreIncomingRequests only receives /// instead of usable URLs.

To fix this, I think the best option is to take request.url and pass it to the callback as you suggested. We cannot change the signature of ignoreIncomingRequests within the v8 major as it's a breaking change and to me this just looks like a bug/something we copied from ignoreOutgoingRequests where we do indeed get RequestOptions and the url works as expected. Though I really wonder why TypeScript doesn't flag the types mismatch 🤔

If this is enough, we're good. Otherwise, we can still let users overwrite the Otel's ignoreIncomingRequestHook just like we do with some other constructor options of Otel's HttpInstrumentation.

[Bruno-DaSilva]

Hey @Lms24 thanks for jumping on this so quickly :)

To fix this, I think the best option is to take request.url and pass it to the callback as you suggested.

yup that sounds reasonable to me.

Though I really wonder why TypeScript doesn't flag the types mismatch 🤔

May be worth a deeper look. Seems it'd be prone to future mistakes in general if we're somehow disabling type checks inadvertently here. I don't have the sdk setup locally to run any typescript builds atm.

---

Otherwise, we can still let users overwrite the Otel's ignoreIncomingRequestHook just like we do with some other constructor options of Otel's HttpInstrumentation.

I was unsure if this is possible here. Are you referring to passing in the experimentalConfig?

sentry-javascript/packages/node/src/integrations/http.ts

Line 85 in eaf6055

..._httpOptions.instrumentation?._experimentalConfig,

The issue with that is the ..._experimentalConfig expansion happens before the sentry-default ones, so don't the sentry-default ones override any same-keys we specify in _experimentalConfig?
eg. if I set _experimentalConfig.ignoreIncomingRequestHook() to a custom hook, won't sentry's override mine?

I'll admit I haven't tried it so instead i'm relying on a patch-package config instead. If the above is true, do we want to move _experimentalConfig below the sentry-defined parameters so they can act as overrides? That could be a breaking change if people are adding some ineffective hooks lol (though I suppose the interface marks it as can-break-at-any-time)

[Lms24]

you're totally right. I also noticed that _experimentalConfig is spread too early/not applicable like this for the two ignore.. hooks. Gonna tag @mydea who implemented _experimentalConfig: Did we do this on purpose? I see a lot of default logic is added to our implementation of these two hooks but maybe it's fine for users to entirely overwrite it?

[mydea]

Hey, yes it is on purpose that we override this, we do not allow to override this because we depend on our custom functionality. The idea was that it should be enough to look at the URL we pass into the proper options we have, if that is incorrect we def. should fix this! Also we could think about adding the request as a second argument to the callback, I guess, this should be non-breaking I believe? Would be an escape hatch at least!

[Lms24]

Also we could think about adding the request as a second argument to the callback, I guess, this should be non-breaking I believe? Would be an escape hatch at least!

that's a good idea! I opened #12929 to fix the url param. Will look into adding the second param as a follow-up!

[Bruno-DaSilva]

Also we could think about adding the request as a second argument to the callback, I guess, this should be non-breaking I believe? Would be an escape hatch at least!

I like this idea; you get the full benefit creating your own hook (access to the request arg and influence on the return) without totally overriding the sentry one.

Then the sentry built-in function becomes more of a wrapper/middleware around the user's custom function. I like that generally to allow power users as much extensibility as possible :)

[Lms24]

Hey, the initial fix for the url param was released with v8.18.0

Adding the second request parameter was just merged (#12930) but will only be released with the next version of the SDK.

[Bruno-DaSilva]

damn, y'all move fast! Thanks so much for jumping on this quickly and with care @Lms24 @mydea. 🙏

[Lms24]

The second request parameter was released with version 8.19.0 last friday. I also just merged the documentation. It should be available in the next hours at https://docs.sentry.io/platforms/javascript/guides/node/configuration/integrations/http/

[Bruno-DaSilva]

Wooo!!! thank you @Lms24 <3