Skip to content

feat(nexjs): Sample out low-quality spans on older Next.js versions #11722

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

Merged
merged 8 commits into from
Apr 24, 2024

Conversation

lforst
Copy link
Contributor

@lforst lforst commented Apr 22, 2024

Extends the sampling logic for bad Next.js spans ot account for older Next.js versions where we don't have the next.span_type attribute.

Relates to https://github.com/getsentry/sentry-javascript-examples/pull/21/files#r1574816010

Copy link
Contributor

github-actions bot commented Apr 22, 2024

size-limit report 📦

Path Size
@sentry/browser 21.65 KB (0%)
@sentry/browser (incl. Tracing) 32.63 KB (0%)
@sentry/browser (incl. Tracing, Replay) 67.98 KB (0%)
@sentry/browser (incl. Tracing, Replay) - with treeshaking flags 61.37 KB (0%)
@sentry/browser (incl. Tracing, Replay with Canvas) 72.02 KB (0%)
@sentry/browser (incl. Tracing, Replay, Feedback) 84.21 KB (0%)
@sentry/browser (incl. Feedback) 37.76 KB (0%)
@sentry/browser (incl. sendFeedback) 26.43 KB (0%)
@sentry/browser (incl. FeedbackAsync) 30.91 KB (0%)
@sentry/react 24.33 KB (0%)
@sentry/react (incl. Tracing) 35.53 KB (0%)
@sentry/vue 25.27 KB (0%)
@sentry/vue (incl. Tracing) 34.38 KB (0%)
@sentry/svelte 21.77 KB (0%)
CDN Bundle 23.95 KB (0%)
CDN Bundle (incl. Tracing) 33.89 KB (0%)
CDN Bundle (incl. Tracing, Replay) 67.61 KB (0%)
CDN Bundle (incl. Tracing, Replay, Feedback) 83.44 KB (0%)
CDN Bundle - uncompressed 70.56 KB (0%)
CDN Bundle (incl. Tracing) - uncompressed 100.57 KB (0%)
CDN Bundle (incl. Tracing, Replay) - uncompressed 210.18 KB (0%)
CDN Bundle (incl. Tracing, Replay, Feedback) - uncompressed 256.49 KB (0%)
@sentry/nextjs (client) 34.75 KB (0%)
@sentry/sveltekit (client) 33.13 KB (0%)
@sentry/node 152.87 KB (+0.03% 🔺)

@lforst lforst requested a review from mydea April 22, 2024 14:57
@lforst
Copy link
Contributor Author

lforst commented Apr 22, 2024

Why the hell is this crashing the server?

@@ -68,7 +84,10 @@ export class SentrySampler implements Sampler {
// The reason for this is that the data quality of the spans varies, it is different per version of Next,
// and we need to keep our manual instrumentation around for the edge runtime anyhow.
// BUT we only do this if we don't have a parent span with a sampling decision yet (or if the parent is remote)
if (spanAttributes['next.span_type'] && (typeof parentSampled !== 'boolean' || parentContext?.isRemote)) {
if (
(spanAttributes['next.span_type'] || NEXTJS_SPAN_NAME_PREFIXES.includes(spanName.split('.')[0])) &&
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm this souns a bit like it could lead to false positives 😬 is there no other attribute type we can check for (e.g. next.xxx)? 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that's the cost we have to pay if we have this so central in the opentelemetry package. Imo would it be isolated to the nextjs sdk it's probably fine but youre right that here its probably too risky.

@lforst lforst merged commit 92bc6c6 into develop Apr 24, 2024
96 checks passed
@lforst lforst deleted the lforst-filter-old-nextjs-spans branch April 24, 2024 10:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants