feat(reporter/sentry): filtering funcs to set custom error level on sentry #18
Conversation
upfluence-bot
added
reviewed/needs-review
reviewed/needs-work
and removed
reviewed/needs-review
labels
reporter/sentry/options.go
Outdated
|
|
||
| var ( | ||
| defaultErrorLevelFuncs = []ErrorLevelFunc{ | ||
| ErrorIsFunc(context.DeadlineExceeded, sentry.LevelWarning), |
There was a problem hiding this comment.
Won't this hide some big errors ? Like deadlock or things like that ?
There was a problem hiding this comment.
Thankfully this doesn't hide them, simply marks them as severity "Warning", which will allow us to have 2 different boards (one for errors, one for warnings), which ought to filter a lot of the noise.
We'll still notice if we have 3000 warnings rise up all of a sudden 👀
upfluence-bot
added
reviewed/needs-work
and removed
reviewed/needs-review
reviewed/needs-work
labels
lordteka
left a comment
lordteka
left a comment
There was a problem hiding this comment.
Nice improvment, also nice effort on the doc/comment
reporter/sentry/options.go
Outdated
| } | ||
| } | ||
|
|
||
| // ErrorCauseTextContainsFunc an ErrorLevelFunc of the passed level that checks |
reporter/sentry/options.go
Outdated
| } | ||
|
|
||
| // AppendErrorLevelFuncs adds the passed funcs to the ErrorLevelFuncs of the Reporter | ||
| func AppendErrorLevelFuncs(funcs []ErrorLevelFunc) Option { |
There was a problem hiding this comment.
| func AppendErrorLevelFuncs(funcs []ErrorLevelFunc) Option { | |
| func AppendErrorLevelFuncs(funcs ...ErrorLevelFunc) Option { |
There was a problem hiding this comment.
UX improvements through the roof.
You can do the replace one as well but I think that one is less important, and it makes sense there to pass nil to remove any filtering, wheras ReplaceErrorLevelFuncs() isn't really self-descriptive
There was a problem hiding this comment.
Yeap, and I feel like it expects a "whole" array for replacement rather than a variadic
It most likely also won't be used much 😄
upfluence-bot
added
reviewed/ready-to-merge
reviewed/needs-work
and removed
reviewed/ready-to-merge
labels
upfluence-bot
added
reviewed/needs-review
reviewed/needs-work
and removed
reviewed/needs-work
reviewed/needs-review
labels
AlexisMontagne
left a comment
AlexisMontagne
left a comment
There was a problem hiding this comment.
Overall it looks good!
| } | ||
|
|
||
| ts := tags.GetTags(err) | ||
| errorTags := tags.GetTags(err) |
There was a problem hiding this comment.
nitpick: the repository is already named errors is the error part of errorTags really needed? could it simply be tags?
Same feedback for:
computeErrorLevelerrorLevelMappers
There was a problem hiding this comment.
The problem here is that tags is a symbol already (the import name for that package), and I really dislike it being called ts or t. Any other suggestion that does not make it illegible? 😓
There was a problem hiding this comment.
that makes sends for errorTags, could you adjust the two others?
There was a problem hiding this comment.
yeah, can do when I next get to work on this one 👍
What does this PR do?
Fixes #
What are the observable changes?
Good PR checklist
Additional Notes