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

Condense Blockstore RPC API datapoints #34045

Merged
merged 3 commits into from
Nov 14, 2023

Conversation

steviez
Copy link
Contributor

@steviez steviez commented Nov 13, 2023

Problem

Currently, the RPC API that touch the Blockstore emit a datapoint for
each call. For an RPC node serving many requests, these datapoints
could get quite noisy, both in logs as well as traffic to the metrics
agent.

Summary of Changes

So, instead of submitting a datapoint for every call, accumulate the
number of calls in a struct and report that entire struct periodically.

Copy link

codecov bot commented Nov 13, 2023

Codecov Report

Merging #34045 (55b823b) into master (47a98eb) will decrease coverage by 0.1%.
The diff coverage is 83.6%.

@@            Coverage Diff            @@
##           master   #34045     +/-   ##
=========================================
- Coverage    81.9%    81.9%   -0.1%     
=========================================
  Files         811      811             
  Lines      219552   219597     +45     
=========================================
+ Hits       179902   179923     +21     
- Misses      39650    39674     +24     

@steviez steviez marked this pull request as ready for review November 13, 2023 23:54
Copy link
Contributor

@CriesofCarrots CriesofCarrots left a comment

Choose a reason for hiding this comment

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

Just the one question about counting get_complete_block calls.

ledger/src/blockstore_metrics.rs Show resolved Hide resolved
@@ -2033,10 +2045,9 @@ impl Blockstore {
slot: Slot,
Copy link
Contributor

Choose a reason for hiding this comment

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

(sorry, this is the closest place I could add a comment) Should we add a datapoint for get_complete_block()? It would be used when getBlock is called with commitment: confirmed, so potentially quite a lot, although it would also double count on commitment: finalized.
Could be a follow-up, if yes is the answer.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point. get_confirmed_signatures_for_address2() could also call get_complete_block() via get_sorted_block_signatures().

I'm tempted to leave things as-is for now as what I introduced is at parity with what we had previously. I think there are several cases of double counts (get_block_time() is called as a subroutine as well), and probably makes sense to review all the methods and come up with a consistent scheme across the board

@steviez steviez merged commit d1d4c1c into solana-labs:master Nov 14, 2023
32 checks passed
@steviez steviez deleted the bstore_condense_rpc_api_metrics branch November 14, 2023 18:19
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