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

OpenTelemetry Express instrumentation does not match Sentry instrumentation #10168

Closed
Tracked by #11070
timfish opened this issue Jan 12, 2024 · 5 comments
Closed
Tracked by #11070
Assignees
Labels

Comments

@timfish
Copy link
Collaborator

timfish commented Jan 12, 2024

Which SDK are you using?

@sentry/node-experimental

SDK Version

latest

Steps to Reproduce

When adding integration tests for @sentry/node-experimental, I started by using the assertions from the @sentry/node tests.

It looks like:

  • transaction for regex routes ends up as just /
  • data.url is now the full URL rather than just the path
  • Http status code tag is not a number rather than string
  • Middleware
    • Descriptions have changed
    • op is gone

I'll add anything else I find!

image
@mydea
Copy link
Member

mydea commented Jan 15, 2024

Good findings!

transaction for regex routes ends up as just /

Hmm, this comes directly from OTEL - not sure if/how we can/want to change this 🤔 what is the actual path that was used in this example?

data.url is now the full URL rather than just the path

I wonder if the OTEL behavior is not more correct there? I see in other places we actually use a full URL there, but not sure what is expected here 🤔 cc @AbhiPrasad ?

Http status code tag is not a number rather than string

Oops, I will fixe this!

Middleware Descriptions have changed & op is gone

Hmm I wonder if we want to/can handle this. can you provide a diff (what it was/became)? Then I can see if we can handle these specifically!

@timfish
Copy link
Collaborator Author

timfish commented Jan 15, 2024

what is the actual path that was used in this example?

It's for this route:

app.get(['/test/arr/:id', /\/test\/arr[0-9]*\/required(path)?(\/optionalPath)?\/(lastParam)?/], (_req, res) => {
  res.send({ response: 'response 4' });
});

Hmm I wonder if we want to/can handle this. can you provide a diff (what it was/became)?

For middlewarre, the Sentry instrumentation:

        spans: [
          expect.objectContaining({
            description: 'corsMiddleware',
            op: 'middleware.express.use',
          }),
        ],

For otel, the descriptions have changed like this, op has become origin?

              expect.objectContaining({
                description: 'middleware - corsMiddleware',
                origin: 'auto.http.otel.express',
              }),

@mydea
Copy link
Member

mydea commented Jan 15, 2024

For middlewarre, the Sentry instrumentation:

        spans: [
          expect.objectContaining({
            description: 'corsMiddleware',
            op: 'middleware.express.use',
          }),
        ],

For otel, the descriptions have changed like this, op has become origin?

              expect.objectContaining({
                description: 'middleware - corsMiddleware',
                origin: 'auto.http.otel.express',
              }),

I'd say the description is OK (it's different, but not wrong - this comes from otel directly).
Origin is "additionally", this is unrelated to that, so alright there. I guess the main issue is the missing op 🤔 I'll take a look if I can add this somehow to keep the op in there!

mydea added a commit that referenced this issue Jan 15, 2024
AbhiPrasad pushed a commit that referenced this issue Jan 16, 2024
- Adds CJS integration tests for express for `@sentry/node-experimental`
- I found some differences with the transactions vs the Sentry
instrumentation (#10168)
- Converts all the existing express tests to use the new runner 
- These are the only tests that dont appear to like the jest runner in
their original form
- Added a `cleanupChildProcesses()` method to the runner than should be
called in the `afterAll` hook to ensure no processes are left
open/leaked

---------

Co-authored-by: Francesco Novy <francesconovy@gmail.com>
@AbhiPrasad
Copy link
Member

Given it seems like OTEL totally drops the ball with paramaterizing with multiple routers, we might have to combine the OTEL integration with ours 😢 (so that we can support multiple routers and dont regress too heavily).

Alternative is that we export an integration that just supports multiple routers, and then people opt-into that, but its not the best UX.

ref #9885

@mydea
Copy link
Member

mydea commented Mar 28, 2024

After moving tests over to the new node, we noticed that actually our express router tests are passing, but only in node 16+. Somehow, on Node 14 multi router support does not fully work, but since Node 16 it seems to work :O Which I guess is fine, we will document this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants