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

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

Closed
3 tasks done
silent1mezzo opened this issue Sep 20, 2022 · 5 comments
Closed
3 tasks done
Labels
Package: nextjs Issues related to the Sentry Nextjs SDK Type: Bug

Comments

@silent1mezzo
Copy link

Is there an existing issue for this?

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
Copy link
Author

Looks like #5778 will solve this?

@AbhiPrasad AbhiPrasad self-assigned this Sep 20, 2022
@AbhiPrasad
Copy link
Member

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
Copy link
Member

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.

@silent1mezzo
Copy link
Author

silent1mezzo commented Sep 20, 2022

@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.

@lobsterkatie
Copy link
Member

@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?)

@AbhiPrasad AbhiPrasad removed their assignment Dec 6, 2022
@HazAT HazAT added the Package: nextjs Issues related to the Sentry Nextjs SDK label Jan 26, 2023
@HazAT HazAT closed this as completed Jan 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Package: nextjs Issues related to the Sentry Nextjs SDK Type: Bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants