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

Add error specific timings in per-program metrics logging#22225

Merged
carllin merged 1 commit intosolana-labs:masterfrom
carllin:FixMetrics2
Jan 3, 2022
Merged

Add error specific timings in per-program metrics logging#22225
carllin merged 1 commit intosolana-labs:masterfrom
carllin:FixMetrics2

Conversation

@carllin
Copy link
Copy Markdown
Contributor

@carllin carllin commented Jan 2, 2022

Problem

  1. Bug in ExecuteDetailsTimings::accumulate(), which didn't accumulate the errors inside the program timings
  2. Compute units, microseconds logged in per-program metrics metrics can be skewed by errors which do not count compute units for errors in the total

Summary of Changes

  1. Accumulate per program timings inside ExecuteDetailsTimings::accumulate(), add tests
  2. Add separate logging for error count, error compute units consumed inside per-program metrics
    Fixes #

@carllin carllin requested review from t-nelson and tao-stones January 2, 2022 23:24
@carllin carllin changed the title Fix bug, add error specific timings Add error specific timings Jan 2, 2022
@carllin carllin changed the title Add error specific timings Add error specific timings in per-program metrics logging Jan 2, 2022
.accumulated_units
.saturating_add(other.accumulated_units);
program_timing.count = program_timing.count.saturating_add(other.count);
program_timing.accumulate_program_timings(other);
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

here was the bug, didn't account for the newly added errored_txs_compute_consumed

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

good catch!

Comment thread core/src/progress_map.rs
Comment on lines +154 to +159
("errored_units", time.total_errored_units, i64),
("count", time.count, i64),
(
"errored_count",
time.errored_txs_compute_consumed.len(),
i64
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The new per-program error compute units and number of errored transactions

@codecov
Copy link
Copy Markdown

codecov Bot commented Jan 3, 2022

Codecov Report

Merging #22225 (6b009bc) into master (d6ec103) will increase coverage by 0.0%.
The diff coverage is 90.4%.

@@           Coverage Diff           @@
##           master   #22225   +/-   ##
=======================================
  Coverage    81.0%    81.0%           
=======================================
  Files         523      523           
  Lines      146497   146558   +61     
=======================================
+ Hits       118741   118797   +56     
- Misses      27756    27761    +5     

.accumulated_units
.saturating_add(other.accumulated_units);
program_timing.count = program_timing.count.saturating_add(other.count);
program_timing.accumulate_program_timings(other);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

good catch!

@carllin carllin merged commit 0055929 into solana-labs:master Jan 3, 2022
mergify Bot pushed a commit that referenced this pull request Jan 3, 2022
(cherry picked from commit 0055929)

# Conflicts:
#	program-runtime/src/timings.rs
mergify Bot pushed a commit that referenced this pull request Jan 3, 2022
carllin added a commit that referenced this pull request Jan 3, 2022
carllin added a commit that referenced this pull request Jan 4, 2022
mergify Bot added a commit that referenced this pull request Jan 4, 2022
(cherry picked from commit 0055929)

Co-authored-by: carllin <carl@solana.com>
carllin added a commit that referenced this pull request Jan 4, 2022
mergify Bot added a commit that referenced this pull request Jan 4, 2022
(cherry picked from commit 0055929)

Co-authored-by: carllin <carl@solana.com>
@brooksprumo brooksprumo mentioned this pull request Jan 5, 2022
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