-
-
Notifications
You must be signed in to change notification settings - Fork 1.8k
fix(nextjs): Treat static route span names as low-cardinality names #23503
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -15,22 +15,40 @@ test('should create a parameterized streamed span when the `app` directory is us | |
| expect(span.attributes['sentry.source']?.value).toBe('route'); | ||
| }); | ||
|
|
||
| test('should create a static streamed span when the `app` directory is used and the route is not parameterized', async ({ | ||
| test('should create a streamed span named after the static route when the `app` directory is used', async ({ | ||
| page, | ||
| }) => { | ||
| const spanPromise = waitForStreamedSpan('nextjs-16-streaming', span => { | ||
| return span.name === 'Pageload' && getSpanOp(span) === 'pageload' && span.is_segment; | ||
| return span.name === '/parameterized/static' && getSpanOp(span) === 'pageload' && span.is_segment; | ||
| }); | ||
|
|
||
| await page.goto(`/parameterized/static`); | ||
|
|
||
| const span = await spanPromise; | ||
|
|
||
| expect(span.name).toBe('/parameterized/static'); | ||
| expect(span.trace_id).toMatch(/[a-f0-9]{32}/); | ||
| expect(span.attributes).toMatchObject({ | ||
| ['sentry.segment.name.source']: { value: 'route', type: 'string' }, | ||
| ['url.template']: { value: '/parameterized/static', type: 'string' }, | ||
| ['url.path']: { value: '/parameterized/static', type: 'string' }, | ||
| }); | ||
| }); | ||
|
|
||
| test('should fall back to a low cardinality span name for routes the manifest does not know', async ({ page }) => { | ||
| const spanPromise = waitForStreamedSpan('nextjs-16-streaming', span => { | ||
| return span.name === 'Pageload' && getSpanOp(span) === 'pageload' && span.is_segment; | ||
| }); | ||
|
|
||
| await page.goto('/this-route-does-not-exist'); | ||
|
|
||
| const span = await spanPromise; | ||
|
|
||
| expect(span.name).toBe('Pageload'); | ||
| expect(span.trace_id).toMatch(/[a-f0-9]{32}/); | ||
| expect(span.attributes).toMatchObject({ | ||
| ['sentry.segment.name.source']: { value: 'url', type: 'string' }, | ||
| ['url.path']: { value: '/parameterized/static', type: 'string' }, | ||
| ['url.path']: { value: '/this-route-does-not-exist', type: 'string' }, | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Missing negative template assertionLow Severity Flagged because the PR review guidelines call out relaxed matchers when a payload field is expected to be absent. This new unknown-route fallback test uses Triggered by project rule: PR Review Guidelines for Cursor Bot Reviewed by Cursor Bugbot for commit b6f7b8f. Configure here. |
||
| }); | ||
| }); | ||
|
|
||
|
|
||


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.
Incomplete static route assertions
Low Severity
Flagged because the PR review guidelines ask to assert newly added payload data thoroughly. This test updates
sentry.segment.name.sourcetoroutefor the static/pageload-tracingpageload, but does not asserturl.template, which sibling updates in this PR (for example the cacheComponents and parameterized static tests) do cover.Triggered by project rule: PR Review Guidelines for Cursor Bot
Reviewed by Cursor Bugbot for commit b6f7b8f. Configure here.