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

Bucket entry counters and metrics #4436

Merged

Conversation

ThomasBrady
Copy link
Contributor

Description

Resolves #4426

  • Adds BucketEntryCounters to BucketIndexImpl to count entry types and duration.
  • Adds functionality to BucketList to accumulate the counters for all buckets
  • Adds functionality to LedgerManager to emit these metrics to medida

Still TODO:

  • Local testing of serialization of indexes with counters

Out of scope:

  • XDR Changes for including counters in ledger close meta
  • grafana updates

Checklist

  • Reviewed the contributing document
  • Rebased on top of master (no merge commits)
  • Ran clang-format v8.0.0 (via make format or the Visual Studio extension)
  • Compiles
  • Ran all tests
  • If change impacts performance, include supporting evidence per the performance document

SirTyson
SirTyson previously approved these changes Aug 23, 2024
Copy link
Contributor

@SirTyson SirTyson left a comment

Choose a reason for hiding this comment

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

Looks great! A lot of super small nit picky things. I'll go ahead and approve now so you can merge after a few fixups next week when I'm out.

src/bucket/BucketIndexImpl.h Outdated Show resolved Hide resolved
src/bucket/BucketIndexImpl.h Outdated Show resolved Hide resolved
src/bucket/Bucket.cpp Outdated Show resolved Hide resolved
src/bucket/Bucket.cpp Outdated Show resolved Hide resolved
src/bucket/BucketIndexImpl.cpp Outdated Show resolved Hide resolved
src/bucket/BucketIndexImpl.h Outdated Show resolved Hide resolved
src/bucket/BucketList.cpp Outdated Show resolved Hide resolved
src/bucket/BucketManagerImpl.cpp Outdated Show resolved Hide resolved
src/ledger/LedgerManagerImpl.cpp Outdated Show resolved Hide resolved
src/bucket/BucketIndexImpl.h Outdated Show resolved Hide resolved
@ThomasBrady ThomasBrady changed the title WIP Bucket entry counters and metrics Bucket entry counters and metrics Aug 26, 2024
@ThomasBrady ThomasBrady force-pushed the add-per-bucket-entry-size-metrics-2 branch from 2d6f714 to f68c7a3 Compare August 30, 2024 21:18
Copy link
Contributor

@SirTyson SirTyson left a comment

Choose a reason for hiding this comment

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

A few more changes. Also, these new metrics should be added to metrics.md.

src/bucket/Bucket.cpp Outdated Show resolved Hide resolved
src/bucket/Bucket.cpp Outdated Show resolved Hide resolved
src/bucket/BucketIndexImpl.cpp Outdated Show resolved Hide resolved
src/bucket/BucketIndexImpl.h Show resolved Hide resolved
src/bucket/BucketManagerImpl.h Outdated Show resolved Hide resolved
src/bucket/BucketManagerImpl.h Outdated Show resolved Hide resolved
src/bucket/Bucket.h Show resolved Hide resolved
src/bucket/BucketManagerImpl.cpp Outdated Show resolved Hide resolved
src/bucket/BucketManagerImpl.cpp Outdated Show resolved Hide resolved
src/bucket/BucketManagerImpl.cpp Outdated Show resolved Hide resolved
SirTyson
SirTyson previously approved these changes Sep 4, 2024
Copy link
Contributor

@SirTyson SirTyson left a comment

Choose a reason for hiding this comment

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

LGTM! A couple last super nit picky things then we can merge.

src/bucket/Bucket.h Outdated Show resolved Hide resolved
src/bucket/BucketIndexImpl.cpp Outdated Show resolved Hide resolved
src/bucket/BucketIndexImpl.cpp Outdated Show resolved Hide resolved
src/bucket/BucketManagerImpl.cpp Show resolved Hide resolved
src/bucket/BucketManagerImpl.cpp Outdated Show resolved Hide resolved
src/bucket/BucketManagerImpl.cpp Outdated Show resolved Hide resolved
src/ledger/LedgerTypeUtils.cpp Outdated Show resolved Hide resolved
@ThomasBrady
Copy link
Contributor Author

This PR is the dashboard changes FYI https://github.com/stellar/puppet-v4/pull/4081

@SirTyson
Copy link
Contributor

SirTyson commented Sep 5, 2024

Last round of changes looks good! Once we have a serialization test, we'll be ready to merge.

@ThomasBrady
Copy link
Contributor Author

ThomasBrady commented Sep 5, 2024

Last round of changes looks good! Once we have a serialization test, we'll be ready to merge.

I'm pretty sure this is already tested by "serialize bucket indexes", but I added some extra assertions to make sure the counters are non-empty before the equality comparison.

@ThomasBrady ThomasBrady force-pushed the add-per-bucket-entry-size-metrics-2 branch from b18d407 to f95a78c Compare September 5, 2024 18:21
SirTyson
SirTyson previously approved these changes Sep 5, 2024
@SirTyson SirTyson added this pull request to the merge queue Sep 17, 2024
Merged via the queue into stellar:master with commit 5711d19 Sep 17, 2024
14 checks passed
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.

Add per-bucket Entry size Metrics
3 participants