-
-
Notifications
You must be signed in to change notification settings - Fork 1.8k
fix(nextjs): Return correct lastEventId for SSR pages #19240
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 4 commits
6cd283a
f042dea
6136923
e880b8a
9df573f
e6145b1
a9c7ed1
eb4c6b5
f93f052
83d8222
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 |
|---|---|---|
| @@ -0,0 +1,36 @@ | ||
| import type { NextPageContext } from 'next'; | ||
| import * as Sentry from '@sentry/nextjs'; | ||
|
|
||
| interface ErrorProps { | ||
| statusCode?: number; | ||
| eventId?: string; | ||
| lastEventId?: string; | ||
| } | ||
|
|
||
| function ErrorPage({ statusCode, eventId, lastEventId }: ErrorProps) { | ||
| return ( | ||
| <div> | ||
| <h1>Error Page</h1> | ||
| <p data-testid="status-code">Status Code: {statusCode}</p> | ||
| <p data-testid="event-id">Event ID from return: {eventId || 'No event ID'}</p> | ||
| <p data-testid="last-event-id">Event ID from lastEventId(): {lastEventId || 'No event ID'}</p> | ||
| </div> | ||
| ); | ||
| } | ||
|
|
||
| ErrorPage.getInitialProps = async (context: NextPageContext) => { | ||
| const { res, err } = context; | ||
|
|
||
| const statusCode = res?.statusCode || err?.statusCode || 404; | ||
|
|
||
| // Capture the error using captureUnderscoreErrorException | ||
| // This should return the already-captured event ID from the data fetcher | ||
| const eventId = await Sentry.captureUnderscoreErrorException(context); | ||
|
|
||
| // Also get the last event ID from lastEventId() | ||
| const lastEventId = Sentry.lastEventId(); | ||
|
|
||
| return { statusCode, eventId, lastEventId }; | ||
| }; | ||
|
|
||
| export default ErrorPage; |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,7 @@ | ||
| export default function TestErrorPage() { | ||
| return <div>This page should never render</div>; | ||
| } | ||
|
|
||
| export function getServerSideProps() { | ||
| throw new Error('Test error to trigger _error.tsx page'); | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,37 @@ | ||
| import { expect, test } from '@playwright/test'; | ||
| import { waitForError } from '@sentry-internal/test-utils'; | ||
|
|
||
| test('lastEventId() should return the event ID after captureUnderscoreErrorException', async ({ page }) => { | ||
| test.skip(!process.env.TEST_ENV?.includes('development'), 'should be skipped for non-dev mode'); | ||
|
|
||
| const errorEventPromise = waitForError('nextjs-pages-dir', errorEvent => { | ||
| return errorEvent?.exception?.values?.[0]?.value === 'Test error to trigger _error.tsx page'; | ||
| }); | ||
|
|
||
| await page.goto('/underscore-error/test-error-page'); | ||
| const errorEvent = await errorEventPromise; | ||
|
|
||
| console.log('errorEvent.exception?.values?.[0]?.mechanism', errorEvent.exception?.values?.[0]?.mechanism); | ||
|
|
||
| // Since the error is already captured by withErrorInstrumentation in getServerSideProps, | ||
| // the mechanism should be 'auto.function.nextjs.wrapped', not 'auto.function.nextjs.underscore_error' | ||
| expect(errorEvent.exception?.values?.[0]?.mechanism?.type).toBe('auto.function.nextjs.wrapped'); | ||
| // The function name might be e.g. 'getServerSideProps$1' | ||
| expect(errorEvent.exception?.values?.[0]?.mechanism?.data?.function).toContain('getServerSideProps'); | ||
| expect(errorEvent.exception?.values?.[0]?.mechanism?.handled).toBe(false); | ||
|
|
||
| const eventIdFromReturn = await page.locator('[data-testid="event-id"]').textContent(); | ||
| const returnedEventId = eventIdFromReturn?.replace('Event ID from return: ', ''); | ||
|
|
||
| const lastEventIdFromFunction = await page.locator('[data-testid="last-event-id"]').textContent(); | ||
| const lastEventId = lastEventIdFromFunction?.replace('Event ID from lastEventId(): ', ''); | ||
|
|
||
| expect(returnedEventId).toBeDefined(); | ||
| expect(returnedEventId).not.toBe('No event ID'); | ||
| expect(lastEventId).toBeDefined(); | ||
| expect(lastEventId).not.toBe('No event ID'); | ||
|
|
||
| expect(lastEventId).toBe(returnedEventId); | ||
| expect(errorEvent.event_id).toBe(returnedEventId); | ||
| expect(errorEvent.event_id).toBe(lastEventId); | ||
| }); | ||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -1,4 +1,10 @@ | ||||||||||||||||||||||||||||||||||||||||||||||
| import { captureException, httpRequestToRequestData, withScope } from '@sentry/core'; | ||||||||||||||||||||||||||||||||||||||||||||||
| import { | ||||||||||||||||||||||||||||||||||||||||||||||
| captureException, | ||||||||||||||||||||||||||||||||||||||||||||||
| getIsolationScope, | ||||||||||||||||||||||||||||||||||||||||||||||
| checkOrSetAlreadyCaught, | ||||||||||||||||||||||||||||||||||||||||||||||
| httpRequestToRequestData, | ||||||||||||||||||||||||||||||||||||||||||||||
| withScope, | ||||||||||||||||||||||||||||||||||||||||||||||
| } from '@sentry/core'; | ||||||||||||||||||||||||||||||||||||||||||||||
| import type { NextPageContext } from 'next'; | ||||||||||||||||||||||||||||||||||||||||||||||
| import { flushSafelyWithTimeout, waitUntil } from '../utils/responseEnd'; | ||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -38,6 +44,13 @@ export async function captureUnderscoreErrorException(contextOrProps: ContextOrP | |||||||||||||||||||||||||||||||||||||||||||||
| return; | ||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||
| // If the error was already captured (e.g., by wrapped functions in data fetchers), | ||||||||||||||||||||||||||||||||||||||||||||||
| // return the existing event ID instead of capturing it again (needed for lastEventId() to work) | ||||||||||||||||||||||||||||||||||||||||||||||
| if (err && checkOrSetAlreadyCaught(err)) { | ||||||||||||||||||||||||||||||||||||||||||||||
|
Member
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. This modifies the error object already before sending it, which we do not want bc this prevents it from capturing it?
Member
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. probably worth exposing |
||||||||||||||||||||||||||||||||||||||||||||||
| waitUntil(flushSafelyWithTimeout()); | ||||||||||||||||||||||||||||||||||||||||||||||
| return getIsolationScope().lastEventId(); | ||||||||||||||||||||||||||||||||||||||||||||||
|
Member
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. I feel like this has a high chance of not being the event id we are looking for... we really should think about storing lastEventId with more info of what the actual event was that this belongs to. I don't really think there's much we can do here, just wanted to mention this tho and this is probably better than straight up return an event id of an event that gets deduped.
Member
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. I think this can work but doesn't feel bullet proof to me. Could we perhaps associate
Member
Author
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. @logaretm that sounds also like a nice approach. But do you mean that independently from
Member
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. What I was thinking here is "how do we know for sure that the very last event is the same one that caught the same error?" I can't say I'm familiar with how this all works yet, so I was wondering if this is an edge case to consider, is it possible for events to slip in between? are they guaranteed to be sequential? What I was suggesting is if the error was caught, we stick an event ID on it if possible, that way if it comes up again then we can use the last event assigned to that error instance. Does that track or is it maybe naive?
Member
Author
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. It can happen that events slip in between (this is also documented in sentry-javascript/packages/core/src/exports.ts Lines 123 to 136 in c5e3249
For example (this was relevant in the case of this PR), sentry-javascript/packages/core/src/client.ts Lines 278 to 285 in b7f8cfe
And this was overriding the event ID.
Do you mean that instead of returning a new event ID, we re-use the same one? |
||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||
|
cursor[bot] marked this conversation as resolved.
|
||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||
| const eventId = withScope(scope => { | ||||||||||||||||||||||||||||||||||||||||||||||
| if (req) { | ||||||||||||||||||||||||||||||||||||||||||||||
| const normalizedRequest = httpRequestToRequestData(req); | ||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||
Uh oh!
There was an error while loading. Please reload this page.