Skip to content
This repository has been archived by the owner on Apr 26, 2021. It is now read-only.

path specific datasource for alerting #15

Merged
merged 2 commits into from
Feb 26, 2021
Merged

Conversation

owen-d
Copy link
Member

@owen-d owen-d commented Feb 25, 2021

This allows differentiating alerting backends based on a datasource ID.

Followup: replace the two (Alertmanager, GrafanaManaged) routing trees from the ApiAlertingConfig. Now that we can differentiate by datasource, we can validate a single routing tree based on which datasource it references.

@@ -9,79 +9,79 @@ import (
"github.com/prometheus/alertmanager/config"
)

// swagger:route POST /api/v1/config alertmanager RoutePostAlertingConfig
// swagger:route POST /api/am/{DatasourceId}/v1/config alertmanager RoutePostAlertingConfig
Copy link
Collaborator

@domasx2 domasx2 Feb 26, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

re our discussion yesterday, to align with cloud, it should probably be: /alertmanager/{DatasourceId}/api/v1/config, and likewise for other endpoitns

Copy link
Collaborator

@papagian papagian Feb 26, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In grafana we prefer identifying datasources using their UID. Also the datasourceId property differs from the datasource name (it's an integer). Therefore, I would suggest changing it to DatasourceUID. If that's not possible and we still need to use the datasource name, I would suggest to use DatasourceName instead (for not confusing the grafana users).

@domasx2
Copy link
Collaborator

domasx2 commented Feb 26, 2021

DatasourceId is for a "Alertmanager" datasource, right? what do we specify if we want the grafana-internal am config?

//
// deletes the Alerting config for a tenant
//
// Responses:
// 200: Ack
// 400: ValidationError

// swagger:route GET /api/v1/alerts alertmanager RouteGetAmAlerts
// swagger:route GET /api/am/{DatasourceId}/v1/alerts alertmanager RouteGetAmAlerts
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I got the impression that for conformance with the existing API we cannot change the path just adding a prefix. Therefore I think that it should become:
/am/{DatasourceId}/api/v1/alerts.

@owen-d
Copy link
Member Author

owen-d commented Feb 26, 2021

I've merged in master & readjusted the endpoints. The "config" endpoints now use /alertmanager/{DatasourceId}/config/api/v1/alerts, effectively making their path prefix /alertmanager/{DatasourceId}/config and then mounting the cortex-AM /api/v1/alerts on top of that.

@owen-d owen-d merged commit 0d56435 into master Feb 26, 2021
@owen-d owen-d deleted the alerts/datasource-id branch March 4, 2021 12:54
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants