Add cortex ruler endpoints #7
Conversation
pkg/api/cortex-ruler.go
Outdated
| //GrafanaManagedAlert yaml.Node `yaml:"grafana_alert,omitempty"` | ||
| GrafanaManagedAlert ExtendedUpsertAlertDefinitionCommand `yaml:"grafana_alert,omitempty"` |
There was a problem hiding this comment.
I think it's better to expose the GrafanaManagedAlert properties instead of having it as yaml.Node (the commented line)
pkg/api/cortex-ruler.go
Outdated
| // swagger:model | ||
| type RuleGroupConfig struct { | ||
| Name string `yaml:"name"` | ||
| Interval model.Duration `yaml:"interval,omitempty"` |
There was a problem hiding this comment.
I think we should add json field tags as well?
pkg/api/cortex-ruler.go
Outdated
| UpsertAlertDefinitionCommand | ||
| NoDataState NoDataState `json:"no_data_state"` | ||
| ExecutionErrorState ExecutionErrorState `json:"exec_err_state"` | ||
| Settings map[string]interface{} `json:"settings"` |
There was a problem hiding this comment.
what's stored in settings?
There was a problem hiding this comment.
Dashboard alerts used to have such a field and NoDataState and ExecutionErrorState used be there. I don't know if some existing dashboard alerts have some additional settings therefore I have kept it.
There was a problem hiding this comment.
Ok. I heard that backenders are trying to figure out what to do with these states in the new alerting system, I guess things will be clarified later
pkg/api/cortex-ruler.go
Outdated
| OrgID int64 `json:"org_id"` | ||
| Condition string `json:"condition"` | ||
| Data []eval.AlertQuery `json:"data"` | ||
| IntervalSeconds *int64 `json:"intervalSeconds"` |
There was a problem hiding this comment.
I think this will be configurable per group rather than on individual alerts
There was a problem hiding this comment.
You are right; I kept it just because it's in the existing model in grafana repo (from where I have copied it because it's not exported). I've changed it to:
// IntervalSeconds is an obsolete field (it will derive from the ruleGroup interval)
IntervalSeconds *int64 `json:"-"
so it's not required to be passed in the request.
Is that any better?
There was a problem hiding this comment.
sure! we can always refine details later once things come into focus more
pkg/api/cortex-ruler.go
Outdated
| https://github.com/grafana/grafana/blob/debb82e12417e82a0e2bd09e1a450065f884c1bc/pkg/services/ngalert/models.go#L85 | ||
| type UpsertAlertDefinitionCommand struct { | ||
| Title string `json:"title"` | ||
| OrgID int64 `json:"org_id"` |
There was a problem hiding this comment.
isn't org implicit, user's current org?
There was a problem hiding this comment.
Same as above: I have changed it to:
// OrgID is an obsolete field (it will derive from the x-grafana-org-id header)
OrgID int64 `json:"-"`
so it's not required to be passed in the request.
Is that any better?
pkg/api/cortex-ruler.go
Outdated
| Data []eval.AlertQuery `json:"data"` | ||
| IntervalSeconds *int64 `json:"intervalSeconds"` | ||
| // UID exists is set only for existing definitins | ||
| UID string `json:"uid"` |
There was a problem hiding this comment.
the prometheus way is to rely on name, there are no IDs for rules. Not usre it makes sense to have one for grafana managed alerts specifically
There was a problem hiding this comment.
I think we need it because in grafana we identify all the entities by UID and we have versioning for the rules so we need to be able to tell when a rule is created or updated and I don't think we want to relay on the name (it can be also hard for the migration of the existing dashboards alerts).
This field will be empty when the rule is created and the grafana backend will assign one UID and will included in the GET responses.
There was a problem hiding this comment.
Alright, let's keep it for now
- /api/v1/rules/{Namespace} should return a map
- update ExtendedUpsertAlertDefinitionCommand properties
Introduces a new
GrafanaManagedAlertrule type (besides existingRecordandAlert) with exposed properties.