feat: emit metrics for any failed notif conversions #39
Conversation
pjenvey
added this to the 2018Q2-2.1 Rust connection node with broadcast deployed to Prod milestone
| @@ -66,19 +68,22 @@ pub fn fetch_messages( | |||
| ..Default::default() | |||
| }; | |||
|
|
|||
| let metrics = Rc::clone(metrics); | |||
There was a problem hiding this comment.
I just noticed the rust team began recommending Rc::clone vs .clone about a year ago, makes it clearer that this is a cheap Rc clone vs worrying about it being a different clone
There was a problem hiding this comment.
It makes sense for containers like RC (and I suppose vec may be similar), but I keep feeling that a given object should be responsible for making it's own clone, since it knows best how deep it needs to copy itself. I wonder if we'll see this pattern play out for other, similar structures?
There was a problem hiding this comment.
Not Vec but it makes sense for smart pointers. the original RFC (which suggested a new method new_ref instead that was rejected) mentioned Rc/Arc and the associated Weak pointer types. Here's a pretty good example it outlines:
https://github.com/nical/rfcs/blob/rc-newref-clone/text/0000-rc-newref-clone.md#example
| where | ||
| E: Display, | ||
| { | ||
| error!("Failed {} conversion: {}", name, err); |
There was a problem hiding this comment.
I had this as debug! but figured let's error! for now so it's seen on prod
|
|
||
| /// Like Result::ok, convert from Result<T, E> to Option<T> but applying a | ||
| /// function to the Err value | ||
| fn ok_or_inspect<T, E, F>(result: StdResult<T, E>, op: F) -> Option<T> |
There was a problem hiding this comment.
almost tempted to make this into a whole trait extension, time will tell how useful it is
There was a problem hiding this comment.
Well, right now you're using it just to contain a call to conversion_err. (I almost wonder if it might just be better to combine them into one call to reduce some of the templating, but that's a super minor nit.)
It'll be interesting to see how these play out in the future.
There was a problem hiding this comment.
I had it that way to start with then ended up here. Already had in mind I wanted an Option::ok_or_else like call for Result (but it doesn't have one because it would be dropping Err in that case).
It had "ua.notification_read.error" either embedded inside of it or that also needed to be an argument, so went for the generic combinator I really wanted
pjenvey
force-pushed
the
feat/33
branch
3 times, most recently
from
ed16b64
to
7ef1594
Compare
to avoid passing them around everywhere Issue #33
|
|
||
| /// Like Result::ok, convert from Result<T, E> to Option<T> but applying a | ||
| /// function to the Err value | ||
| fn ok_or_inspect<T, E, F>(result: StdResult<T, E>, op: F) -> Option<T> |
There was a problem hiding this comment.
Well, right now you're using it just to contain a call to conversion_err. (I almost wonder if it might just be better to combine them into one call to reduce some of the templating, but that's a super minor nit.)
It'll be interesting to see how these play out in the future.
a separate refactor commit
Closes #33