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

separates out connection-cache metrics for different protocols #31803

Merged
merged 1 commit into from
May 25, 2023

Conversation

behzadnouri
Copy link
Contributor

Problem

Need to distinguish connection-cache metrics for different protocols (e.g. TPU vs turbine).

Summary of Changes

Separated out connection-cache metrics for different protocols

@codecov
Copy link

codecov bot commented May 25, 2023

Codecov Report

Merging #31803 (6cf31b4) into master (0e93090) will decrease coverage by 0.1%.
The diff coverage is 81.6%.

@@            Coverage Diff            @@
##           master   #31803     +/-   ##
=========================================
- Coverage    81.9%    81.9%   -0.1%     
=========================================
  Files         738      738             
  Lines      205902   205906      +4     
=========================================
- Hits       168705   168662     -43     
- Misses      37197    37244     +47     

KirillLykov
KirillLykov previously approved these changes May 25, 2023
Copy link
Contributor

@KirillLykov KirillLykov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like that the metrics are grouped now, the one concern beside "bench-tps" naming is that we might end up with too many metrics at the end with some not following naming convention you've introduced implicitly here. One solution might be to use enums instead but this will require bigger boilerplate so don't have a strong opinion

@@ -83,10 +83,16 @@ fn create_connection_cache(
client_node_id: Option<&Keypair>,
) -> ConnectionCache {
if !use_quic {
return ConnectionCache::with_udp(tpu_connection_pool_size);
return ConnectionCache::with_udp(
"connection_cache_bench_tps_udp",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bench-tps metrics are usually named like bench-tps-.*, so it is simpler to search based on the bench client name.
Outside of scope of this PR, would be generally good to agree on some convention about metrics names (snake case vs kebab, start with capital letter or not etc)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

renamed to bench-tps-*

@behzadnouri behzadnouri merged commit 9281ab7 into solana-labs:master May 25, 2023
@behzadnouri behzadnouri deleted the connection-cache-metric branch May 25, 2023 14:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants