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

Report mean,min,max ConsumeWorkerMetrics::timing_metrics #3321

Merged
merged 3 commits into from
Nov 4, 2024

Conversation

ksolana
Copy link

@ksolana ksolana commented Oct 26, 2024

Problem

Only total time is reported for load_execute

Summary of Changes

Report mean, min, max metrics.

Testing:
$ solana airdrop -u http://127.0.0.1:8899 10 An8CLJA36yAivbBCo3dneq9rQz4h5MqM2exXEre4yheu

solana_metrics::metrics datapoint: banking_stage_worker_timing,id=2 cost_model_us=17i collect_balances_us=30i load_execute_us=188i load_execute_us_min=0i load_execute_us_max=188i commit_count=1i freeze_lock_us=0i record_us=42i commit_us=45i find_and_send_votes_us=35i wait_for_bank_success_us=1i wait_for_bank_failure_us=0i slot=6547i

Fixes #467

@ksolana ksolana requested a review from apfitzge October 26, 2024 06:30
@ksolana ksolana force-pushed the report_load_execute branch 2 times, most recently from e4781aa to 7db5fe3 Compare October 26, 2024 06:31
@ksolana ksolana force-pushed the report_load_execute branch from 7db5fe3 to d19f6be Compare October 26, 2024 13:53
freeze_lock_us: AtomicU64,
record_us: AtomicU64,
commit_us: AtomicU64,
find_and_send_votes_us: AtomicU64,
wait_for_bank_success_us: AtomicU64,
wait_for_bank_failure_us: AtomicU64,
count: AtomicU64,

Choose a reason for hiding this comment

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

we can just report this instead of mean - can derive the mean of any of the metrics that way instead of a separate point for each.

Copy link
Author

Choose a reason for hiding this comment

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

yeah i was also thinking that way. glad that you have the same point of view.

),
(
"load_execute_us_mean",
self.load_execute_us.swap(0, Ordering::Relaxed)/self.count.swap(0, Ordering::Relaxed),

Choose a reason for hiding this comment

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

You need to test the metrics you're adding. This will never be anything be 0 because we have already swapped self.load_execute_us to 0 above

@ksolana ksolana force-pushed the report_load_execute branch from 7240bb3 to b4fa34a Compare October 28, 2024 16:02
@ksolana
Copy link
Author

ksolana commented Oct 28, 2024

Updated the commit message with output from the validator log.

@apfitzge apfitzge self-requested a review October 31, 2024 14:15
apfitzge
apfitzge previously approved these changes Nov 1, 2024
@ksolana ksolana force-pushed the report_load_execute branch from 0d0e933 to 4e76a80 Compare November 2, 2024 02:14
@ksolana ksolana merged commit afd766a into anza-xyz:master Nov 4, 2024
40 checks passed
@ksolana ksolana deleted the report_load_execute branch November 11, 2024 02:37
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.

BankingStage: Execution Timing Metrics
2 participants