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

Vendor update mimir-prometheus at af2a1cb10c89de496cd4309ac532624b34112d74 #10188

Merged
merged 8 commits into from
Dec 11, 2024

Conversation

krajorama
Copy link
Contributor

@krajorama krajorama commented Dec 9, 2024

What this PR does

This is upstream at
https://github.com/prometheus/prometheus/tree/af2a1cb10c89de496cd4309ac532624b34112d74

Changes listed in
grafana/mimir-prometheus#787

Fixed query sharding test TestQuerySharding_FunctionCorrectness failing due to prometheus/prometheus#15479.

Which issue(s) this PR fixes or relates to

N/A

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

What if we introduced something like InstantVectorSeriesDataIterator, but for iterating over a RangeVectorStepData? I think that would simplify a lot of the logic here, and more clearly separate the iteration logic from the resets / changes logic.

Copy link
Contributor

Choose a reason for hiding this comment

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

I had thought about that too. I'm happy to do it, but shall we do it as a follow up PR?

Copy link
Contributor

Choose a reason for hiding this comment

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

Up to you, I'm happy either way.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'll do it as a followup 👍

@jesusvazquez
Copy link
Member

jesusvazquez commented Dec 10, 2024

I'm trying to add two reverts here #10203 maybe we could combine the effort @krajorama

Copy link
Contributor

@charleskorn charleskorn left a comment

Choose a reason for hiding this comment

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

Approving MQE changes to unblock this, but I'd like to see the change suggested here applied in a follow up PR, and I'd also like to see us use the exported functions from prometheus/prometheus#15190 in a follow up PR as well.

No need to test query sharding on time functions for native histograms
since prometheus/prometheus#15479 . As those
functions do not return samples on native histograms.

Signed-off-by: György Krajcsovits <[email protected]>
Looked through changes from #10102, #10123, #10136, #10168, #10188

Signed-off-by: György Krajcsovits <[email protected]>
@krajorama krajorama marked this pull request as ready for review December 11, 2024 07:33
@krajorama krajorama requested review from stevesg, grafanabot and a team as code owners December 11, 2024 07:33
@@ -103,6 +103,15 @@
* [BUGFIX] Querier: Fix stddev+stdvar aggregations to treat Infinity consistently. #9844
* [BUGFIX] Ingester: Chunks could have one unnecessary zero byte at the end. #9844
* [BUGFIX] OTLP receiver: Preserve colons and combine multiple consecutive underscores into one when generating metric names in suffix adding mode (`-distributor.otel-metric-suffixes-enabled`). #10075
* [BUGFIX] PromQL: Ignore native histograms in `clamp`, `clamp_max` and `clamp_min` functions. #10136
Copy link
Contributor Author

@krajorama krajorama Dec 11, 2024

Choose a reason for hiding this comment

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

Note to reviewers: native histograms is an experimental feature, so these don't count as CHANGE as they just fix the code to follow the spec. @beorn7

@@ -988,15 +988,8 @@ func TestQuerySharding_FunctionCorrectness(t *testing.T) {

testsForBoth := []queryShardingFunctionCorrectnessTest{
{fn: "count_over_time", rangeQuery: true},
{fn: "days_in_month"},
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note to reviewers: due to prometheus/prometheus#15479

@krajorama
Copy link
Contributor Author

I'm trying to add two reverts here #10203 maybe we could combine the effort @krajorama

We'll follow up in separate PR. Let this be pure Prometheus upstream update.

CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Show resolved Hide resolved
Copy link
Contributor

@jhesketh jhesketh left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

Copy link
Member

@jesusvazquez jesusvazquez left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@beorn7 beorn7 left a comment

Choose a reason for hiding this comment

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

Some small things…

CHANGELOG.md Outdated
@@ -103,6 +103,15 @@
* [BUGFIX] Querier: Fix stddev+stdvar aggregations to treat Infinity consistently. #9844
* [BUGFIX] Ingester: Chunks could have one unnecessary zero byte at the end. #9844
* [BUGFIX] OTLP receiver: Preserve colons and combine multiple consecutive underscores into one when generating metric names in suffix adding mode (`-distributor.otel-metric-suffixes-enabled`). #10075
* [BUGFIX] PromQL: Ignore native histograms in `clamp`, `clamp_max` and `clamp_min` functions. #10136
* [BUGFIX] PromQL: Ignore native histograms in `max`, `min`, `stdvar`, `stddev` functions and instead return an info annotation. #10136
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: These are aggregation operators, not functions.

CHANGELOG.md Outdated
* [BUGFIX] PromQL: Ignore native histograms in `max`, `min`, `stdvar`, `stddev` functions and instead return an info annotation. #10136
* [BUGFIX] PromQL: Ignore native histograms when compared to float values with `==`, `!=`, `<`, `>`, `<=`, `>=` and instead return an info annotation. #10136
* [BUGFIX] PromQL: Return an info annotation if the `quantile` function is used on a float series that does not have `le` label. #10136
* [BUGFIX] PromQL: Fix `count_values` not taking into account native histograms. #10168
Copy link
Contributor

Choose a reason for hiding this comment

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

count_values does take native histograms into account with the recent fix. See prometheus/prometheus#15422

Copy link
Contributor Author

Choose a reason for hiding this comment

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

lol, unfortunate wording :)

CHANGELOG.md Outdated
* [BUGFIX] PromQL: Ignore native histograms when compared to float values with `==`, `!=`, `<`, `>`, `<=`, `>=` and instead return an info annotation. #10136
* [BUGFIX] PromQL: Return an info annotation if the `quantile` function is used on a float series that does not have `le` label. #10136
* [BUGFIX] PromQL: Fix `count_values` not taking into account native histograms. #10168
* [BUGFIX] PromQL: Ignore native histograms in time functions `day_of_month`, `day_of_week`, `day_of_year`, `days_in_month`, `hour`, `minute`, `month` and `year`, which means they no longer any value when encountering a native histograms series. #10188
Copy link
Contributor

Choose a reason for hiding this comment

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

A word is missing here, like "yield" or "return".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@beorn7 updated the descriptions, thank you.

Signed-off-by: György Krajcsovits <[email protected]>
Copy link
Contributor

@beorn7 beorn7 left a comment

Choose a reason for hiding this comment

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

For NH/PromQL part.

# Conflicts:
#	pkg/distributor/otel_test.go
@krajorama krajorama enabled auto-merge (squash) December 11, 2024 11:33
@krajorama krajorama merged commit 06689ae into main Dec 11, 2024
29 checks passed
@krajorama krajorama deleted the krajo/update-mimir-prometheus branch December 11, 2024 11:53
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.

6 participants