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

skip reporting all-zero stats#21907

Merged
tao-stones merged 1 commit intosolana-labs:masterfrom
tao-stones:skip_empty_banking_stage_stats_reporting
Dec 21, 2021
Merged

skip reporting all-zero stats#21907
tao-stones merged 1 commit intosolana-labs:masterfrom
tao-stones:skip_empty_banking_stage_stats_reporting

Conversation

@tao-stones
Copy link
Copy Markdown
Contributor

Problem

Noticed a ton of all-zero banking_stage_loop_states were reported. Random sample: within 10min, there were 15,368 not-all-zero stats reporting, but 2,440,420 all-zero. Probably we should not reporting those all-zeros to save cpu time and influx DB size.

Summary of Changes

  • skip reporting if all fields are zero.

Fixes #

@tao-stones tao-stones force-pushed the skip_empty_banking_stage_stats_reporting branch 3 times, most recently from cd0a639 to e94bebf Compare December 15, 2021 02:16
@tao-stones tao-stones force-pushed the skip_empty_banking_stage_stats_reporting branch from e94bebf to 010529d Compare December 15, 2021 18:38
@codecov
Copy link
Copy Markdown

codecov Bot commented Dec 15, 2021

Codecov Report

Merging #21907 (010529d) into master (8d22ca5) will increase coverage by 0.0%.
The diff coverage is 100.0%.

@@           Coverage Diff           @@
##           master   #21907   +/-   ##
=======================================
  Coverage    81.2%    81.2%           
=======================================
  Files         516      516           
  Lines      144284   144334   +50     
=======================================
+ Hits       117256   117340   +84     
+ Misses      27028    26994   -34     

@tao-stones tao-stones requested a review from carllin December 15, 2021 22:16
Comment thread core/src/banking_stage.rs
}

fn report(&self, report_interval_ms: u64) {
// skip repoting metrics if stats is empty
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.

nit: repoting -> reporting

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.

fixed...thanks. (I need find a plugin for vim to spell-check for me)

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.

fixed...thanks. (I need find a plugin for vim to spell-check for me)

It's built in! :set spell

Comment thread core/src/banking_stage.rs
Comment on lines +123 to +151
0 == self.process_packets_count.load(Ordering::Relaxed) as u64
+ self.new_tx_count.load(Ordering::Relaxed) as u64
+ self.dropped_packet_batches_count.load(Ordering::Relaxed) as u64
+ self.dropped_packets_count.load(Ordering::Relaxed) as u64
+ self
.dropped_duplicated_packets_count
.load(Ordering::Relaxed) as u64
+ self.newly_buffered_packets_count.load(Ordering::Relaxed) as u64
+ self.current_buffered_packets_count.load(Ordering::Relaxed) as u64
+ self
.current_buffered_packet_batches_count
.load(Ordering::Relaxed) as u64
+ self.rebuffered_packets_count.load(Ordering::Relaxed) as u64
+ self.consumed_buffered_packets_count.load(Ordering::Relaxed) as u64
+ self
.consume_buffered_packets_elapsed
.load(Ordering::Relaxed)
+ self.process_packets_elapsed.load(Ordering::Relaxed)
+ self
.handle_retryable_packets_elapsed
.load(Ordering::Relaxed)
+ self.filter_pending_packets_elapsed.load(Ordering::Relaxed)
+ self.packet_duplicate_check_elapsed.load(Ordering::Relaxed)
+ self.packet_conversion_elapsed.load(Ordering::Relaxed)
+ self
.unprocessed_packet_conversion_elapsed
.load(Ordering::Relaxed)
+ self.transaction_processing_elapsed.load(Ordering::Relaxed)
}
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.

I think this may be a bit clunky to maintain every time we add a field.

What if we moved all the fields excluding last_report and id into a separate BankingStageStatsInner which derives default, and then this is_empty() check can be self == BankingStageStatsInner::default()

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.

brilliant, would work great if those counter are not atomic type. self.inner == BankingStageStatsInner::default() would invoke == on AtomicU64, which I need to implemented counter by counter, would end to be the same effect. Maybe I missed some neat Rust feature.

@tao-stones tao-stones merged commit 9c5d825 into solana-labs:master Dec 21, 2021
@tao-stones tao-stones deleted the skip_empty_banking_stage_stats_reporting branch December 21, 2021 22:20
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.

3 participants