Skip to content
This repository has been archived by the owner on Jan 22, 2025. It is now read-only.

Add Bank::accounts_data_len to SetRootMetrics #22509

Merged

Conversation

brooksprumo
Copy link
Contributor

@brooksprumo brooksprumo commented Jan 14, 2022

Problem

Per @ryoqun's comment here, I'd like to add an alert based on the total accounts data size. But, a datapoint for it does not exist yet.

Summary of Changes

Add a datapoint for Bank::accounts_data_len to set_root()

@brooksprumo brooksprumo force-pushed the accounts-data-len/datapoint branch from 84f5b3d to 3198a87 Compare January 14, 2022 17:36
@brooksprumo brooksprumo marked this pull request as ready for review January 14, 2022 17:49
@brooksprumo brooksprumo requested a review from sakridge January 14, 2022 17:49
@codecov
Copy link

codecov bot commented Jan 14, 2022

Codecov Report

Merging #22509 (c5a733c) into master (2756abc) will decrease coverage by 0.0%.
The diff coverage is 81.6%.

@@            Coverage Diff            @@
##           master   #22509     +/-   ##
=========================================
- Coverage    81.1%    81.1%   -0.1%     
=========================================
  Files         556      559      +3     
  Lines      150665   151002    +337     
=========================================
+ Hits       122300   122497    +197     
- Misses      28365    28505    +140     

@brooksprumo brooksprumo requested a review from carllin January 14, 2022 20:06
@@ -285,6 +287,7 @@ impl BankForks {
total_squash_cache_ms += squash_timing.squash_cache_ms as i64;
}
let new_tx_count = root_bank.transaction_count();
let accounts_data_len = root_bank.load_accounts_data_len() as i64;
Copy link
Contributor

Choose a reason for hiding this comment

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

this is the size of the accounts data modified in a block right, not the entire size of AccountsDb? Maybe should be named accounts_data_delta_len

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's the entire size, but only on this fork. So more than the delta, but not the entire AccountsDb.

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!

@brooksprumo brooksprumo merged commit 70633b5 into solana-labs:master Jan 15, 2022
@brooksprumo brooksprumo deleted the accounts-data-len/datapoint branch January 15, 2022 02:00
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants