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

Update vendored mimir-prometheus #8305

Merged
merged 9 commits into from
Jun 10, 2024
Merged

Update vendored mimir-prometheus #8305

merged 9 commits into from
Jun 10, 2024

Conversation

pracucci
Copy link
Collaborator

@pracucci pracucci commented Jun 7, 2024

What this PR does

Updating vendored mimir-prometheus, getting the changes synched in grafana/mimir-prometheus#641 (see list of changes there).

Notes:

While working on this PR I've found a couple of issue, whose fixes have already merged upstream:

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.

Offset: selector.Offset,
OriginalOffset: selector.OriginalOffset,
Timestamp: copyTimestamp(selector.Timestamp),
SkipHistogramBuckets: selector.SkipHistogramBuckets,
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Note to reviewers, in particular @krajorama: shardVectorSelector() is meant to copy the input VectorSelector, only modifying the LabelMatchers. For this reason, I think it's safer (and more consistent) to also copy SkipHistogramBuckets.

Copy link
Contributor

Choose a reason for hiding this comment

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

From how the skip histogram bucket works, you cannot really break things by omitting it. Omitting it would just mean that we copy buckets as well in the histogram iterator - so more data.

Conversely, adding it would only break if in the sharded query we started to look at buckets suddenly, which would mean adding a function call like histogram_quantile, histogram_fraction, histogram_stddev, histogram_stdvar. But we never do, we just modify the query to add sum(), which is safe.

Copy link
Contributor

Choose a reason for hiding this comment

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

But this question does point out that we need a generic way to handle optimizations on the AST so we can just re-run the optimization without having to worry about this on a case by case basis @beorn7

Copy link
Contributor

Choose a reason for hiding this comment

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

No better insights to share from my side, unfortunately. It sounds to me we have to check optimization on a case by case basis to see how they interact with sharded query evaluation.

@pracucci
Copy link
Collaborator Author

pracucci commented Jun 7, 2024

I think I've found an issue in prometheus/common which is causing one of our Alertmanager tests to fail. See:
prometheus/common#651

@@ -36,6 +36,7 @@ const (
"http_config": {
"enable_http2": true,
"follow_redirects": true,
"http_headers": null,
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Note to reviewers: this is the default value. Added so that the check done by TestMultitenantAlertmanager_GetUserGrafanaConfig pass.

Signed-off-by: Marco Pracucci <marco@pracucci.com>
Signed-off-by: Marco Pracucci <marco@pracucci.com>
Signed-off-by: Marco Pracucci <marco@pracucci.com>
Signed-off-by: Marco Pracucci <marco@pracucci.com>
Signed-off-by: Marco Pracucci <marco@pracucci.com>
Signed-off-by: Marco Pracucci <marco@pracucci.com>
Signed-off-by: Marco Pracucci <marco@pracucci.com>
Signed-off-by: Marco Pracucci <marco@pracucci.com>
Signed-off-by: Marco Pracucci <marco@pracucci.com>
@krajorama krajorama self-requested a review June 10, 2024 06:54
github.com/dennwc/varint v1.0.0
github.com/golang/groupcache v0.0.0-20210331224755-41bb18bfe9da
github.com/google/go-cmp v0.6.0
github.com/google/go-github/v57 v57.0.0
github.com/google/uuid v1.6.0
github.com/grafana-tools/sdk v0.0.0-20220919052116-6562121319fc
github.com/grafana/alerting v0.0.0-20240605124151-5d695b88086a
github.com/grafana/regexp v0.0.0-20221122212121-6b5c0a4cb7fd
github.com/grafana/regexp v0.0.0-20240518133315-a468a5bfb3bc
Copy link
Contributor

Choose a reason for hiding this comment

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

No effect due to replace directive with v0.0.0-20240531075221-3685f1377d7b

Copy link
Contributor

Choose a reason for hiding this comment

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

looks ok. what we're using is coming from newer optimization branch by @bboreham

github.com/hashicorp/golang-lru/v2 v2.0.7
github.com/hashicorp/vault/api v1.10.0
github.com/mitchellh/colorstring v0.0.0-20190213212951-d06e56a500db
github.com/munnerz/goautoneg v0.0.0-20191010083416-a7dc8b61c822
github.com/prometheus/procfs v0.12.0
github.com/prometheus/procfs v0.15.1
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a big jump and the library does not guarantee backwards compatibility. That said, their release notes do not call out any breaking change.

@pracucci pracucci marked this pull request as ready for review June 10, 2024 08:03
@pracucci pracucci requested review from stevesg, grafanabot and a team as code owners June 10, 2024 08:03
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

@pracucci pracucci merged commit c1bbd94 into main Jun 10, 2024
29 checks passed
@pracucci pracucci deleted the update-mimir-prometheus branch June 10, 2024 08:31
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.

3 participants