shotover/shotover-proxy

Update metrics + metrics-exporter-prometheus

Closed this issue · 0 comments

These two crates must be upgraded together since metrics-exporter-prometheus exposes types from metrics in its public API that we use.

However, the new version of metrics has improved its diagnostics such that unused diagnostics are shown as compiler warnings.
We have 2 such instances of these warnings.
image

Since CI will prevent merging with warnings, we need to either ignore the warnings or fix them.
I can see that our usages that are getting warned do have an associated performance cost that we should try to avoid.

  • For RedisSinkCluster
    • we can easily store the Counter in the transform instance to avoid recreating it. #1650
  • For QueryCounter we can:
    • leak the counter_name so we store it as &'static str - this will definitely be faster.
    • store every Counter we create in a HashMap<String, Counter> this should be faster, we should measure it with a microbenchmark though.

But also I think there are legitimate uses of creating a metric without using it so I've made a comment to see what upstream thinks: metrics-rs/metrics#475 (comment)

TODO, all in separate PRs:

  • #1650
  • Write a microbenchmark for QueryCounter: #1663
    • Add the bench to shotover/benches/benches/chain.rs
    • Copy redis_filter or loopback as a base for the new bench.
    • The bench should have a chain consisting of QueryCounter followed by Loopback. This is because QueryCounter ignores the responses and Loopback is the fastest transform for when we dont care about the value of responses.
    • The bench can send in redis requests as they are simplest and fastest to work with.
  • QueryCounter - String::leak() the counter_name so we store it as &'static str - this will definitely be faster, run the benchmark to double check. #1669
    • leaking memory sounds bad but its actually fine since we only create this value once for the lifetime of shotover. And leaking the memory gives us 2 advantages:
      • we never have to pay the cost of freeing its allocation when dropping it
      • we can pass it around as a reference without having to worry about lifetimes, this reduces cloning.
  • QueryCounter - store every Counter we create in a HashMap<String, Counter> this should be faster, run the benchmark to see if it is. #1671
    • When we recreate the counter every time, its doing something similar to a hashmap lookup in the background but I believe the way it does it will be slower since it has more fields to hash.
  • Update metrics + metrics-exporter-prometheus #1672