align endpoint names with cloud #16
Conversation
domasx2
force-pushed
the
domas/align-api-naming-with-cloud
branch
from
4434ec4
to
15a64a9
Compare
| @@ -9,79 +9,79 @@ import ( | |||
| "github.com/prometheus/alertmanager/config" | |||
| ) | |||
|
|
|||
| // swagger:route POST /api/v1/config alertmanager RoutePostAlertingConfig | |||
| // swagger:route POST /alertmanager/api/v1/config alertmanager RoutePostAlertingConfig | |||
There was a problem hiding this comment.
I think that api/v1/config does not exist. The endpoints for managing configuration of the alert manager are here.
So the path should be: /api/v1/alerts for not introducing a breaking change and for not conflicting with the prometheus endpoint for listing alert, we need the prefix. Therefore, I think that the path, for conformance with the existing API, should be:
/alertmanager/api/v1/alerts.
There was a problem hiding this comment.
we can't have /alertmanager/api/v1/alerts as AM has another endpoint api/v1/alerts... But updating config is not actually part of the promehteus alertmanager api at all, just rleated to it. Maybe just /api/v1/config then?
There was a problem hiding this comment.
Cortex mounts the get/set config routes on /api/v1/alerts and then registers the alertmanager alert endpoints on /alertmanager/api/v1/alerts. Since we're unlikely to use the base route /api/v1/alerts for our API, I think using /config is a reasonable workaround. I'm open to alternatives here :/
| // | ||
| // sets an Alerting config | ||
| // | ||
| // Responses: | ||
| // 201: Ack | ||
| // 400: ValidationError | ||
|
|
||
| // swagger:route GET /api/v1/config alertmanager RouteGetAlertingConfig | ||
| // swagger:route GET /alertmanager/api/v1/config alertmanager RouteGetAlertingConfig |
There was a problem hiding this comment.
I think that the path, for conformance with the existing API, should be: /alertmanager/api/v1/alerts.
| // | ||
| // gets an Alerting config | ||
| // | ||
| // Responses: | ||
| // 200: AlertingConfigResponse | ||
| // 400: ValidationError | ||
|
|
||
| // swagger:route DELETE /api/v1/config alertmanager RouteDeleteAlertingConfig | ||
| // swagger:route DELETE /alertmanager/api/v1/config alertmanager RouteDeleteAlertingConfig |
There was a problem hiding this comment.
I think that the path, for conformance with the existing API, should be: /alertmanager/api/v1/alerts.
There was a problem hiding this comment.
but AM has another endpoint api/v1/alerts that does a different thing :/ I'm not sure what's the best way to go here..
There was a problem hiding this comment.
Considering the existing endpoint with the same path, I think suffixing /config is ok here.
There was a problem hiding this comment.
I'm confused; I don't think that there exists endpoint with /config path.
There was a problem hiding this comment.
you're right, not alertmanager native. Let's drop prefix and leave /api/v1/config maybe?
There was a problem hiding this comment.
- change to
/api/v1/alerts
| // | ||
| // deletes the Alerting config for a tenant | ||
| // | ||
| // Responses: | ||
| // 200: Ack | ||
| // 400: ValidationError | ||
|
|
||
| // swagger:route GET /api/v1/alerts alertmanager RouteGetAmAlerts | ||
| // swagger:route GET /alertmanager/api/v2/alerts alertmanager RouteGetAmAlerts |
There was a problem hiding this comment.
alertmanager has v1 and v2 versions, they are definitely prefixed here . so I think /alertmanager/api/v2/alerts is the correct one for the newest endpoint. Also it's the one we use in cloud alerting ui
There was a problem hiding this comment.
thanks for clarifying it; can you point me where it's used in the UI?
There was a problem hiding this comment.
in cloud alerting ui we used it to find alerts matching specific labels, to preview alerts that are affected by a silence.
I'm not entirely sure unified alerting UI will be the same for grafana v8 tho, but very possible...
| @@ -6,14 +6,14 @@ import ( | |||
| v1 "github.com/prometheus/client_golang/api/prometheus/v1" | |||
| ) | |||
|
|
|||
| // swagger:route GET /api/v1/status/rules prometheus RouteGetRuleStatuses | |||
| // swagger:route GET /prometheus/api/v1/rules prometheus RouteGetRuleStatuses | |||
There was a problem hiding this comment.
I would prefer a prefix other than prometheus because this does not make much sense for a user that wants to create a grafana managed alert for a querying a datasource other than prometheus. However, I'm not sure if this endpoint will be used for creating grafana managed alerts and also I don't have some better prefix to suggest.
There was a problem hiding this comment.
well, it signifies that it's a prometheus-like endpoint rathre than prom is the datasource. Loki has this too, and loki is not prometheus or supports promql: https://grafana.com/docs/loki/latest/api/
There was a problem hiding this comment.
I think it makes sense to keep /prometheus here for parity.
| @@ -6,7 +6,7 @@ import ( | |||
| "github.com/prometheus/common/model" | |||
| ) | |||
|
|
|||
| // swagger:route Get /api/v1/rules ruler RouteGetRulesConfig | |||
| // swagger:route Get /ruler/api/v1/rules ruler RouteGetRulesConfig | |||
There was a problem hiding this comment.
I'm not sure if we need to change this since there are no conflicting endpoints but also I don't see a problem changing it.
Imho it's simplest and least confusing to just prefix every endpoint with the name of the system that we copy it from, not on case-by-case basis This is not our versioning and not being increased, just matching endpoint name from alertmanager |
|
Going to merge and follow up going with the cortex path for now |
as discussed yesterday, align endpoint names with cloud versions, prefixing with name of appropriate system