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

http.route missing from spans after enabling Sentry with OTEL #14660

Open
3 tasks done
jirimoravcik opened this issue Dec 11, 2024 · 11 comments
Open
3 tasks done

http.route missing from spans after enabling Sentry with OTEL #14660

jirimoravcik opened this issue Dec 11, 2024 · 11 comments
Labels
Package: node Issues related to the Sentry Node SDK

Comments

@jirimoravcik
Copy link

jirimoravcik commented Dec 11, 2024

Is there an existing issue for this?

How do you use Sentry?

Sentry Saas (sentry.io)

Which SDK are you using?

@sentry/node

SDK Version

8.43.0

Framework Version

Node js 20.12.2

Link to Sentry event

No response

Reproduction Example/SDK Setup

First, we initialize sentry using:

Sentry.init({
        dsn: process.env.SENTRY_DSN,
        environment: process.env.SENTRY_ENVIRONMENT || 'local',
        release: process.env.BUILD_VERSION || 'local_version',
        skipOpenTelemetrySetup: true, // We have our custom OpenTelemetry setup
        registerEsmLoaderHooks: false,
        beforeBreadcrumb() { return null; }, // Disable breadcrumbs
        beforeSend(event, hint) {
            // some stuff that doesn't affect this in any way
        },
    });

Next, we initialize OpenTelemetry SDK using:

import type { InstrumentationConfig } from '@opentelemetry/instrumentation';
import { AwsInstrumentation, type AwsSdkInstrumentationConfig } from '@opentelemetry/instrumentation-aws-sdk';
import { DnsInstrumentation, type DnsInstrumentationConfig } from '@opentelemetry/instrumentation-dns';
import { ExpressInstrumentation, type ExpressInstrumentationConfig } from '@opentelemetry/instrumentation-express';
import { FsInstrumentation, type FsInstrumentationConfig } from '@opentelemetry/instrumentation-fs';
import { HttpInstrumentation, type HttpInstrumentationConfig } from '@opentelemetry/instrumentation-http';
import { IORedisInstrumentation, type IORedisInstrumentationConfig } from '@opentelemetry/instrumentation-ioredis';
import { MongoDBInstrumentation, type MongoDBInstrumentationConfig } from '@opentelemetry/instrumentation-mongodb';
import { NestInstrumentation } from '@opentelemetry/instrumentation-nestjs-core';
import { NetInstrumentation } from '@opentelemetry/instrumentation-net';
import { envDetectorSync } from '@opentelemetry/resources';
import { NodeSDK } from '@opentelemetry/sdk-node';

const sdk = new NodeSDK({
    instrumentations: [
        new AwsInstrumentation({
            //  This hides the underlying HTTP spans that aren't useful since we already have instrumentation for AWS SDK
            suppressInternalInstrumentation: true,
            ...config?.awsInstrumentationConfig,
        }),
        new DnsInstrumentation(config?.dnsInstrumentationConfig),
        new ExpressInstrumentation(config?.expressInstrumentationConfig),
        new FsInstrumentation(config?.fsInstrumentationConfig),
        new HttpInstrumentation({
            ignoreIncomingRequestHook(req) {
                // Do not include health check, icon, and metrics endpoints in traces.
                const isHealthCheckIconOrMetricsUrl = ['/health-check', '/favicon.ico', '/metrics'].includes(req.url || '');
                return isHealthCheckIconOrMetricsUrl;
            },
            ...config?.httpInstrumentationConfig,
        }),
        new IORedisInstrumentation(config?.ioRedisInstrumentationConfig),
        new MongoDBInstrumentation(config?.mongoDBInstrumentationConfig),
        new NestInstrumentation(config?.nestInstrumentationConfig),
        new NetInstrumentation(config?.netInstrumentationConfig),
    ],
    resourceDetectors: [envDetectorSync],
});

sdk.start();

Steps to Reproduce

If you use the setup above, you'll have spans that look like following (coming from debug OpenTelemetry logs):

Span {
    attributes: {
      'http.url': 'http://localhost:3333/v2/users/me?token=secret',
      'http.host': 'localhost:3333',
      'net.host.name': 'localhost',
      'http.method': 'GET',
      'http.scheme': 'http',
      'http.target': '/v2/users/me?token=secret',
      'http.user_agent': 'Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_7) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/131.0.0.0 Safari/537.36',
      'http.flavor': '1.1',
      'net.transport': 'ip_tcp',
      'sentry.origin': 'auto.http.otel.http',
      'net.host.ip': '::1',
      'net.host.port': 3333,
      'net.peer.ip': '::1',
      'net.peer.port': 59381,
      'http.status_code': 200,
      'http.status_text': 'OK'
    },
}

If I turn Sentry instrumentation off, there'll also be http.route, which is coming from OpenTelemetry express instrumentation.

If I instrument OpenTelemetry before Sentry, it breaks the ignoreIncomingRequestHook defined above (i.e. it doesn't get called at all, probably overwritten by something from Sentry)

Expected Result

The expected span would be something like the following (which I can get when I turn Sentry off):

Span {
    attributes: {
      'http.url': 'http://localhost:3333/v2/users/me?token=secret',
      'http.host': 'localhost:3333',
      'net.host.name': 'localhost',
      'http.method': 'GET',
      'http.scheme': 'http',
      'http.target': '/v2/users/me?token=secret',
      'http.user_agent': 'Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_7) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/131.0.0.0 Safari/537.36',
      'http.flavor': '1.1',
      'net.transport': 'ip_tcp',
      'net.host.ip': '::1',
      'net.host.port': 3333,
      'net.peer.ip': '::1',
      'net.peer.port': 61320,
      'http.status_code': 200,
      'http.status_text': 'OK',
      'http.route': '/v2/users/:username'
    },
}

you can notice the http.route at the end, that's what's missing.

Actual Result

Span {
    attributes: {
      'http.url': 'http://localhost:3333/v2/users/me?token=secret',
      'http.host': 'localhost:3333',
      'net.host.name': 'localhost',
      'http.method': 'GET',
      'http.scheme': 'http',
      'http.target': '/v2/users/me?token=secret',
      'http.user_agent': 'Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_7) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/131.0.0.0 Safari/537.36',
      'http.flavor': '1.1',
      'net.transport': 'ip_tcp',
      'sentry.origin': 'auto.http.otel.http',
      'net.host.ip': '::1',
      'net.host.port': 3333,
      'net.peer.ip': '::1',
      'net.peer.port': 59381,
      'http.status_code': 200,
      'http.status_text': 'OK'
    },
}
@getsantry getsantry bot moved this to Waiting for: Product Owner in GitHub Issues with 👀 3 Dec 11, 2024
@github-actions github-actions bot added the Package: node Issues related to the Sentry Node SDK label Dec 11, 2024
@lforst
Copy link
Member

lforst commented Dec 11, 2024

Hi, we need to narrow this down a bit (unless you happen to be able to provide repro). Is the route attr that is missing related to express or nestjs? I am wondering if we are stomping the options of one of the instrumentations you provided.

@jirimoravcik
Copy link
Author

Hello, it's an express API. IMO you should find the actual code doing it in https://github.com/open-telemetry/opentelemetry-js-contrib/blob/main/plugins/node/opentelemetry-instrumentation-express/src/instrumentation.ts#L175-L183

Let me know if it helps. Thank you

@getsantry getsantry bot moved this to Waiting for: Product Owner in GitHub Issues with 👀 3 Dec 11, 2024
@lforst
Copy link
Member

lforst commented Dec 11, 2024

Thanks. Looking at our code, I cannot find any places relevant to this stack where we are overriding http.route 🤔 Would you mind providing a minimal repro example we could pull to reproduce this and debug more effectively?

@jirimoravcik
Copy link
Author

I'll try to prepare a repro example for you.

@getsantry getsantry bot moved this to Waiting for: Product Owner in GitHub Issues with 👀 3 Dec 11, 2024
@mydea
Copy link
Member

mydea commented Dec 12, 2024

Just looking at this again, one thing that is incorrect is that you have to opt-out of the Sentry SDK emitting spans, like this:

Sentry.init({
        dsn: process.env.SENTRY_DSN,
        environment: process.env.SENTRY_ENVIRONMENT || 'local',
        release: process.env.BUILD_VERSION || 'local_version',
        skipOpenTelemetrySetup: true, // We have our custom OpenTelemetry setup
        registerEsmLoaderHooks: false,
        beforeBreadcrumb() { return null; }, // Disable breadcrumbs
        beforeSend(event, hint) {
            // some stuff that doesn't affect this in any way
        },
       integrations: [Sentry.httpIntegration({ spans: false })]
    });

You can see by 'sentry.origin': 'auto.http.otel.http', that this is coming from one of our integrations, maybe this conflicts with your version of HttpInstrumentation somehow 🤔

cc @lforst we should probably flip this default in v9 too, so this is false by default if skipOpenTelemetrySetup: true is set - we could not do this in v8 due to backwards compatibility reasons, but it is the more sensible default I'd say?

@jirimoravcik
Copy link
Author

@mydea thanks for the idea. I just tested it and 'sentry.origin': 'auto.http.otel.http' disappeared from the span, but http.route is still missing.

I don't see the @opentelemetry_sentry-patched/instrumentation-http in logs anymore, but still see @sentry/instrumentation-http so I'd guess that's what causing the problem?

@mydea
Copy link
Member

mydea commented Dec 12, 2024

So this is the correct/intended outcome then - the @opentelemetry_sentry-patched/instrumentation-http logs are what emits spans, so this being gone is what you want (you emit your own http spans!). @sentry/instrumentation-http does not emit any spans, it is only used to make sure that request isolation works for error monitoring and should not interfer with any spans at all 🤔 Can you paste the new http span you are getting?

Could you also share your package.json? My best guess is that this has to do with some versioning stuff, e.g. the Sentry SDK has dependencies on various instrumentation packages, possibly those conflict with some versions you have (should not be the case but 🤷 package managers can be weird that way...)

@jirimoravcik
Copy link
Author

Ok, I found out @sentry/node depends on different versions of OpenTelemetry instrumentations than our internal setup, so I think that's what may be causing the problems. I'll investigate further.

@getsantry getsantry bot moved this to Waiting for: Product Owner in GitHub Issues with 👀 3 Dec 12, 2024
@jirimoravcik
Copy link
Author

Ok, after some more investigation this is what happens inside the internals of express and http instrumentations:

  1. Sentry imported but Sentry.init not called
    This works. The following code https://github.com/open-telemetry/opentelemetry-js-contrib/blob/main/plugins/node/opentelemetry-instrumentation-express/src/instrumentation.ts#L190-L192 saves the route to rpcMetadata. This will be picked up in https://github.com/open-telemetry/opentelemetry-js/blob/main/experimental/packages/opentelemetry-instrumentation-http/src/utils.ts#L932-L935
  2. Sentry imported and Sentry.init called
    Doesn't work.
    The following code https://github.com/open-telemetry/opentelemetry-js-contrib/blob/main/plugins/node/opentelemetry-instrumentation-express/src/instrumentation.ts#L190-L192 saves the route to rpcMetadata - this part happens and I can see the route on the object.
    But when the following code is executed https://github.com/open-telemetry/opentelemetry-js/blob/main/experimental/packages/opentelemetry-instrumentation-http/src/utils.ts#L932-L935 route is missing from the rpcMetadata object. i.e. property added by instrumentation-express doesn't make it back to instrumentation-http

@jirimoravcik
Copy link
Author

Ok, managed to fix this by swapping init order.
I first initialize OpenTelemetry node sdk and do Sentry.init afterwards.
Note that it's essential to have integrations: [Sentry.httpIntegration({ spans: false })] in Sentry.init.
If you don't have that, Sentry will overwrite the one configured.

@mydea
Copy link
Member

mydea commented Dec 12, 2024

Hmm, this is weird. Running Sentry.init() first and then otel should not interfere. Could you paste your full instrumentation code, e.g. with all the imports, sentry.init and then the otel setup in one snippet? I'd love to have a look if something else is going on there 🤔 (the httpIntergation({ spans: false}) part is true though, this will be fixed in v9 though: #14678 🎉 )

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Package: node Issues related to the Sentry Node SDK
Projects
Status: No status
Development

No branches or pull requests

3 participants