sentry-javascript icon indicating copy to clipboard operation
sentry-javascript copied to clipboard

NextJS doesn't wrap API routes so DB Spans are only attached to root transaction

Open silent1mezzo opened this issue 2 years ago • 5 comments

Is there an existing issue for this?

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

How do you use Sentry?

Sentry Saas (sentry.io)

Which package are you using?

@sentry/nextjs

SDK Version

7.13

Framework Version

12.3

Link to Sentry event

https://sentry.io/organizations/adam-bl/performance/nextjs:9b1bb4037b9f4df8ab087b6521493df0/?project=6761253&query=&showTransactions=recent&statsPeriod=24h&transaction=GET+%2Fapi%2Freports&unselectedSeries=p100%28%29

Steps to Reproduce

  1. Build an API endpoint in Nextjs
  2. Use Prisma to load data
  3. Look at span in Sentry
// /api/reports.tsx
const handler = async (req, res) => {
    console.log("here");
    let reports = await prisma.report.findMany({
        orderBy: { submittedDate: 'desc' }
      })

    for (const report of reports) {
      const expenses = await prisma.expense.findMany({
        where: { reportId: report.id }
      })
    }
    res.status(200).json({ reports })
}

export default withSentry(handler);

Expected Result

DB spans would be nested under an API route

transaction /api/reports db.prisma db.prisma db.prisma db.prisma ...

Actual Result

Screen Shot 2022-09-20 at 7 12 43 AM

silent1mezzo avatar Sep 20 '22 11:09 silent1mezzo

Looks like https://github.com/getsentry/sentry-javascript/pull/5778 will solve this?

silent1mezzo avatar Sep 20 '22 11:09 silent1mezzo

So at the moment, all the spans are associated to the transaction - the parent child relationship does not hold. This is because we didn't set the span on the scope properly in the integration, will fix!

AbhiPrasad avatar Sep 20 '22 11:09 AbhiPrasad

Alright so little more complicated than we thought - so putting it back into the backlog. Good thing though is that your ask has triggered a lot of convos about parent-child relationships and scope management, so perhaps we do have a more wholistic solutions

NextJS with Prisma won't create n+1 by default for a simple case. It ties all of the DB spans to parent transaction.

This is because each query -> separate span, and we don't have any kind of "db" wrapping span that puts them all together.

For example, let's take a look at your code here.

let reports = await prisma.report.findMany({
  orderBy: { submittedDate: 'desc' }
})

for (const report of reports) {
  const expenses = await prisma.expense.findMany({
    where: { reportId: report.id }
  })
}

each call to prisma is going to be it's own database span, but there's no way we can associate the prisma.report.findMany to the prisma.expense.findMany calls, since we don't instrument a db connection or something like that.

- transaction T
  - span A started
  - prisma.report.findMany
  - span A ended
  - span B started
  - prisma.expense.findMany
  - span B ended
  - span C started
  - prisma.expense.findMany
  - span C ended

We can't make the guarantee that span A and span B, C, D, ... are related - therefore they just have to be children to the transaction. Doesn't the perf detector still work here though? It'll recognize that span B, C, D are duplicates, and see that span A is not.

AbhiPrasad avatar Sep 20 '22 13:09 AbhiPrasad

@AbhiPrasad so for what it's worth, for n+1 detection we don't actually need the spans associated to one another, we just need them under something other than the root transaction. So if we auto-instrumented the handler here and nested the db calls (report.findMany and expense.findMany) under that API handler that would be enough

That would be more accurate too. Right now when looking at the span tree we're missing details that these spans happen in the API handler.

silent1mezzo avatar Sep 20 '22 13:09 silent1mezzo

@silent1mezzo - I think we may be talking at slightly cross-purposes here. The API route is being instrumented. That's what withSentry does. (All the PR you linked does is keep you from having to manually wrap every API route with withSentry; otherwise, there's no behavior change from today.) But that doesn't mean creating a span inside the transaction - it is the transaction.

(Perhaps I'm mistaken, but I would have said that that’s what it's like in other backend SDKs, too - each incoming request is one transaction. No?)

lobsterkatie avatar Sep 21 '22 04:09 lobsterkatie