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

RedisSinkCluster - Avoid recreating Counter #1650

Closed
Tracked by #1649
rukai opened this issue Jun 3, 2024 · 0 comments · Fixed by #1658
Closed
Tracked by #1649

RedisSinkCluster - Avoid recreating Counter #1650

rukai opened this issue Jun 3, 2024 · 0 comments · Fixed by #1658
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

The counter!() macro is used to create a Counter instance which we can then call increment(1) on to increment a metric reported in shotover's prometheus metrics.

Counter instances can be either:

  • stored to be reused many times
  • created, used, discarded on each use.

The end result of both approaches is identical.

However, the advantages of reusing Counters is:

  • Better performance - creating a Counter can be expensive
  • Easier maintenance - the metrics labels are defined in a single place

RedisSinkCluster contains a metric named shotover_failed_requests_count, we should create that metric in RedisSinkCluster::new().
We already register the shotover_failed_requests_count counter metric in RedisSinkCluster::new, we just dont hold onto the Counter for later use.
So we should take the registration, and store the returned value in RedisSinkCluster.
To do this we will need to replace the call to sink_cluster.get_name() with just the NAME constant, which enables further cleanup in removing the get_name method. (just follow what the warnings tell you)

When implementing this PR, you can refer to how other transforms store and reuse Counter's:

out_of_rack_requests: counter!("shotover_out_of_rack_requests_count", "chain" => chain_name, "transform" => NAME),

@rukai rukai added good first issue Good for newcomers cleanup-internal Any cleanup that is not cleanup-api labels Jun 3, 2024
@justinweng-instaclustr justinweng-instaclustr self-assigned this Jun 5, 2024
justinweng-instaclustr added a commit to justinweng-instaclustr/shotover-proxy that referenced this issue Jun 6, 2024
justinweng-instaclustr added a commit to justinweng-instaclustr/shotover-proxy that referenced this issue Jun 11, 2024
justinweng-instaclustr added a commit to justinweng-instaclustr/shotover-proxy that referenced this issue Jun 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cleanup-internal Any cleanup that is not cleanup-api good first issue Good for newcomers
Projects
None yet
2 participants