-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
feat(node): Capture SystemError
context and remove paths from message
#17331
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
base: develop
Are you sure you want to change the base?
Conversation
for (const exception of event.exception?.values || []) { | ||
if (exception.value) { | ||
if (error.path && exception.value.includes(error.path)) { | ||
exception.value = exception.value.replace(`'${error.path}'`, '').trim(); |
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.
I wonder if we should expose an integration option to control this replacement as well
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.
Ideally I would have used util.getSystemErrorMessage(err)
to get the plain message without the paths but it was only added in Node v22
@@ -32,6 +32,7 @@ import { onUncaughtExceptionIntegration } from '../integrations/onuncaughtexcept | |||
import { onUnhandledRejectionIntegration } from '../integrations/onunhandledrejection'; | |||
import { processSessionIntegration } from '../integrations/processSession'; | |||
import { INTEGRATION_NAME as SPOTLIGHT_INTEGRATION_NAME, spotlightIntegration } from '../integrations/spotlight'; | |||
import { systemErrorIntegration } from '../integrations/systemError'; |
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.
we'll have to export this from all of our node packages as well
size-limit report 📦
|
SystemError
context and remove paths from messageSystemError
context and remove paths from message
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.
Think we are good to go after we fix bun
…om:getsentry/sentry-javascript into timfish/feat/node-system-error-integration
resolves #17332
Node.js has a
SystemError
error class.This error type contains numerous properties which can be useful for debugging but it also includes many of them in the error message which stops fingerprinting from grouping these errors.
This PR adds a default enabled integration that copies the error properties to a new context and strips paths from the error message.
If the top-level
sendDefaultPii
is not enabled, thepath
anddest
properties will not be included.