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

adds metrics to turbine QUIC endpoint #33819

Merged
merged 1 commit into from
Oct 24, 2023

Conversation

behzadnouri
Copy link
Contributor

Problem

monitor turbine QUIC endpoint

Summary of Changes

added metrics.

@codecov
Copy link

codecov bot commented Oct 23, 2023

Codecov Report

Merging #33819 (0c65440) into master (b0dcaf2) will decrease coverage by 0.1%.
Report is 4 commits behind head on master.
The diff coverage is 67.0%.

❗ Current head 0c65440 differs from pull request most recent head 8e05575. Consider uploading reports for the commit 8e05575 to get more accurate results

@@            Coverage Diff            @@
##           master   #33819     +/-   ##
=========================================
- Coverage    81.8%    81.8%   -0.1%     
=========================================
  Files         809      807      -2     
  Lines      217643   217523    -120     
=========================================
- Hits       178247   178119    -128     
- Misses      39396    39404      +8     

Copy link
Contributor

@steviez steviez left a comment

Choose a reason for hiding this comment

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

Things look mostly good; just the one question about unhandled error cases


fn record_error(err: &Error, stats: &TurbineQuicStats) {
match err {
Error::CertificateError(_) => (),
Copy link
Contributor

Choose a reason for hiding this comment

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

I see we do nothing for several cases; do we just not really expect these to happen at all? Even if we don't break them all out individually, should we make a "other" case as a catch-all so do account for all possible errors?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  • CertificateError, TlsError and IoError can only happen during start-up and are passed up the callstack which causes panic: https://github.com/solana-labs/solana/blob/8e0acf481/core/src/validator.rs#L1175-L1189
  • ChannelSendError only happens during shutdown where downstream processes have winded down and the receiver end of the channel is dropped.
  • SendDatagramError::Disabled can never happen because that is a local config and we obviously are not disabling datagrams when starting the endpoint.

Copy link
Contributor

Choose a reason for hiding this comment

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

Got it, thanks for the explanation

@behzadnouri behzadnouri added the automerge Merge this Pull Request automatically once CI passes label Oct 24, 2023
@mergify mergify bot merged commit 6470544 into solana-labs:master Oct 24, 2023
33 checks passed
@behzadnouri behzadnouri added the v1.17 PRs that should be backported to v1.17 label Oct 24, 2023
mergify bot pushed a commit that referenced this pull request Oct 24, 2023
@behzadnouri behzadnouri deleted the turbine-quic-metrics branch October 24, 2023 15:08
mergify bot added a commit that referenced this pull request Oct 24, 2023
…3838)

adds metrics to turbine QUIC endpoint (#33819)

(cherry picked from commit 6470544)

Co-authored-by: behzad nouri <behzadnouri@gmail.com>
anwayde pushed a commit to firedancer-io/solana that referenced this pull request Nov 16, 2023
…#33819) (solana-labs#33838)

adds metrics to turbine QUIC endpoint (solana-labs#33819)

(cherry picked from commit 6470544)

Co-authored-by: behzad nouri <behzadnouri@gmail.com>
anwayde pushed a commit to firedancer-io/solana that referenced this pull request Nov 16, 2023
…#33819) (solana-labs#33838)

adds metrics to turbine QUIC endpoint (solana-labs#33819)

(cherry picked from commit 6470544)

Co-authored-by: behzad nouri <behzadnouri@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge Merge this Pull Request automatically once CI passes v1.17 PRs that should be backported to v1.17
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants