Migrate IPA code to new metrics engine #1379
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1379 +/- ##
==========================================
+ Coverage 93.54% 93.63% +0.08%
==========================================
Files 222 223 +1
Lines 36986 36998 +12
==========================================
+ Hits 34600 34644 +44
+ Misses 2386 2354 -32 ☔ View full report in Codecov by Sentry. |
ipa-core/src/telemetry/mod.rs
Outdated
| @@ -33,84 +31,20 @@ pub mod metrics { | |||
| } | |||
| } | |||
|
|
|||
| impl From<RequestProtocolVersion> for KeyName { | |||
| impl From<RequestProtocolVersion> for &'static str { | |||
There was a problem hiding this comment.
Maybe this should be an as_str method?
There was a problem hiding this comment.
its a little less ergonomic at the callsite, but not too bad
| @@ -84,6 +86,16 @@ impl<T> IndexMut<ShardIndex> for Vec<T> { | |||
| } | |||
| } | |||
|
|
|||
| impl LabelValue for ShardIndex { | |||
There was a problem hiding this comment.
Does it makes sense to implement LabelValue for T: Into<u64>? Given the restrictions on overlapping impls, that does seem like it could lead to problems down the line. On the other hand, since this isn't a published API, we can always switch back to the set of impls in this PR if the blanket impl becomes a problem.
There was a problem hiding this comment.
yea, overlapping impls was the reason not to. I ran into this problem in an early implementation and it gets very annoying if you need to implement LabelValue a bit differently. Scraping out blanket implementation when you would need that change would probably be less efficient.
Maybe I can provide a macro to implement LabelValue on those types....
There was a problem hiding this comment.
I'm not opposed to a macro, but I also don't think there's a clear justification for one here vs. just live with a little boilerplate.
There was a problem hiding this comment.
yea, that is my thinking too - LabelValue does not add too much clutter, but provides explicitness
This is the final PR to upgrade IPA to use new metric engine. This change removes the old metric dependency and updates the code to use new metrics.
This approach was tested in #1278 and the only difference here is that we don't consume metrics yet. #1353 will take care of it