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

Mongoose instrumentation does not work for Mongoose >= 7 #11499

Closed
jennyunh opened this issue Apr 8, 2024 · 17 comments · Fixed by #13587
Closed

Mongoose instrumentation does not work for Mongoose >= 7 #11499

jennyunh opened this issue Apr 8, 2024 · 17 comments · Fixed by #13587
Assignees
Labels
Feature: Performance Package: node Issues related to the Sentry Node SDK

Comments

@jennyunh
Copy link

jennyunh commented Apr 8, 2024

Environment

SaaS (https://sentry.io/)

What are you trying to accomplish?

I've added in the appropriate database integrations to see db queries in my transactions:
https://docs.sentry.io/platforms/node/performance/database/

How are you getting stuck?

Database queries are not being detected and showing up for some transactions.

Sentry initialization:

import * as Sentry from "@sentry/node";
import {ProfilingIntegration} from "@sentry/profiling-node";
import {sentryAddMetrics} from "@/middleware/sentryAddMetrics";

export function initSentry(app): void {
    const isProd = process.env.REACT_APP_ENVIRONMENT === "production";
    Sentry.init({
        dsn: "...",
        environment: process.env.REACT_APP_ENVIRONMENT,
        integrations: [
           
            new ProfilingIntegration(),
            new Sentry.Integrations.Express({
                // to trace all requests to the default router
                app
            }),
            new Sentry.Integrations.Mongo({
                useMongoose: true
            }),
            new Sentry.Integrations.Http({tracing: true})
        ],
        beforeSend: (event, hint) => {
            const error: any = hint.originalException;

            if (error && (
                (error.status && error.status < 500) ||
                (error.httpStatus && error.httpStatus < 500)
            )) {
                return null
            }

            return event;
        },

        // Performance Monitoring
        tracesSampleRate: isProd ? 0.1 : 1.0,
        // Set sampling rate for profiling - this is relative to tracesSampleRate
        profilesSampleRate: 1.0,
    });
    app.use(Sentry.Handlers.requestHandler())
    app.use(Sentry.Handlers.tracingHandler())

    app.use(sentryAddMetrics);

}

No db queries:
Screenshot 2024-04-09 at 10 33 48 AM

Has db queries:
Screenshot 2024-04-09 at 10 35 00 AM

Where in the product are you?

Performance

Link

No response

DSN

https://524f54dda82743f3445b4f28533ef38d@o4505830066159616.ingest.sentry.io/4505869944881152

Version

No response

@getsantry
Copy link

getsantry bot commented Apr 8, 2024

Assigning to @getsentry/support for routing ⏲️

@jennyunh jennyunh changed the title Node SDK: setMeasurement not adding metrics to POST/PUT transactions Node SDK: Some transactions are not showing db queries Apr 9, 2024
@InterstellarStella InterstellarStella transferred this issue from getsentry/sentry Apr 9, 2024
@Lms24
Copy link
Member

Lms24 commented Apr 10, 2024

Hey @jennyunh thanks for writing in!
Can you provide some further information about this behaviour:

  • Which SDK version are you using?
  • Any chance that the express endpoints don't always execute the db queries?
  • Could you link a Sentry event which should contain db spans but doesn't?
  • Most importantly: Is this reproducible in a minimal scenario? If yes please provide a minimal reproducible example?

We're currently working on the next SDK major version (v8) which should increase the quality of our express and DB integration spans because we're using OpentTelemetry instrumentation. Currently, we're still in an alpha state but we're targetting a beta in the next days. Might be worth checking out to see if this behaviour resolved itself.

@jennyunh
Copy link
Author

jennyunh commented Apr 10, 2024

Hi, thank you for your response.
To address your points:

  • I am using:
    "@sentry/node": "^7.109.0",
    "@sentry/profiling-node": "^1.3.5"

  • A Sentry event that should contain db spans but doesn't:

Screenshot 2024-04-10 at 11 49 47 AM

Event id: 4c43068ccb864d8c967645b310452075

  • Here is a minimal API. This API's transaction event is shown in the screenshot above. As you can see, no db spans appear:
app.put("/users/update-profile", userAuthentication, async (req: Request<any, UpdateProfileBody>, res: Response) => {
    const {
        email,
        phoneNumber: phoneNumberBody,
        firstName,
        lastName,
    } = req.body;
    
   await UserModel.findByIdAndUpdate(req.user._id, {
        email,
        firstName,
        lastName,
        updatedAt: Date.now()
    });

    const theUser = await UserModel.findOne({_id: req.user._id});

    console.log(theUser);

    res.sendStatus(200);
})

Appreciate the heads-up on the next version and upcoming beta.

@AbhiPrasad AbhiPrasad added the Package: node Issues related to the Sentry Node SDK label Apr 10, 2024
@Lms24
Copy link
Member

Lms24 commented Apr 12, 2024

Thanks for the information. I don't see anything concretely wrong with this setup on first glance. Have you tried turning on debug: true in Sentry.init to see if there are any spans related to the database calls? Also might be worth to check the startup logs where we install all SDK integrations. Do you see any errors or warnings?

Another idea: Are you initializing the SDK as early as possible in your application lifecycle? I suggest calling Sentry.init before you require mongoose/express.

I still think that this is probably already fixed in v8 so (assuming there's a bug) I don't think we'll get to fix this for v7 in the immediate future. If you want to get a headstart on updating, our latest v8 alpha is fairly stable by now and we don't expect breaking changes in a conventional SDK setup anymore. However, I can ofc understand that updating to an alpha is scary, so no worries :)

@jennyunh
Copy link
Author

jennyunh commented Apr 12, 2024

Upon turning on debug: true, the logs indicate that no db spans were detected for the transaction that I am testing (which has db calls and should have db spans). Also checked that there are no errors/warnings in the startup logs where we install the SDK integrations.
I've also tried initializing the SDK at various points in the app lifecycle, including before mongoose/express.

If this is a bug in v7, then I will try upgrading to v8 after its release at a later time.
When will v8 be officially released?
Thank you!

@Lms24
Copy link
Member

Lms24 commented Apr 15, 2024

We'll get a beta build out this week. After that our main focus is ensuring data quality so it'd actually be super helpful for us if you could give the beta a try and report if you now get the missing DB spans. Depending on community feedback around bugs/quality, the beta period can be shorter or longer. Can't give you an ETA on a stable release, yet.

@mydea
Copy link
Member

mydea commented May 15, 2024

We have v8.0.0 out, so you can try it out in the stable version today!

@mydea mydea removed this from the 8.0.0 milestone May 15, 2024
@richardsimko
Copy link
Contributor

What version of mongoose are you running? We're seeing the same issue with Sentry v8 since opentelemetry doesn't support mongoose v7/v8 and since Sentry relies fully on that in Sentry v8 (From my understanding) this breaks the mongoose integration for anyone running a mildly modern version of Mongoose.

@mydea
Copy link
Member

mydea commented May 17, 2024

Out of curiosity, did mongoose v7/v8 work in Sentry v7?

You did the best thing to move this forward already, which is to leave a comment and show interest upstream in open-telemetry/opentelemetry-js-contrib#1606 - we'll also keep an eye on this, maybe we can help bringing support for newer mongoose versions too!

@richardsimko
Copy link
Contributor

richardsimko commented May 17, 2024

You're right, it didn't! When I check again the spans we're seeing when using 7.x are from the mongo integration. These have disappeared in 8.x. I'll raise a separate issue for that.

Edit: I raised open-telemetry/opentelemetry-js-contrib#2218 to get some clarity into how to get the mongodb integration to work.

Edit2: Actually when I take a second look at the screenshot in the issue here I see the same thing we're experiencing, db transactions were present before but no longer so it's possible that @jennyunh is seeing the same issue related to the mongodb integration and not mongoose.

You can see which integration it comes from by clicking one of the spans and checking the right panel for Origin:

image

@mydea
Copy link
Member

mydea commented May 17, 2024

Great findings! So I guess more stuff to fix upstream 😅 We're putting this on our agenda to try to get these things fixed/updated in opentelemetry 🤞

@mydea mydea changed the title Node SDK: Some transactions are not showing db queries Mongoose instrumentation does not work for Mongoose >= 7 May 17, 2024
@richardsimko
Copy link
Contributor

Great to hear! Do you want me to create a separate issue for the mongo problem or are you tracking both of those with this one or should I wait until I get a reply in open-telemetry/opentelemetry-js-contrib#2218 ?

@mydea
Copy link
Member

mydea commented May 21, 2024

Great to hear! Do you want me to create a separate issue for the mongo problem or are you tracking both of those with this one or should I wait until I get a reply in open-telemetry/opentelemetry-js-contrib#2218 ?

Feel free to open a separate issue, does not hurt to track this here as well - thank you!

@AbhiPrasad
Copy link
Member

As an update - @onurtemizkan opened a PR for this here: open-telemetry/opentelemetry-js-contrib#2353

we are waiting on reviews.

@AbhiPrasad
Copy link
Member

open-telemetry/opentelemetry-js-contrib@770130a

Once we update the dep, we need to update the docs for supported versions.

mydea added a commit that referenced this issue Sep 5, 2024
…13587)

This bumps all our internal OTEL instrumentation to their latest
version.

Mainly, this fixes two things:

* Mongoose now supports v7 & v8
open-telemetry/opentelemetry-js-contrib#2353
* A variety of bug fixes, including a problem with http.get in ESM mode
open-telemetry/opentelemetry-js#4857 - See:
https:/open-telemetry/opentelemetry-js/releases/tag/experimental%2Fv0.53.0

Related:

* open-telemetry/opentelemetry-js#4975 Issue
about relaxing deps in "core" instrumentation packages
* PR to bump deps in `@prisma/instrumentation`:
prisma/prisma#25160
* PR to bump deps in `opentelemetry-instrument-remix`:
justindsmith/opentelemetry-instrumentations-js#52
* PR to bump deps in `opentelemetry-instrumentation-fetch-node`:
gas-buddy/opentelemetry-instrumentation-fetch-node#17
(which will also be superseded by
#13485 eventually)

* Closes #11499
@AbhiPrasad
Copy link
Member

This has been released with https:/getsentry/sentry-javascript/releases/tag/8.29.0, the sdk now supports mongoose v8!

@SteffenLanger
Copy link

Unfortunately, I'm still seeing this error with Sentry 8.30.0. I created a new issue with a GitHub repo to reproduce the issue.

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

Successfully merging a pull request may close this issue.

7 participants