-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
feat(nextjs): Use spans generated by Next.js for App Router #12729
Conversation
This is ready for review, just need to figure out why 1 e2e test is failing. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pretty cool!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! Does this have any effect on page routes as well?
size-limit report 📦
|
8c2fe86
to
84d9bd7
Compare
}, | ||
{ | ||
// This comes with the risk of tests leaking into each other but the tests run quite slow so we should parallelize | ||
workers: '100%', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤞
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
my thought was: If the tests leak, the tests are bad anyways.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
packages/nextjs/src/server/index.ts
Outdated
} | ||
}); | ||
|
||
client?.on('spanEnd'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
client?.on('spanEnd'); |
I guess this is a leftover? 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! 🚀
Previously when we attempted to use the OTEL tracing provided by Next.js we faced data quality issues. The problem was that we used to look at the data holistically and not in isolation. We discovered that certain data provided by Next.js is actually more accurate than the data we collect, in particular the data we collect for app router SSR requests.
With this PR we start using OTEL data for app router SSR requests. We do this by allow-listing spans created for these requests by Next.js so that we are only capturing these spans/traces.
With this PR, we're starting to use our/otel's HTTP instrumentation (only for app router SSR requests!) so we can capture the most accurate data.
There are a few gotchas:
Traces look like this now: