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

Show staked vs nonstaked packets sent down/throttled #600

Merged

Conversation

lijunwangs
Copy link

@lijunwangs lijunwangs commented Apr 5, 2024

Problem

The current metrics cannot tell what are the portions of packets sent down and throttled from staked vs non-staked.

Summary of Changes

Add two metrics in streamer:

staked_packets_sent_for_batching
unstaked_packets_sent_for_batching
throttled_unstaked_streams
throttled_staked_streams

Fixes #

@lijunwangs lijunwangs requested review from ryleung-solana and removed request for ryleung-solana April 5, 2024 08:56
@lijunwangs lijunwangs changed the title Show staked vs nonstaked packets sent for batching Show staked vs nonstaked packets sent down/throttled Apr 5, 2024
ryleung-solana
ryleung-solana previously approved these changes Apr 5, 2024
@codecov-commenter
Copy link

Codecov Report

Attention: Patch coverage is 64.28571% with 5 lines in your changes are missing coverage. Please review.

Project coverage is 81.8%. Comparing base (0b9c637) to head (7a00f15).
Report is 6 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #600   +/-   ##
=======================================
  Coverage    81.8%    81.8%           
=======================================
  Files         851      851           
  Lines      230149   230163   +14     
=======================================
+ Hits       188438   188467   +29     
+ Misses      41711    41696   -15     

Copy link

mergify bot commented Apr 5, 2024

Backports to the stable branch are to be avoided unless absolutely necessary for fixing bugs, security issues, and perf regressions. Changes intended for backport should be structured such that a minimum effective diff can be committed separately from any refactoring, plumbing, cleanup, etc that are not strictly necessary to achieve the goal. Any of the latter should go only into master and ride the normal stabilization schedule.

Copy link

mergify bot commented Apr 5, 2024

Backports to the beta branch are to be avoided unless absolutely necessary for fixing bugs, security issues, and perf regressions. Changes intended for backport should be structured such that a minimum effective diff can be committed separately from any refactoring, plumbing, cleanup, etc that are not strictly necessary to achieve the goal. Any of the latter should go only into master and ride the normal stabilization schedule. Exceptions include CI/metrics changes, CLI improvements and documentation updates on a case by case basis.

@lijunwangs lijunwangs merged commit b443cfb into anza-xyz:master Apr 5, 2024
40 checks passed
mergify bot pushed a commit that referenced this pull request Apr 5, 2024
* Show staked vs nonstaked packets sent down

* add metrics on throttled staked vs non-staked

(cherry picked from commit b443cfb)

# Conflicts:
#	streamer/src/quic.rs
mergify bot pushed a commit that referenced this pull request Apr 5, 2024
* Show staked vs nonstaked packets sent down

* add metrics on throttled staked vs non-staked

(cherry picked from commit b443cfb)

# Conflicts:
#	streamer/src/quic.rs
Copy link

@HaoranYi HaoranYi left a comment

Choose a reason for hiding this comment

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

lgtm

Comment on lines +180 to +183
pub(crate) total_staked_packets_sent_for_batching: AtomicUsize,
pub(crate) total_unstaked_packets_sent_for_batching: AtomicUsize,
pub(crate) throttled_staked_streams: AtomicUsize,
pub(crate) throttled_unstaked_streams: AtomicUsize,
Copy link

Choose a reason for hiding this comment

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

Change looks good to me; should we deprecate total_packets_sent_for_batching and throttled_streams; both of these can now be implied by adding unstaked + staked

Copy link
Author

Choose a reason for hiding this comment

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

I think it is okay to leave it alone for convenience in doing Grafana.

lijunwangs added a commit that referenced this pull request Apr 11, 2024
… of #600) (#614)

* Show staked vs nonstaked packets sent down/throttled (#600)

* Show staked vs nonstaked packets sent down

* add metrics on throttled staked vs non-staked

(cherry picked from commit b443cfb)

# Conflicts:
#	streamer/src/quic.rs

* fix merge conflicts

---------

Co-authored-by: Lijun Wang <83639177+lijunwangs@users.noreply.github.com>
Co-authored-by: HaoranYi <haoran.yi@solana.com>
t-nelson pushed a commit that referenced this pull request Apr 11, 2024
… of #600) (#613)

* Show staked vs nonstaked packets sent down/throttled (#600)

* Show staked vs nonstaked packets sent down

* add metrics on throttled staked vs non-staked

(cherry picked from commit b443cfb)

# Conflicts:
#	streamer/src/quic.rs

* fix merge conflicts

---------

Co-authored-by: Lijun Wang <83639177+lijunwangs@users.noreply.github.com>
Co-authored-by: HaoranYi <haoran.yi@solana.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants