-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
fix(nextjs): Avoid sending http.client transactions for Sentry fetch requests on edge & dev env #16772
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
Closed
Closed
fix(nextjs): Avoid sending http.client transactions for Sentry fetch requests on edge & dev env #16772
Changes from all commits
Commits
Show all changes
7 commits
Select commit
Hold shift + click to select a range
b96fd7f
WIP: nextjs http.client spans
mydea 3cab755
fix by looking at url :(
mydea d80c681
use event processor instead, bandaid!
mydea 5dc90f8
fix linting
mydea 6efbca3
fix linting
mydea 8373f6d
more reliable test
mydea 188a8ff
fix test
mydea File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
61 changes: 0 additions & 61 deletions
61
dev-packages/e2e-tests/test-applications/nextjs-app-dir/tests/edge-route.test.ts
This file was deleted.
Oops, something went wrong.
130 changes: 130 additions & 0 deletions
130
...packages/e2e-tests/test-applications/nextjs-app-dir/tests/pages-router-edge-route.test.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,130 @@ | ||
import { expect, test } from '@playwright/test'; | ||
import { waitForError, waitForTransaction } from '@sentry-internal/test-utils'; | ||
|
||
test('Should create a transaction for edge routes', async ({ request }) => { | ||
const edgerouteTransactionPromise = waitForTransaction('nextjs-app-dir', async transactionEvent => { | ||
return ( | ||
transactionEvent?.transaction === 'GET /api/edge-endpoint' && | ||
transactionEvent.contexts?.runtime?.name === 'vercel-edge' | ||
); | ||
}); | ||
|
||
const response = await request.get('/api/edge-endpoint', { | ||
headers: { | ||
'x-yeet': 'test-value', | ||
}, | ||
}); | ||
expect(await response.json()).toStrictEqual({ name: 'Jim Halpert' }); | ||
|
||
const edgerouteTransaction = await edgerouteTransactionPromise; | ||
|
||
expect(edgerouteTransaction.contexts?.trace?.status).toBe('ok'); | ||
expect(edgerouteTransaction.contexts?.trace?.op).toBe('http.server'); | ||
expect(edgerouteTransaction.request?.headers?.['x-yeet']).toBe('test-value'); | ||
}); | ||
|
||
test('Faulty edge routes', async ({ request }) => { | ||
const edgerouteTransactionPromise = waitForTransaction('nextjs-app-dir', async transactionEvent => { | ||
return ( | ||
transactionEvent?.transaction === 'GET /api/error-edge-endpoint' && | ||
transactionEvent.contexts?.runtime?.name === 'vercel-edge' | ||
); | ||
}); | ||
|
||
const errorEventPromise = waitForError('nextjs-app-dir', errorEvent => { | ||
return ( | ||
errorEvent?.exception?.values?.[0]?.value === 'Edge Route Error' && | ||
errorEvent.contexts?.runtime?.name === 'vercel-edge' | ||
); | ||
}); | ||
|
||
request.get('/api/error-edge-endpoint').catch(() => { | ||
// Noop | ||
}); | ||
|
||
const [edgerouteTransaction, errorEvent] = await Promise.all([ | ||
test.step('should create a transaction', () => edgerouteTransactionPromise), | ||
test.step('should create an error event', () => errorEventPromise), | ||
]); | ||
|
||
test.step('should create transactions with the right fields', () => { | ||
expect(edgerouteTransaction.contexts?.trace?.status).toBe('unknown_error'); | ||
expect(edgerouteTransaction.contexts?.trace?.op).toBe('http.server'); | ||
}); | ||
|
||
test.step('should have scope isolation', () => { | ||
expect(edgerouteTransaction.tags?.['my-isolated-tag']).toBe(true); | ||
expect(edgerouteTransaction.tags?.['my-global-scope-isolated-tag']).not.toBeDefined(); | ||
expect(errorEvent.tags?.['my-isolated-tag']).toBe(true); | ||
expect(errorEvent.tags?.['my-global-scope-isolated-tag']).not.toBeDefined(); | ||
}); | ||
}); | ||
|
||
test('Should not create spans for outgoing Sentry requests on edge routes', async ({ request }) => { | ||
// Ensure no http.client transaction is created for any orphaned request | ||
waitForTransaction('nextjs-app-dir', async transactionEvent => { | ||
if (transactionEvent.contexts?.trace?.op === 'http.client') { | ||
throw new Error(`Should not receive http.client transaction, but got: ${transactionEvent.transaction}`); | ||
} | ||
return false; | ||
}); | ||
|
||
// We hit the endpoint three times and check that nowhere a http.client span for Sentry is to be found | ||
// this way, we ensure that nothing is sent to Sentry in a follow up span | ||
const edgerouteTransactionPromise1 = waitForTransaction('nextjs-app-dir', async transactionEvent => { | ||
return ( | ||
transactionEvent?.transaction === 'GET /api/edge-endpoint' && | ||
transactionEvent.contexts?.runtime?.name === 'vercel-edge' | ||
); | ||
}); | ||
|
||
await request.get('/api/edge-endpoint', { | ||
headers: { | ||
'x-yeet': 'test-value', | ||
}, | ||
}); | ||
|
||
const edgerouteTransactionPromise2 = waitForTransaction('nextjs-app-dir', async transactionEvent => { | ||
return ( | ||
transactionEvent?.transaction === 'GET /api/edge-endpoint' && | ||
transactionEvent.contexts?.runtime?.name === 'vercel-edge' | ||
); | ||
}); | ||
|
||
await request.get('/api/edge-endpoint', { | ||
headers: { | ||
'x-yeet': 'test-value-2', | ||
}, | ||
}); | ||
|
||
const edgerouteTransactionPromise3 = waitForTransaction('nextjs-app-dir', async transactionEvent => { | ||
return ( | ||
transactionEvent?.transaction === 'GET /api/edge-endpoint' && | ||
transactionEvent.contexts?.runtime?.name === 'vercel-edge' | ||
); | ||
}); | ||
|
||
await request.get('/api/edge-endpoint', { | ||
headers: { | ||
'x-yeet': 'test-value-3', | ||
}, | ||
}); | ||
|
||
const [edgerouteTransaction1, edgerouteTransaction2, edgerouteTransaction3] = await Promise.all([ | ||
edgerouteTransactionPromise1, | ||
edgerouteTransactionPromise2, | ||
edgerouteTransactionPromise3, | ||
]); | ||
|
||
expect(edgerouteTransaction1.contexts?.trace?.op).toBe('http.server'); | ||
expect(edgerouteTransaction2.contexts?.trace?.op).toBe('http.server'); | ||
expect(edgerouteTransaction3.contexts?.trace?.op).toBe('http.server'); | ||
|
||
expect(edgerouteTransaction1.spans?.length).toBe(1); | ||
expect(edgerouteTransaction2.spans?.length).toBe(1); | ||
expect(edgerouteTransaction3.spans?.length).toBe(1); | ||
|
||
expect(edgerouteTransaction1.spans?.[0].description).toBe('handler (/api/edge-endpoint)'); | ||
expect(edgerouteTransaction2.spans?.[0].description).toBe('handler (/api/edge-endpoint)'); | ||
expect(edgerouteTransaction3.spans?.[0].description).toBe('handler (/api/edge-endpoint)'); | ||
}); |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
A bit unrelated to this PR, is there a reason why we are returning the event in an
else
block?Wouldn't it make more sense to remove the
else
and return the event after theif
statement?