Skip to content
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

Update metrics + metrics-exporter-prometheus #1649

Closed
5 tasks done
rukai opened this issue Jun 3, 2024 · 0 comments
Closed
5 tasks done

Update metrics + metrics-exporter-prometheus #1649

rukai opened this issue Jun 3, 2024 · 0 comments
Assignees
Labels
cleanup-internal Any cleanup that is not cleanup-api good first issue Good for newcomers

Comments

@rukai
Copy link
Member

rukai commented Jun 3, 2024

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
  • 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:

  • RedisSinkCluster - Avoid recreating Counter #1650
  • Write a microbenchmark for QueryCounter: #1649 Add 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. #1649 Leak query counter name to improve QueryCounter performance #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. #1649 Reuse query counters #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 #1649 Upgrade metrics + metrics-exporter-prometheus dependencies #1672
@rukai rukai added the cleanup-internal Any cleanup that is not cleanup-api label Jun 3, 2024
@rukai rukai added the good first issue Good for newcomers label Jun 11, 2024
justinweng-instaclustr added a commit to justinweng-instaclustr/shotover-proxy that referenced this issue Jun 13, 2024
justinweng-instaclustr added a commit to justinweng-instaclustr/shotover-proxy that referenced this issue Jun 17, 2024
justinweng-instaclustr added a commit to justinweng-instaclustr/shotover-proxy that referenced this issue Jun 17, 2024
justinweng-instaclustr added a commit to justinweng-instaclustr/shotover-proxy that referenced this issue Jun 19, 2024
justinweng-instaclustr added a commit to justinweng-instaclustr/shotover-proxy that referenced this issue Jun 19, 2024
justinweng-instaclustr added a commit to justinweng-instaclustr/shotover-proxy that referenced this issue Jun 19, 2024
justinweng-instaclustr added a commit to justinweng-instaclustr/shotover-proxy that referenced this issue Jun 19, 2024
justinweng-instaclustr added a commit to justinweng-instaclustr/shotover-proxy that referenced this issue Jun 20, 2024
justinweng-instaclustr added a commit to justinweng-instaclustr/shotover-proxy that referenced this issue Jun 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment