config¬ifier: Add option to use Alertmanager API v2 #5482
Conversation
notifier/notifier.go
Outdated
| @@ -490,13 +513,40 @@ func (n *Manager) sendAll(alerts ...*Alert) bool { | |||
| wg.Done() | |||
| }(ams, am) | |||
| } | |||
| ams.mtx.RUnlock() | |||
There was a problem hiding this comment.
This would unlock the read lock, even though the spawned go routines still had references to ams, resulting in possible race conditions. Instead, the lock is dropped at the end of the function via defer now. This is safe as the function waits for all spawned goroutines before exiting.
There was a problem hiding this comment.
Another option would be to still release the lock here and pass the necessary inputs to the goroutine:
ams.mtx.RLock()
for _, am := range ams.ams {
...
go func(c *http.Client, u string) {
if err := n.sendOne(ctx, c, u, b); err != nil {
...
}
}(ams.client, am.url().String())
}
ams.mtx.RUnlock()There was a problem hiding this comment.
That's even better, thanks @simonpasquier.
| "github.com/pkg/errors" | ||
| "github.com/prometheus/client_golang/prometheus" | ||
| config_util "github.com/prometheus/common/config" | ||
| "github.com/prometheus/common/model" | ||
| "github.com/prometheus/common/version" | ||
|
|
||
| "github.com/prometheus/alertmanager/api/v2/models" |
There was a problem hiding this comment.
This patch vendors the Alertmanager API v2 model. From my point of view, this gives us a strong contract between the two APIs. Let me know what you think.
config/config.go
Outdated
| return err | ||
| } | ||
|
|
||
| // Default to Alertmanager API v1. |
There was a problem hiding this comment.
This isn't how we generally implement defaults.
There was a problem hiding this comment.
Changed to the default way of doing defaults. Thanks.
notifier/notifier.go
Outdated
| @@ -490,13 +513,40 @@ func (n *Manager) sendAll(alerts ...*Alert) bool { | |||
| wg.Done() | |||
| }(ams, am) | |||
| } | |||
| ams.mtx.RUnlock() | |||
There was a problem hiding this comment.
Another option would be to still release the lock here and pass the necessary inputs to the goroutine:
ams.mtx.RLock()
for _, am := range ams.ams {
...
go func(c *http.Client, u string) {
if err := n.sendOne(ctx, c, u, b); err != nil {
...
}
}(ams.client, am.url().String())
}
ams.mtx.RUnlock()| ams.mtx.RLock() | ||
| defer ams.mtx.RUnlock() | ||
|
|
||
| switch ams.cfg.APIVersion { |
There was a problem hiding this comment.
Maybe it could be cached to avoid serializing several times the same data.
| "github.com/pkg/errors" | ||
| "github.com/prometheus/client_golang/prometheus" | ||
| config_util "github.com/prometheus/common/config" | ||
| "github.com/prometheus/common/model" | ||
| "github.com/prometheus/common/version" | ||
|
|
||
| "github.com/prometheus/alertmanager/api/v2/models" |
mxinden
force-pushed
the
am-v2
branch
4 times, most recently
from
348ac70
to
585381b
Compare
|
@brian-brazil I updated docs/configuration/configuration.md. Any other docs that I am missing? |
|
There's Alerting -> Clients, but that's not in this repo. |
With v0.16.0 Alertmanager introduced a new API (v2). This patch adds a configuration option for Prometheus to send alerts to the v2 endpoint instead of the defautl v1 endpoint. Signed-off-by: Max Leonard Inden <[email protected]>
|
@simonpasquier @brian-brazil any further thoughts on this? |
|
👍 |
|
Tested this on a larger setup as well. Thanks for the help. |
With v0.16.0 Alertmanager introduced a new API (v2). This patch adds a
configuration option for Prometheus to send alerts to the v2 endpoint
instead of the defautl v1 endpoint.
Fixes #5167