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

Add vote instruction count metrics #35015

Closed

Conversation

HaoranYi
Copy link
Contributor

@HaoranYi HaoranYi commented Jan 30, 2024

Problem

In #24696, we removed the processed vote instruction counters because of spamming.

However, it is useful to know how many vote instructions per second are processed by the validator.

Summary of Changes

Add vote instruction counter metrics.

Fixes #

@HaoranYi HaoranYi changed the title add vote instruction count metrics Add vote instruction count metrics Jan 30, 2024
@HaoranYi HaoranYi force-pushed the 2024_jan_30_vote_instr_metric branch 8 times, most recently from 755e480 to 7226901 Compare January 30, 2024 21:53
@HaoranYi HaoranYi force-pushed the 2024_jan_30_vote_instr_metric branch from 7226901 to 5ec4dab Compare January 30, 2024 21:59
@HaoranYi
Copy link
Contributor Author

HaoranYi commented Jan 30, 2024

image

We are processing about 3K vote programs per second.

@HaoranYi HaoranYi marked this pull request as ready for review January 30, 2024 22:11
Copy link

codecov bot commented Jan 30, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (b1f8a89) 81.6% compared to head (cc1030e) 81.6%.
Report is 108 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master   #35015   +/-   ##
=======================================
  Coverage    81.6%    81.6%           
=======================================
  Files         830      830           
  Lines      224841   224854   +13     
=======================================
+ Hits       183486   183524   +38     
+ Misses      41355    41330   -25     

@sakridge
Copy link
Member

sakridge commented Feb 1, 2024

We may port this to BPF in the future, will this strategy work in a world where the program is a standard bpf program?

@sakridge
Copy link
Member

sakridge commented Feb 1, 2024

lazy_static is not ideal also, but I can see the benefit of accumulating the counters closer to where we do the actual vote processing

@HaoranYi
Copy link
Contributor Author

HaoranYi commented Feb 1, 2024

We may port this to BPF in the future, will this strategy work in a world where the program is a standard bpf program?

No, it won't work with a standard bpf program. But it will work for any other builtin programs.

For standard bfp program, we could collect and report the matrics at a higher level in the call stack here.

@HaoranYi
Copy link
Contributor Author

HaoranYi commented Feb 1, 2024

lazy_static is not ideal also, but I can see the benefit of accumulating the counters closer to where we do the actual vote processing

Yes. Not ideal. I had thought about to attach the metrics to invoke_context. But it will affect all program execution. So I decided to keep it local and use lazy_static.

@jeffwashington
Copy link
Contributor

lazy_static is not ideal also, but I can see the benefit of accumulating the counters closer to where we do the actual vote processing

What are the objections to lazy_static here?

@HaoranYi
Copy link
Contributor Author

HaoranYi commented Feb 1, 2024

lazy_static is not ideal also, but I can see the benefit of accumulating the counters closer to where we do the actual vote processing

What are the objections to lazy_static here?

lazy_static is like global variable. And global varaible make code smells.

@brooksprumo brooksprumo removed their request for review February 2, 2024 16:07
@brooksprumo
Copy link
Contributor

I'm not familiar with these metrics/how they are used, so I've removed myself as a reviewer. I defer to the subject matter experts here.

@github-actions github-actions bot added the stale [bot only] Added to stale content; results in auto-close after a week. label Feb 19, 2024
@sakridge
Copy link
Member

I'm fine with the lazy_static, just wanted to make sure we considered all the options since as haoran said it's like a global variable and we should avoid it if possible. And it could give incorrect metrics within the same process if you had multiple validators running.

@HaoranYi
Copy link
Contributor Author

HaoranYi commented Feb 23, 2024

Yes, it won't give you vote instruction metrics for individual validators. But it can provide the aggregated vote metric all the validators.
The main usage of this metric is to monitor validator running on mainnet or testnet, which is unlikely to have multiple validators running in the same process.
Do you think it is worth merging into master? @sakridge @jeffwashington @Lichtso

@jeffwashington
Copy link
Contributor

Yes, it won't give you vote instruction metrics for individual validators. But it can provide the aggregated vote metric all the validators.

I think it will 'give you vote instruction metrics for individual validators', as long as you aren't running multiple validators concurrently in the same process, right?

@jeffwashington
Copy link
Contributor

@behzadnouri had mentioned these metrics would be useful to restore, I believe.

@github-actions github-actions bot removed the stale [bot only] Added to stale content; results in auto-close after a week. label Feb 26, 2024
@willhickey
Copy link
Contributor

This repository is no longer in use. Please re-open this pull request in the agave repo: https://github.com/anza-xyz/agave

@willhickey willhickey closed this Mar 3, 2024
@HaoranYi
Copy link
Contributor Author

HaoranYi commented Mar 5, 2024

Moved to agave #90

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.

5 participants