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

feat: Do not add empty blooms to offsets #14577

Merged
merged 4 commits into from
Oct 25, 2024

Conversation

salvacorts
Copy link
Contributor

@salvacorts salvacorts commented Oct 22, 2024

What this PR does / why we need it:

Series w/o structured metadata are appended to blocks with empty blooms, even though empty blooms are small, this adds up among all the blocks. This also prevent us from experimenting with single-stream blocks since we'd end up with many tiny blocks with empty blooms.

This PR solves this by only appending series to the index section if the series yield empty blooms, but not populating the offsets for the series. Now if we want to try out single-stream blocks, we can:

  1. Set a really small block size target, so adding a single populated bloom will exceed the max size
  2. Series with empty blooms will be part of a blocks that spans multiple series but where only a single series actually has a bloom.

This is obviously not optimal, but will allow us to test out single-stream blocks with what we currently have.

Besides allowing us to test single-stream blocks, this PR would also be beneficial to reduce the total size of the blocks by not appending empty blooms at all.

This PR also fixes the maximum block size since we did not look at the unflushed index and blooms pages size.

Checklist

  • Reviewed the CONTRIBUTING.md guide (required)
  • Documentation added
  • Tests updated
  • Title matches the required conventional commits format, see here
    • Note that Promtail is considered to be feature complete, and future development for logs collection will be in Grafana Alloy. As such, feat PRs are unlikely to be accepted unless a case can be made for the feature actually being a bug fix to existing behavior.
  • Changes that require user attention or interaction to upgrade are documented in docs/sources/setup/upgrade/_index.md
  • If the change is deprecating or removing a configuration option, update the deprecated-config.yaml and deleted-config.yaml files respectively in the tools/deprecated-config-checker directory. Example PR

@salvacorts salvacorts force-pushed the salvacorts/do-not-append-empty-blooms branch from 83d42fc to 29004a2 Compare October 22, 2024 14:33
@pull-request-size pull-request-size bot added size/L and removed size/M labels Oct 23, 2024
@salvacorts salvacorts marked this pull request as ready for review October 23, 2024 14:22
@salvacorts salvacorts requested a review from a team as a code owner October 23, 2024 14:22
@salvacorts salvacorts force-pushed the salvacorts/do-not-append-empty-blooms branch from 34fc4fc to aa75e5f Compare October 25, 2024 10:44
@salvacorts salvacorts mentioned this pull request Oct 25, 2024
6 tasks

// remove blooms from series
for i := range series {
series[i].Blooms = v2.NewSliceIter([]*Bloom{})
Copy link
Contributor

Choose a reason for hiding this comment

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

nit

Suggested change
series[i].Blooms = v2.NewSliceIter([]*Bloom{})
series[i].Blooms = v2.NewEmptyIter[*Bloom]()

@salvacorts salvacorts merged commit 51c42e8 into main Oct 25, 2024
60 checks passed
@salvacorts salvacorts deleted the salvacorts/do-not-append-empty-blooms branch October 25, 2024 14:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants