Skip to content
This repository was archived by the owner on Jun 30, 2025. It is now read-only.

zinnia: fix double reporting of crash errors#434

Merged
juliangruber merged 6 commits intomainfrom
fix/zinnia-crash-double-reporting
May 3, 2024
Merged

zinnia: fix double reporting of crash errors#434
juliangruber merged 6 commits intomainfrom
fix/zinnia-crash-double-reporting

Conversation

@juliangruber
Copy link
Copy Markdown
Member

@juliangruber juliangruber commented Apr 26, 2024

  • swap sentry token

@juliangruber juliangruber requested a review from bajtos April 26, 2024 16:40
Comment thread lib/zinnia.js Outdated
})
}
// If it should be reported, it was already handled
err.reportToSentry = false
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I am a bit confused about why we are reporting the error to Sentry twice. Shouldn't we fix that instead of adding a new flag to the error object?

The condition at the top of this file checks err.shouldReportToSentry, but this code is setting err.reportToSentry.

How can we (manually) verify that the changes in the pull request work as intended?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I am a bit confused about why we are reporting the error to Sentry twice. Shouldn't we fix that instead of adding a new flag to the error object?

Absolutely, we're fixing it by adding the flag :D Or at least that's the plan.

The problem is this: There's an inner try/catch and an outer try/catch. The inner try/catch reports the error to Sentry. It then also throws the error, because that is required for control flow (so that the outer loop ends). In this outer loop, we're catching all errors and reporting them to Sentry. By adding the flag we're telling the outer catch handler that the error has already been submitted to Sentry.

Do you have another suggestion how to implement this?

Comment thread lib/zinnia.js Outdated
@juliangruber juliangruber requested a review from bajtos April 29, 2024 14:33
@juliangruber
Copy link
Copy Markdown
Member Author

I have changed the sentry key and disabled the old one

Copy link
Copy Markdown
Member

@bajtos bajtos left a comment

Choose a reason for hiding this comment

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

I prefer slightly more reportedToSentry over reportToSentry for the following reasons:

  • reportedToSentry describes what happened. When nobody reads/interprets this field, then it's ok - we are telling what happened, not asking what to do
  • I understand reportToSentry as a command asking some other piece of code to report this error to Sentry. If that other piece of code does not report the error, then this command will be unfulfilled.

I also find it a bit weird when a property has a default value of true.

At the same time, this is subjective and a minor detail, so I am okay with landing this PR as it is now.

Comment thread lib/zinnia.js Outdated
Comment thread lib/zinnia.js Outdated
@juliangruber juliangruber merged commit 856cb55 into main May 3, 2024
@juliangruber juliangruber deleted the fix/zinnia-crash-double-reporting branch May 3, 2024 05:59
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants