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

track fec set turbine stats #23989

Merged
merged 7 commits into from Apr 4, 2022
Merged

Conversation

jbiseda
Copy link
Contributor

@jbiseda jbiseda commented Mar 29, 2022

Problem

need to gather data on number of shreds received over turbine per fec set

(previous PR #23725)

Summary of Changes

  • track shred count per fec set per slot in a per slot cache
  • report data when entries are evicted from the cache

Fixes #

@codecov
Copy link

codecov bot commented Mar 29, 2022

Codecov Report

Merging #23989 (4a516cf) into master (2da4e3e) will decrease coverage by 0.0%.
The diff coverage is 73.4%.

❗ Current head 4a516cf differs from pull request most recent head 1ea186d. Consider uploading reports for the commit 1ea186d to get more accurate results

@@            Coverage Diff            @@
##           master   #23989     +/-   ##
=========================================
- Coverage    81.8%    81.7%   -0.1%     
=========================================
  Files         581      585      +4     
  Lines      158312   159354   +1042     
=========================================
+ Hits       129518   130278    +760     
- Misses      28794    29076    +282     

@jbiseda jbiseda marked this pull request as ready for review March 29, 2022 23:05
@jbiseda jbiseda changed the title track fec set stats track fec set turbine stats Mar 29, 2022
@behzadnouri
Copy link
Contributor

This commit still has several design issues; at least some were already discussed before: #23872 (comment)
You are also removing some of the constructs just added in #23872 which meant to simplify things, and I don't think that was the right thing to do.
But there has been too many back and forth on this metrics work, and I think it is better that you go ahead and merge this commit, and I will follow up with some patches to address the outstanding issues here.

@jbiseda
Copy link
Contributor Author

jbiseda commented Apr 1, 2022

This commit still has several design issues; at least some were already discussed before: #23872 (comment) You are also removing some of the constructs just added in #23872 which meant to simplify things, and I don't think that was the right thing to do. But there has been too many back and forth on this metrics work, and I think it is better that you go ahead and merge this commit, and I will follow up with some patches to address the outstanding issues here.

I've changed this to only report stats as entries are evicted from the cache. @behzadnouri I'll let you follow up if you think additional changes are required here. per our discussion, we're not going to get perfect data with a bounded cache. these metrics are for sanity checks.

@jbiseda jbiseda added v1.10 and removed v1.10 labels Apr 1, 2022
@jbiseda jbiseda merged commit ee6bb0d into solana-labs:master Apr 4, 2022
@jbiseda jbiseda deleted the turbine-stats-round2 branch April 4, 2022 21:49
@jbiseda jbiseda added the v1.10 label Apr 19, 2022
mergify bot pushed a commit that referenced this pull request Apr 19, 2022
(cherry picked from commit ee6bb0d)

# Conflicts:
#	ledger/src/blockstore.rs
joeaba pushed a commit that referenced this pull request Apr 19, 2022
(cherry picked from commit ee6bb0d)

# Conflicts:
#	ledger/src/blockstore.rs
mergify bot added a commit that referenced this pull request Apr 20, 2022
* track fec set turbine stats (#23989)

(cherry picked from commit ee6bb0d)

# Conflicts:
#	ledger/src/blockstore.rs

* merge

Co-authored-by: Jeff Biseda <jbiseda@gmail.com>
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.

None yet

2 participants