-
Notifications
You must be signed in to change notification settings - Fork 9.4k
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
misc: only allow 1 error from each audit/gatherer #6215
Conversation
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 do like this. It's way better than handling manually, since usually the loop these are being generated in is already accumulating some other value as well.
Is there a good way to alter it so it's per-error per-audit? (an additional tag? not sure of the best way) That way we can track if there's more than one type of error in a gatherer
also, this makes me wish that we had |
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.
LGTM. Just a few questions since I'm unfamiliar with this code.
const tags = opts.tags || {}; | ||
if (tags.audit) { | ||
const key = `audit-${tags.audit}-${err.message}`; | ||
if (sentryExceptionCache.has(key)) return; |
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.
Just curious: Could 1 exception have both audit and gatherer tags? In which case returning here would not allow the gatherer tags to be analyzed? I don't think so, but wanted to make sure.
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.
nope, it either happens in an audit, or a gatherer, or neither :)
|
||
if (tags.gatherer) { | ||
const key = `gatherer-${tags.gatherer}-${err.message}`; | ||
if (sentryExceptionCache.has(key)) return; |
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.
Why check has
? Can't it just set
it and override the old version in the map? Or is that not readable JS? 😕
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.
if it has the key we want to return
and skip the rest of function execution, not just the rest of the block. worth adding a comment since it's confusing already? :)
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.
🐛🔁 🧙 🐛 🔂
LGTM!
Summary
WDYT about this @brendankenny ?
Related Issues/PRs
closes #5994