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)