-
Notifications
You must be signed in to change notification settings - Fork 542
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
Alertmanager: Update to latest main #7103
Conversation
cb7101c
to
2a9e1d7
Compare
04a8da2
to
25a22d1
Compare
I tested this commit in Here is a screenshot showing the version on startup: Test 1: mimirtool works as expected with non UTF-8 configurations
route:
receiver: default
routes:
- continue: false
matchers:
- foo=bar
- bar=baz
mute_time_intervals: []
receiver: ""
routes: []
and the configuration can be seen in Grafana: Test 2: UTF-8 configurations are rejectedUsing a modified version of mimirtool that supports UTF-8, it cannot load UTF-8 configurations as the request is rejected in the API: route:
receiver: default
routes:
- continue: false
matchers:
- foo=bar
- bar=baz
- baz="\xf0\x9f\x99\x82"
- bar🙂=baz
mute_time_intervals: []
receiver: ""
routes: []
The input Test 3: amtool works as expected with non UTF-8 alerts and silencesamtool can create alerts and silences without UTF-8 as normal:
Test 4: UTF-8 alerts and silences are rejectedA modified version of amtool that supports UTF-8 cannot create alerts or silences containing UTF-8 as these are rejected in the API:
Test 5: UTF-8 is allowed on the right hand sideOf course, UTF-8 is allowed on the right hand side (either as the value of a label or right hand side of a matcher). There is no change here, and this is how Alertmanager works (we are adding support for UTF-8 on both sides). For example: route:
receiver: default
routes:
- continue: false
matchers:
- foo=🙂bar
- bar=🙂baz
mute_time_intervals: []
receiver: ""
routes: []
|
Test 6: APIv1 has been deprecatedHere is a test showing that APIv1 has been deprecated in
and here is
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you add a CHANGELOG entry to describe the changes (including the removal of API v1), please?
25a22d1
to
e8e0f62
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I went very quickly through the changes and I don't see anything concerning from Mimir side. I let @gotjosh do the proper review. Thanks!
Updated Alertmanager to commit |
@gotjosh something I thought about is whether we should also add this change in this PR instead of #6898: diff --git a/vendor/github.com/grafana/mimir/pkg/mimir/modules.go b/vendor/github.com/grafana/mimir/pkg/mimir/modules.go
index 58a03273e..9cc23ae3c 100644
--- a/vendor/github.com/grafana/mimir/pkg/mimir/modules.go
+++ b/vendor/github.com/grafana/mimir/pkg/mimir/modules.go
@@ -26,6 +26,8 @@ import (
"github.com/opentracing-contrib/go-stdlib/nethttp"
"github.com/opentracing/opentracing-go"
"github.com/pkg/errors"
+ "github.com/prometheus/alertmanager/featurecontrol"
+ "github.com/prometheus/alertmanager/matchers/compat"
"github.com/prometheus/client_golang/prometheus"
"github.com/prometheus/common/config"
"github.com/prometheus/prometheus/model/labels"
@@ -881,6 +883,10 @@ func (t *Mimir) initRuler() (serv services.Service, err error) {
}
func (t *Mimir) initAlertManager() (serv services.Service, err error) {
+ f, err := featurecontrol.NewFlags(util_log.Logger, featurecontrol.FeatureClassicMode)
+ util_log.CheckFatal("initializing Alertmanager feature flags", err)
+ compat.InitFromFlags(util_log.Logger, compat.RegisteredMetrics, f)
+
t.Cfg.Alertmanager.ShardingRing.Common.ListenPort = t.Cfg.Server.GRPCListenPort
t.Cfg.Alertmanager.CheckExternalURL(t.Cfg.API.AlertmanagerHTTPPrefix, util_log.Logger) There will be no change in behavior. The difference is that we initialize the compat package with a logger, so we will get debug level logs. The default variables use |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall LGTM, but please see my comments.
Title: `{{ template "msteams.default.title" . }}`, | ||
Text: `{{ template "msteams.default.text" . }}`, | ||
Title: `{{ template "msteams.default.title" . }}`, | ||
Summary: `{{ template "msteams.default.summary" . }}`, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gillesdemey one for you - the msteams integrations for the cloud alertmanager now has a new configuration option, we need to support summary
.
InvalidTotal *prometheus.GaugeVec | ||
} | ||
|
||
func NewMetrics(r prometheus.Registerer) *Metrics { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see these metrics being exposed as part of the alertmanager metrics neither here or in #6898, just to confirm you don't want to expose these yet, correct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Those were available in dev when I tested just from the default registrar. Should we use another registrar? If so, we can initialize the compat package #7103 (comment).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems Mimir uses the default registerer. If I attempt to re-register the metrics using the registerer in initAlertManager
then I get a panic: duplicate metrics collector registration attempted
.
This commit updates Alertmanager in Mimir. It deprecates APIv1, replacing all APIv1 endpoints with a deprecation notice. The version of Alertmanager vendored in this commit contains support for UTF-8, however is disabled until we initialize the compat package from within Mimir using compat.InitFromFlags. UTF-8 will be enabled at a later time.
153a8df
to
5e7d1ff
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
What this PR does
This pull request updates Alertmanager in Mimir. It deprecates Alertmanager APIv1. The version of Alertmanager vendored in this commit contains support for UTF-8, however is disabled until we initialize the compat package from within Mimir using
compat.InitFromFlags
. UTF-8 will be enabled at a later time.Checklist
CHANGELOG.md
updated - the order of entries should be[CHANGE]
,[FEATURE]
,[ENHANCEMENT]
,[BUGFIX]
.about-versioning.md
updated with experimental features.