-
Notifications
You must be signed in to change notification settings - Fork 537
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
Changed -alertmanager.web.external-url default #1067
Conversation
Signed-off-by: Marco Pracucci <[email protected]>
Signed-off-by: Marco Pracucci <[email protected]>
Signed-off-by: Marco Pracucci <[email protected]>
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 - I just have one comment.
# Alertmanager is served via a reverse proxy). Used for generating relative and | ||
# absolute links back to Alertmanager itself. If the URL has a path portion, it | ||
# will be used to prefix all HTTP endpoints served by Alertmanager. | ||
# The URL under which Alertmanager is externally reachable (eg. could be |
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 reckon, I'd keep the If the URL has a path portion, it will be used to prefix all HTTP endpoints served by Alertmanager
part of the description. This can be extremely confusing for some folks - as the UI and API are mounted separately but under the same path.
Something like: If the URL has a path portion, it will be used to prefix all HTTP endpoints served by Alertmanager, both the UI and API.
This means that, UI would be at https://admin-ops-us-east-0.grafana.net/alertmanager/#/alerts
and API would be at https://admin-ops-us-east-0.grafana.net/alertmanager/api/v1/alerts
as opposed to API being at https://admin-ops-us-east-0.grafana.net/alertmanager/#/alerts/api/v1/alerts
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.
mimir/pkg/alertmanager/alertmanager.go
Lines 283 to 286 in fe7fe60
router := route.New().WithPrefix(am.cfg.ExternalURL.Path) | |
ui.Register(router, webReload, log.With(am.logger, "component", "ui")) | |
am.mux = am.api.Register(router, am.cfg.ExternalURL.Path) |
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.
Something like: If the URL has a path portion, it will be used to prefix all HTTP endpoints served by Alertmanager, both the UI and API.
I've added it. I kept the rest of the description (even if verbose) to try to clarify how it works as much as possible. This stuff is tricky and today it took me 2 hours to figure it out.
Signed-off-by: Marco Pracucci <[email protected]>
@@ -226,7 +226,7 @@ Usage of ./cmd/mimir/mimir: | |||
-alertmanager.storage.retention duration | |||
How long to keep data for. (default 120h0m0s) | |||
-alertmanager.web.external-url value | |||
The URL under which Alertmanager is externally reachable (for example, if Alertmanager is served via a reverse proxy). Used for generating relative and absolute links back to Alertmanager itself. If the URL has a path portion, it will be used to prefix all HTTP endpoints served by Alertmanager. (default http://localhost) | |||
The URL under which Alertmanager is externally reachable (eg. could be different than -http.alertmanager-http-prefix in case Alertmanager is served via a reverse proxy). This setting is used both to configure the internal requests router and to generate links in alert templates. If the external URL has a path portion, it will be used to prefix all HTTP endpoints served by Alertmanager, both the UI and API. (default http://localhost:8080/alertmanager) |
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.
cc @jdbaldry
What this PR does:
This PR fixes #1066 doing two changes:
-alertmanager.web.external-url
to match the actual default configWhich issue(s) this PR fixes:
Fixes #1066
Checklist
CHANGELOG.md
updated - the order of entries should be[CHANGE]
,[FEATURE]
,[ENHANCEMENT]
,[BUGFIX]