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

Add support for ingesting out-of-order native histograms #7175

Merged
merged 10 commits into from
Sep 30, 2024

Conversation

fionaliao
Copy link
Contributor

@fionaliao fionaliao commented Jan 19, 2024

This PR adds OOO native histogram functionality to Mimir.

The core OOO logic is within prometheus and was synced to mimir-prometheus in grafana/mimir-prometheus#703. This PR just adds a flag to Mimir for enabling out-of-order native histograms and some tests.

What this PR does

Which issue(s) this PR fixes or relates to

Fixes #9421

Checklist

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

@fionaliao fionaliao force-pushed the fl/add-ooo-native-histograms-flag branch from ca56ddb to a0f7d19 Compare January 23, 2024 18:21
@fionaliao fionaliao force-pushed the fl/add-ooo-native-histograms-flag branch 2 times, most recently from 9e8d017 to ea66cc9 Compare February 6, 2024 15:11
@fionaliao fionaliao force-pushed the fl/add-ooo-native-histograms-flag branch from ea66cc9 to 1a4244a Compare February 20, 2024 11:53
@fionaliao fionaliao force-pushed the fl/add-ooo-native-histograms-flag branch from aa4b114 to b966932 Compare September 25, 2024 18:48
@fionaliao fionaliao changed the title [WIP] Add OOO native histograms flag [WIP] Add support for ingesting out-of-order native histograms Sep 25, 2024
@fionaliao fionaliao marked this pull request as ready for review September 25, 2024 19:27
@fionaliao fionaliao changed the title [WIP] Add support for ingesting out-of-order native histograms Add support for ingesting out-of-order native histograms Sep 25, 2024
@@ -3284,6 +3284,12 @@ The `limits` block configures default and per-tenant limits imposed by component
# CLI flag: -ingester.native-histograms-ingestion-enabled
[native_histograms_ingestion_enabled: <boolean> | default = false]

# (experimental) Enable experimental out-of-order native histogram ingestion.
# This will only take effect if ingester.out-of-order-time-window is > 0 and if
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
# This will only take effect if ingester.out-of-order-time-window is > 0 and if
# This only takes effect if the `ingester.out-of-order-time-window` value is greater than zero and if

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@@ -3284,6 +3284,12 @@ The `limits` block configures default and per-tenant limits imposed by component
# CLI flag: -ingester.native-histograms-ingestion-enabled
[native_histograms_ingestion_enabled: <boolean> | default = false]

# (experimental) Enable experimental out-of-order native histogram ingestion.
# This will only take effect if ingester.out-of-order-time-window is > 0 and if
# ingester.native-histograms-ingestion-enabled = true
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
# ingester.native-histograms-ingestion-enabled = true
# `ingester.native-histograms-ingestion-enabled = true`.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

@krajorama krajorama left a comment

Choose a reason for hiding this comment

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

Awesome stuff! I only have comments on the testing side and want to investigate a little bit if there's impact on compaction - so might have some more comments later.

integration/native_histograms_test.go Outdated Show resolved Hide resolved
integration/native_histograms_test.go Outdated Show resolved Hide resolved
integration/native_histograms_test.go Outdated Show resolved Hide resolved
integration/ooo_ingestion_test.go Show resolved Hide resolved
pkg/ingester/ingester_test.go Show resolved Hide resolved
@narqo
Copy link
Contributor

narqo commented Sep 27, 2024

The CHANGELOG has just been cut to prepare for the next release. Please rebase main and eventually move the CHANGELOG entry added / updated in this PR to the top of the CHANGELOG.md document. Thanks!

@fionaliao fionaliao force-pushed the fl/add-ooo-native-histograms-flag branch from 80bf6fe to 0887714 Compare September 27, 2024 15:56
Copy link
Contributor

@krajorama krajorama left a comment

Choose a reason for hiding this comment

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

LGTM

@krajorama krajorama merged commit f2cff61 into main Sep 30, 2024
29 checks passed
@krajorama krajorama deleted the fl/add-ooo-native-histograms-flag branch September 30, 2024 09:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

vendor out of order support from prometheus
4 participants