Use grafana URL to test receivers #9125
Conversation
|
|
titolins
force-pushed
the
titolins/use_grafana_url_on_templates
branch
from
38bd360 to
7b6cbe8
Compare
pkg/alertmanager/alertmanager.go
Outdated
| @@ -92,6 +92,7 @@ type Config struct { | |||
| Retention time.Duration | |||
| MaxConcurrentGetRequestsPerTenant int | |||
| ExternalURL *url.URL | |||
| TmplExternalURL *url.URL | |||
There was a problem hiding this comment.
This new field is not "config" in the same sense as the other fields in the struct, i.e. global config taken from a YAML file and applied to all Alertmanagers.
I think the Alertmanager struct would be a better fit for this field (you could add tmplExternalURL here).
There was a problem hiding this comment.
Got it! Changed it now 👍
pkg/alertmanager/multitenant.go
Outdated
| @@ -901,6 +901,7 @@ func (am *MultitenantAlertmanager) newAlertmanager(userID string, amConfig *defi | |||
| Retention: am.cfg.Retention, | |||
| MaxConcurrentGetRequestsPerTenant: am.cfg.MaxConcurrentGetRequestsPerTenant, | |||
| ExternalURL: am.cfg.ExternalURL.URL, | |||
| TmplExternalURL: tmplExternalURL, | |||
There was a problem hiding this comment.
The external URL can change when a new config is applied. For example, consider the following scenario:
- A user has no Grafana configuration, they are using Mimir's external URL
- They upload some Grafana configuration, they should be using Grafana's external URL
We should assign the correct external URL on every config sync (check here, we're updating the templates in the Alertmanager struct, we could use the same lock to update the external URL).
There was a problem hiding this comment.
Ah yeah, ofc.. I missed the else and presumed a new instance would be built every time. Tks, fixed now 👍
titolins
force-pushed
the
titolins/use_grafana_url_on_templates
branch
from
b91b073 to
66c1ca4
Compare
pkg/alertmanager/alertmanager.go
Outdated
| @@ -191,7 +192,7 @@ type Replicator interface { | |||
| } | |||
|
|
|||
| // New creates a new Alertmanager. | |||
| func New(cfg *Config, reg *prometheus.Registry) (*Alertmanager, error) { | |||
| func New(cfg *Config, reg *prometheus.Registry, tmplExternalURL *url.URL) (*Alertmanager, error) { | |||
There was a problem hiding this comment.
If we're already passing the URL in ApplyConfig(), we can skip adding a new parameter here. On the initial sync (right after the Alertmanager is created), the AM will get the correct URL to be used when testing receivers.
I'd remove this new parameter.
There was a problem hiding this comment.
Ah, good catch! Removed now 👍
titolins
force-pushed
the
titolins/use_grafana_url_on_templates
branch
from
af62cde to
a113290
Compare
Fixes https://github.com/grafana/alerting-squad/issues/872
As described, alerts should use the Grafana URL instead of the remote AM's. For regular alert notifications this was already done, this PR fixes for tests