Skip to content

[Rhythm] Block-builder improvement to skip WAL #4647

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 21 commits into from
Feb 6, 2025
Merged

[Rhythm] Block-builder improvement to skip WAL #4647

merged 21 commits into from
Feb 6, 2025

Conversation

mdisibio
Copy link
Contributor

@mdisibio mdisibio commented Jan 31, 2025 •
edited
Loading

What this PR does:
Update the block-builder to skip the WAL and go directly from in-memory live traces to the backend block. This is more efficient and the block-builders have much higher throughput. Memory allocs are improved, but total working must now be large enough to contain the full cycle of data. Overall the higher throughput is the most important for operational reasons.

               │ before.txt  │              after.txt              │
               │   sec/op    │   sec/op     vs base                │
BlockBuilder-4   808.0m ± 1%   374.7m ± 2%  -53.63% (p=0.000 n=10)

               │  before.txt  │               after.txt               │
               │     B/s      │     B/s       vs base                 │
BlockBuilder-4   36.17Mi ± 1%   78.41Mi ± 1%  +116.80% (p=0.000 n=10)

               │  before.txt   │              after.txt               │
               │     B/op      │     B/op      vs base                │
BlockBuilder-4   1165.5Mi ± 2%   839.9Mi ± 2%  -27.94% (p=0.000 n=10)

               │ before.txt  │             after.txt              │
               │  allocs/op  │  allocs/op   vs base               │
BlockBuilder-4   5.773M ± 0%   5.417M ± 0%  -6.17% (p=0.000 n=10)

Other changes:

  • Integrate max trace size check into liveTraces. Block-builders don't track trace sizes across cycles so don't need traceSizes and reduces map overhead.
  • Validate block-builder config on startup.

Which issue(s) this PR fixes:
Fixes #

Checklist

  • Tests updated
  • Documentation added
  • CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

mdisibio added 20 commits January 28, 2025 22:17
@mdisibio
Version of block builder that skips WAL stage for better performance
b42c32a
@mdisibio
flush tenants in parallel
903ee00
@mdisibio
fix bloom filter issue because totalobjects was missing
7e13527
@mdisibio
simplify vparquet loop
03340c7
@mdisibio
snappy compress in memory and fix dedicated columns mapping reuse
5943bda
@mdisibio
vparquet4 occasionally GC while writing blocks
ae1a132
@mdisibio
gcs backend debug logs for written objects
c31ca51
@mdisibio
more pooling and reduce memory
9112e98
@mdisibio
async live traces iter to allow unmarshaling on a separate goroutine,…
e17bda5 
… remove snappy
@mdisibio
Block-builder validate config on startup
5f31485
@mdisibio
Fix error handling
de19f55
@mdisibio
Integrate trace size check into live traces to avoid tokenizing the t…
6489277 
…race ID twice
@mdisibio
Block-builder validate config on startup
2c1279d
@mdisibio
Fix benchmark to exclude allocing test data
c91d547
@mdisibio
code cleanup
0a110ff
@mdisibio
Fix param order
ddd2fcf
@mdisibio
Warn on trace quality, cleanup
e448930
@mdisibio
Merge branch 'main' into bb-nowal
966bdf2
@mdisibio
lint
c3e9728
@mdisibio
Merge branch 'main' into bb-nowal
19f8565
@mdisibio mdisibio marked this pull request as ready for review February 5, 2025 13:20
mapno
mapno reviewed Feb 5, 2025
View reviewed changes
Copy link
Contributor

@mapno mapno left a comment

Choose a reason for hiding this comment

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

I like this changes, but I have mixed feelings about the live traces iterator. It's nondeterministic too much, makes it less predictable.

@mdisibio mdisibio merged commit 545ab11 into grafana:main Feb 6, 2025
14 checks passed
@mdisibio mdisibio mentioned this pull request Feb 6, 2025
Merged
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mapno mapno mapno approved these changes

@joe-elliott joe-elliott Awaiting requested review from joe-elliott joe-elliott is a code owner

@yvrhdn yvrhdn Awaiting requested review from yvrhdn yvrhdn is a code owner

@zalegrala zalegrala Awaiting requested review from zalegrala zalegrala is a code owner

@electron0zero electron0zero Awaiting requested review from electron0zero electron0zero is a code owner

@ie-pham ie-pham Awaiting requested review from ie-pham ie-pham is a code owner

@stoewer stoewer Awaiting requested review from stoewer stoewer is a code owner

@javiermolinar javiermolinar Awaiting requested review from javiermolinar javiermolinar is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@mdisibio @mapno