Bump mimir-prometheus after merge with upstream #1695

jesusvazquez · 2022-04-13T13:19:06Z

What this PR does

We've merged prometheus/main into mimir-prometheus/main in
grafana/mimir-prometheus#199.

This commit brings those changes by updating the vendored dependency of
mimir-prometheus.

Also brings the changes from
grafana/mimir-prometheus@db8c550
which fix a memory leak.

cc @pracucci you asked me to help with this please take a look 👍

Which issue(s) this PR fixes or relates to

No related issue, simply bumping mimir-prometheus

Checklist

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

jesusvazquez · 2022-04-13T13:23:55Z

I'm seeing some errors in the build so I'm taking a look 👀

jesusvazquez · 2022-04-13T14:43:46Z

I suggest reviewing this commit by commit knowing that the first one brings all the changes from the vendor update.

pracucci · 2022-04-14T11:44:44Z

We may want to wait for prometheus/prometheus#10590

jesusvazquez · 2022-04-15T10:55:34Z

@pracucci I've changed the first commit in this branch to include the latest changes from mimir-prometheus. As soon as CI passes I think we're good to go.

colega · 2022-04-21T09:15:43Z

vendor/github.com/grafana/regexp/README.md

@@ -0,0 +1,12 @@
+# Grafana Go regexp package


I'm surprised to see that this wasn't a dependency before. This is a relevant change.

I think we should migrate Mimir to grafana/regexp too. I will do it in a separate PR.

colega · 2022-04-21T10:07:33Z

vendor/github.com/prometheus/common/config/http_config.go

@@ -42,6 +39,7 @@ import (
 // DefaultHTTPClientConfig is the default HTTP client configuration.
 var DefaultHTTPClientConfig = HTTPClientConfig{
 	FollowRedirects: true,
+	EnableHTTP2:     true,


This replaces the previous env var PROMETHEUS_COMMON_DISABLE_HTTP2 below. But afaik we're not using this.

colega · 2022-04-21T10:09:14Z

vendor/github.com/prometheus/alertmanager/api/api.go

 		apiPrefix+"/api/v2/",
-		api.limitHandler(http.StripPrefix(apiPrefix+"/api/v2", api.v2.Handler)),
+		api.limitHandler(http.StripPrefix(apiPrefix, api.v2.Handler)),


This looks like a bugfix. Are we exposing /api/v2 ?

colega · 2022-04-21T10:15:35Z

I have reviewed both vendored changes (with a medium-depth review) and Mimir changes to adapt to vendored changes and both look good to me.

pracucci · 2022-04-21T15:24:17Z

While reviewing this PR I've noticed we were upgrading memberlist too and that was unexpected:
#1738

pracucci · 2022-04-21T15:49:57Z

github.com/prometheus/alertmanager from v0.23.1-0.20210914172521-e35efbddb66a to v0.24.0

There are two relevant changes for us. Telegram receiver has been introduced and OpsGenie APIKeyFile config option. We need to add Telegram support and both of them to our firewall.

I'm working on it. Will commit changes directly to this PR.

pracucci · 2022-04-21T17:05:24Z

github.com/prometheus/alertmanager from v0.23.1-0.20210914172521-e35efbddb66a to v0.24.0

There are two relevant changes for us. Telegram receiver has been introduced and OpsGenie APIKeyFile config option. We need to add Telegram support and both of them to our firewall.

Done in this commit: 3b60c87

I will keep reviewing this PR tomorrow.

We've merged prometheus/main into mimir-prometheus/main in grafana/mimir-prometheus#199. This commit brings those changes by updating the vendored dependency of mimir-prometheus. Also brings the changes from grafana/mimir-prometheus@db8c550 which fix a memory leak. Signed-off-by: Jesus Vazquez <jesus.vazquez@grafana.com>

Upstream prometheus rules manager Update() method has an extra argument to execute a function before each group rule evaluation. I've extended the interface to include this argument. Also updated all the callers to pass nil meaning the function won't be run. Signed-off-by: Jesus Vazquez <jesus.vazquez@grafana.com>

The constructos for both NewRangeQuery and NewInstantQuery now accept QueryOps. So far there is only one opt called EnablePerStepStats which is disabled by default so I figured for now we could pass nil. Signed-off-by: Jesus Vazquez <jesus.vazquez@grafana.com>

The initialization constructor of v1.NewAPI now accepts a StatsRendered. I assume we're not using this yet so I'm passing nil. Signed-off-by: Jesus Vazquez <jesus.vazquez@grafana.com>

When initializing v1.NewAPI we're passing a compiled Regexp that unmatches the expected type becuase we were importing "regexp" instead of "github.com/grafana/regexp". This commit makes it consistent by replacing "regexp" with "github.com/grafana/regexp" and updating go mod. Signed-off-by: Jesus Vazquez <jesus.vazquez@grafana.com>

…file config option Signed-off-by: Marco Pracucci <marco@pracucci.com>

Signed-off-by: Marco Pracucci <marco@pracucci.com>

pracucci · 2022-04-22T07:59:25Z

github.com/prometheus/common from v0.32.1 to v0.33.0

This upgrade added OAuth2 proxy URL support in prometheus/common#358. We need to block it through our firewall.

Done in c2d0ca6

pracucci · 2022-04-22T08:29:53Z

Prometheus upgrade

👍

Notes:

PromQL added support for EnablePerStepStats. It's disabled by default and it's fine to always keep it disabled in Mimir for now.
I checked changes introduced for RuleGroupPostProcessFunc support in the rules evaluation (PR) and LGTM
TSDB DefaultWriteQueueSize changed from 1000 to 0: doesn't impact us because we never use the default but override it explicitly.

pracucci · 2022-04-22T08:47:03Z

google.golang.org/grpc from v1.43.0 to v1.45.0

Checked the release notes and I can't see any change that might affect us.

pracucci · 2022-04-22T08:49:01Z

I also checked diff for the following vendored libs and LGTM:

vendor/cloud.google.com/go/
vendor/github.com/Azure/go-autorest
vendor/github.com/asaskevich/govalidator
vendor/github.com/go-logr/logr
vendor/github.com/google/pprof
vendor/github.com/hashicorp/go-multierror
vendor/github.com/aws/aws-sdk-go/aws
vendor/google.golang.org/protobuf/
vendor/google.golang.org/api/

jesusvazquez requested a review from pracucci April 13, 2022 13:19

jesusvazquez force-pushed the jvp/bump-mimir-prometheus-after-merge-with-upstream-prometheus branch from 9e77502 to cca25ba Compare April 13, 2022 13:23

jesusvazquez force-pushed the jvp/bump-mimir-prometheus-after-merge-with-upstream-prometheus branch 3 times, most recently from a26e007 to 1527169 Compare April 13, 2022 14:03

jesusvazquez force-pushed the jvp/bump-mimir-prometheus-after-merge-with-upstream-prometheus branch from 860792b to 34279a8 Compare April 15, 2022 10:50

jesusvazquez force-pushed the jvp/bump-mimir-prometheus-after-merge-with-upstream-prometheus branch from 34279a8 to 5bbabf7 Compare April 20, 2022 10:39

colega reviewed Apr 21, 2022

View reviewed changes

colega approved these changes Apr 21, 2022

View reviewed changes

colega mentioned this pull request Apr 21, 2022

Update mimir-prometheus to 19bfb1e6e195 #1734

Merged

3 tasks

jesusvazquez and others added 6 commits April 22, 2022 09:48

NewAPI now receives a StatsRenderer

2ac7604

The initialization constructor of v1.NewAPI now accepts a StatsRendered. I assume we're not using this yet so I'm passing nil. Signed-off-by: Jesus Vazquez <jesus.vazquez@grafana.com>

Added Alertmanager Telegram support and blocked new OpsGenie api_key_…

cf49e37

…file config option Signed-off-by: Marco Pracucci <marco@pracucci.com>

pracucci force-pushed the jvp/bump-mimir-prometheus-after-merge-with-upstream-prometheus branch from 3b60c87 to cf49e37 Compare April 22, 2022 07:49

Do not allow to set OAuth2 proxy_url

c2d0ca6

Signed-off-by: Marco Pracucci <marco@pracucci.com>

pracucci merged commit a6cdf5c into main Apr 22, 2022

pracucci deleted the jvp/bump-mimir-prometheus-after-merge-with-upstream-prometheus branch April 22, 2022 08:57

pracucci mentioned this pull request Apr 22, 2022

Use grafana/regexp instead of regexp package #1745

Merged

3 tasks

pracucci mentioned this pull request Aug 12, 2022

Fix reporting of tracing spans from PromQL engine #2707

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bump mimir-prometheus after merge with upstream #1695

Bump mimir-prometheus after merge with upstream #1695

jesusvazquez commented Apr 13, 2022 •

edited

Loading

jesusvazquez commented Apr 13, 2022

jesusvazquez commented Apr 13, 2022 •

edited

Loading

pracucci commented Apr 14, 2022

jesusvazquez commented Apr 15, 2022

colega Apr 21, 2022

pracucci Apr 22, 2022

colega Apr 21, 2022

colega Apr 21, 2022

colega commented Apr 21, 2022

pracucci commented Apr 21, 2022

pracucci commented Apr 21, 2022

pracucci commented Apr 21, 2022

pracucci commented Apr 22, 2022

pracucci commented Apr 22, 2022

pracucci commented Apr 22, 2022

pracucci commented Apr 22, 2022

Bump mimir-prometheus after merge with upstream #1695

Bump mimir-prometheus after merge with upstream #1695

Conversation

jesusvazquez commented Apr 13, 2022 • edited Loading

What this PR does

Which issue(s) this PR fixes or relates to

Checklist

jesusvazquez commented Apr 13, 2022

jesusvazquez commented Apr 13, 2022 • edited Loading

pracucci commented Apr 14, 2022

jesusvazquez commented Apr 15, 2022

colega Apr 21, 2022

Choose a reason for hiding this comment

pracucci Apr 22, 2022

Choose a reason for hiding this comment

colega Apr 21, 2022

Choose a reason for hiding this comment

colega Apr 21, 2022

Choose a reason for hiding this comment

colega commented Apr 21, 2022

pracucci commented Apr 21, 2022

pracucci commented Apr 21, 2022

pracucci commented Apr 21, 2022

pracucci commented Apr 22, 2022

pracucci commented Apr 22, 2022

pracucci commented Apr 22, 2022

pracucci commented Apr 22, 2022

jesusvazquez commented Apr 13, 2022 •

edited

Loading

jesusvazquez commented Apr 13, 2022 •

edited

Loading