Skip to content
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

feat(integrations): Capture error cause with captureErrorCause in ExtraErrorData integration #9914

Merged
merged 6 commits into from
Dec 22, 2023

Conversation

lforst
Copy link
Member

@lforst lforst commented Dec 19, 2023

Fixes #9913

We currently have no proper way of capturing error causes in the SDK when they're not errors but serializable objects. In theory the spec allows for this so we should support it: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Error/cause#providing_structured_data_as_the_error_cause

@jeengbe
Copy link
Contributor

jeengbe commented Dec 19, 2023

Why is this in the ExtraErrorData integration, should that not be part of the default behaviour?

function _enhanceEventWithErrorData(event: Event, hint: EventHint = {}, depth: number): Event {
function _enhanceEventWithErrorData(
event: Event,
hint: EventHint = {},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
hint: EventHint = {},
hint: EventHint | undefined = {},

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Optional values should be at the end of the arguments list. I thought we had a rule for this?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The parameter is not optional. It just takes undefined and falls back to {}. Optional parameters are marked with a ? like this hint?: EventHint. Additionally, your suggestion doesn't do anything because TS already knows to allow undefined as type for parameters with default.

@@ -117,6 +125,12 @@ function _extractErrorData(error: ExtendedError): Record<string, unknown> | null
extraErrorInfo[key] = isError(value) ? value.toString() : value;
}

// Error.cause is a standard property that is non enumerable, we therefore need to access it separately.
// https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Error/cause
if (captureErrorCause && error['cause']) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if (captureErrorCause && error['cause']) {
if (captureErrorCause && error.hasOwnProperty('cause')) {

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We cannot do this because cause is a prototype (? or class field, or getter idk) and hasOwnProperty will return false even though it is there.

packages/integrations/src/extraerrordata.ts Outdated Show resolved Hide resolved
packages/integrations/test/extraerrordata.test.ts Outdated Show resolved Hide resolved
@lforst
Copy link
Member Author

lforst commented Dec 19, 2023

Why is this in the ExtraErrorData integration, should that not be part of the default behaviour?

Based on patterns we have already established, likely no.

@lforst lforst requested review from mydea and Lms24 December 22, 2023 10:56
packages/integrations/src/extraerrordata.ts Outdated Show resolved Hide resolved
lforst and others added 2 commits December 22, 2023 12:17
Co-authored-by: Lukas Stracke <lukas.stracke@sentry.io>
@lforst lforst merged commit cbdba10 into develop Dec 22, 2023
96 checks passed
@lforst lforst deleted the lforst-capture-error-cause branch December 22, 2023 12:21
anonrig pushed a commit that referenced this pull request Jan 3, 2024
…ExtraErrorData` integration (#9914)

Co-authored-by: Lukas Stracke <lukas.stracke@sentry.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Error's cause not respected (for non-Error causes)
4 participants