feat: metrics NG #22

xDarksome · 2024-07-01T10:12:49Z

Description

Replaces metrics crate with the new one built on top of metrics facade.

Copy-pasted crate doc:

Alternative API facade to the [`metrics`] backend.

Priorities: performance, ergonomics (in that order).

Metrics reporting should be as fast as possible, without substantially
hurting the code ergonomics.

A trivial atomic counter increment MUST NOT allocate stuff on the heap
(looking at you [`metrics::counter`]) and it SHOULD NOT acquire locks or
do [`HashMap`](std::collections::HashMap) lookups unless absolutely
necessary.

Additionally:

remove metrics example, examples are now in metrics crate docs
update alloc example to use new metrics

How Has This Been Tested?

Some unit tests / doc tests inside metrics crate itself, plus going to test in the relay

Due Diligence

Breaking change
Requires a documentation update
Requires a e2e/integration test update

crates/future/src/lib.rs

crates/metrics/src/future.rs

heilhead · 2024-07-03T09:22:49Z

crates/metrics/src/label.rs

+    type Target = M;
+
+    fn resolve_labels(&self, (label,): (EnumLabel<NAME, T>,)) -> &M {
+        let debug_panic = || {


Maybe move this outside the function body?

Why? It's only being used inside this function

I don't know. Maybe it's just me, but it feels weird to have these embedded util functions.

An item should have the smallest possible scope so you don't have a question "where else this thing is being used"

crates/metrics/src/label.rs

heilhead · 2024-07-03T09:25:13Z

crates/metrics/src/label.rs

+{
+    // TODO: Switch to `[(T, M); T::VARIANT_COUNT]` once generic parameters are
+    // allowed to be used in const expressions.
+    type MetricCollection = Vec<(T, M)>;


Since this PR is focused on micro-optimizations, we should consider stack-allocated SmallVec for this.

It won't be on the stack anyway, as it's always static. And it will only allocate once per metric during the initialization
I guess it will remove a single indirection (heap pointer) for happy-path scenarios

Yeah, the idea is to reduce indirection and allocations on push/resize.

Did some benchmarking and the difference is around a rounding error, however Vec allows to allocate exactly the amount we need, as we know all the label values beforehand.

heilhead · 2024-07-03T09:26:50Z

crates/metrics/src/label.rs

+        };
+
+        let idx = label.0.ordinal();
+        let mut idx = if idx < 0 {


Why not use debug_assert!(idx >= 0)?

Because I'm also doing idx = 0 in the branch, so the release build won't crash

That can be replaced by idx.max(0).

We need to check that the index is in [0, vec.len()) range

heilhead · 2024-07-03T09:27:15Z

crates/metrics/src/label.rs

+            idx as usize
+        };
+
+        if idx >= self.collection.len() {


Same, I'd use debug_assert!() instead of custom panic macro/function.

heilhead · 2024-07-03T09:30:45Z

crates/metrics/src/label.rs

+}
+
+pub struct StringCollection<T, M: 'static> {
+    inner: ArcSwap<HashMap<T, &'static M>>,


Now that you're migrating the relay, can you guesstimate the highest cardinality that such StringCollection can have? If it's pretty low, maybe it'd make sense to replace HashMap with SmallVec.

Would depend on the application and whether people ab(using) this instead of using proper EnumLabels.
But for the relay the only "valid" use-cases are HTTP routes and status codes, which makes the cardinality <10

Probably makes sense to implement both versions, but let's try SmallVec

But for the relay the only "valid" use-cases are HTTP routes and status codes, which makes the cardinality <10

If it's route_name * response_code it'd likely be more than 10. Not sure if it's worth it. Can you double check the existing HTTP route metrics to confirm cardinality?

Different labels use separate collections, so it won't multiply them

We definitely have <10 http routes in the relay

Benchmark results: Binary search on Vec/SmallVec is 2x faster for 10 elements, and the same as HashMap lookup for 20 elements.
After that the performance falls significantly, so I suggest we leave HashMap here, as StringCollection is not the recommended way to implement labels anyway and it should support all other edge-cases.

crates/metrics/src/label.rs

heilhead · 2024-07-03T09:31:50Z

crates/metrics/src/label.rs

+            let mut inner_clone: HashMap<_, _> = (**inner).clone();
+
+            let name = const { resolve_label_name::<NAME>() };
+            let label_ = Label::new(name, label.to_owned().to_string());


What's up with the _ suffix?

I didn't have the brain power to come up with a different name, basically there are two labels in scope and both of them are being used later. It's similar to the math notation when you have variable a and a'

Looks weird...

heilhead

Looks good overall, though I left a few suggestions.

The main thing missing is tests. Can we also cover the Box::leak stuff with some allocation tests to confirm there's no unsound or memory leak scenario?

xDarksome · 2024-07-03T10:34:38Z

@heilhead converting Duration to milliseconds everywhere is PITA, as metrics already supports Duration directly, but converts it to f64 seconds.

As we are going to update the graphs anyway, I'm removing the conversion here and from the relay, so Duration is always being represented in seconds.

heilhead

Looks great!

feat: metrics NG

62cfec1

xDarksome self-assigned this Jul 1, 2024

xDarksome added 3 commits July 1, 2024 10:14

fix: remove unused file

8fc99dd

fix: tidy up cargo tree

2a008f4

fix: disambiguate crate names

3aaaf27

xDarksome requested review from nopestack, heilhead, chris13524 and geekbrother July 1, 2024 10:56

xDarksome added 7 commits July 1, 2024 13:36

fix: missing derives

a83329a

feat: generalize EnumLabel to support different label names

b9d0c85

feat: accept impl<Into<T>> in EnumLabel constructor

c0a6ad2

feat: Optional label

f255dc5

fix: OptionalLabel -> Optional

0e8e64e

feat: macros 💀

eb668b8

fix: make Optional label compatible with macro

fa69002