feat(nuxt): Add server error hook #12796
Conversation
|
|
||
| if (context) { | ||
| captureException(error, { | ||
| captureContext: { contexts: { nuxt: context } }, |
There was a problem hiding this comment.
m: I would be more selective in what we record here instead of dumping the entire context object in here. It could become a problem if context suddenly starts containing PII, becomes very large, or circular in a future nitro update.
There was a problem hiding this comment.
yes ideally we:
- We parse the incoming nuxt context and extract the fields we care about
- We put that in a nuxt context attached to the sentry event
- Document the nuxt context in the develop docs
| // Do not handle 404 and 422 | ||
| if (error instanceof H3Error) { | ||
| if (error.statusCode === 404 || error.statusCode === 422) { | ||
| return; | ||
| } | ||
| } |
There was a problem hiding this comment.
l/m: We could arguably skip all 4xxs (also 3xxs if that can ever happen).
Lms24
left a comment
Lms24
left a comment
There was a problem hiding this comment.
Two optional suggestion to streamline the code but otherwise LGTM! (feel free to merge and streamline in a follow-up PR or not at all)
| if (errorContext) { | ||
| const structuredContext = extractErrorContext(errorContext); | ||
|
|
||
| captureException(error, { | ||
| captureContext: { contexts: { nuxt: structuredContext } }, | ||
| mechanism: { handled: false }, | ||
| }); | ||
| } else { | ||
| captureException(error, { mechanism: { handled: false } }); | ||
| } |
There was a problem hiding this comment.
l (feel free to disregard): WDYT about calling captureException only once and setting captureContext depending on errorContext
| tags: undefined, | ||
| }; | ||
|
|
||
| if (errorContext) { |
There was a problem hiding this comment.
l: do we need this guard? i.e. can CapturedErrorContext be undefined?
Preparing `@sentry/nuxt` for a release (experimental) Merge after this PR: #12796
Reports errors thrown in nitro. Tests will be added when adding the E2E test application.
closes #12795