-
Notifications
You must be signed in to change notification settings - Fork 84
Handle metrics for benign application failures #905
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
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.
LGTM, though may want to wait for @Sushisource approval
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.
Nothing blocking but I think we can ditch the closure-passing
core/src/telemetry/metrics.rs
Outdated
pub(crate) fn record_failure_metric( | ||
failure: &Option<Failure>, | ||
metric_fn: impl FnOnce(), | ||
) { | ||
let is_benign = failure | ||
.as_ref() | ||
.map_or(false, |f| f.is_benign_application_failure()); | ||
if !is_benign { | ||
metric_fn(); | ||
} | ||
} | ||
|
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 don't think we really need this function - the closure passing is a bit performative. I think it'd be easier to call this at the callsite, or, if you want to save a few lines, you can make a trait that handles the option stuff an implement it for Option<Failure>
and that way you can call should_record_metric()
directly on the Option<Failure>
f133e5d
to
6f4dceb
Compare
What was changed
Updated temporal api to get application error category, use the category to filter out benign application failures from failure metrics.
Why?
Part of benign exceptions work
Closes #897