Alerting: Add database migration for recording rule fields #87012
Conversation
alexweav
added
area/alerting
alexweav
requested review from
rwwiv,
JacobsonMT,
yuri-tceretian,
grobinson-grafana and
zserge
and removed request for
a team
| @@ -240,6 +240,8 @@ type AlertRule struct { | |||
| PanelID *int64 `xorm:"panel_id"` | |||
| RuleGroup string | |||
| RuleGroupIndex int `xorm:"rule_group_idx"` | |||
| Record string | |||
There was a problem hiding this comment.
Prometheus just reuses the title for this - but it also calls the field Record. That's where the name came from.
I think we should not reuse the title directly. There is a chance that the other fields may not be needed - we need some field to identify a recording rule, or it becomes indistinguishable from certain alert rule patch requests. Nothing stops users from using the same string for title and record.
| @@ -240,6 +240,8 @@ type AlertRule struct { | |||
| PanelID *int64 `xorm:"panel_id"` | |||
| RuleGroup string | |||
| RuleGroupIndex int `xorm:"rule_group_idx"` | |||
| Record string | |||
| RecordFrom string | |||
There was a problem hiding this comment.
This is literally the same thing as Condition. But, the term "condition" doesn't make any sense for recording rules.
I'm also open to not including this field, and instead renaming Condition to something more generic. Naming is hard though, and i don't think the duplication hurts anything as it's logically a different thing.
| @@ -240,6 +240,8 @@ type AlertRule struct { | |||
| PanelID *int64 `xorm:"panel_id"` | |||
| RuleGroup string | |||
| RuleGroupIndex int `xorm:"rule_group_idx"` | |||
| Record string | |||
| RecordFrom string | |||
There was a problem hiding this comment.
There might be another field called RecordTo, indicating the datasource to write the results to. Whether we want a per-rule datasource, or just config.ini knobs for that, is still up for debate in the design doc and has not reached consensus. So, I've left that field out for now.
That's where the current name came from. "RecordFrom" and "RecordTo" make a nice pair, if both exist - but open to alternative suggestions.
There was a problem hiding this comment.
In the new struct format we could call this To or Target. Less important now though
| if alertRule.Record != "" { | ||
| return fmt.Errorf("%w: Storing recording rules is not yet allowed.", ErrAlertRuleFailedValidation) | ||
| } |
There was a problem hiding this comment.
Not really reachable since we do not have API layer fields for this, just making things apparent. This was useful to catch serialization bugs in tests.
| @@ -240,6 +240,8 @@ type AlertRule struct { | |||
| PanelID *int64 `xorm:"panel_id"` | |||
| RuleGroup string | |||
| RuleGroupIndex int `xorm:"rule_group_idx"` | |||
| Record string | |||
There was a problem hiding this comment.
This also begs the question - we should probably either rename AlertRule to just Rule, since it might not actually be an alerting rule any more... or have separated domain model for alert rules vs recording rules that are combined in the API/rule table.
I'm considering both not in scope for this PR and leaving them for the future, as they're deep refactor type changes.
…dles nulls when deserializing
alexweav
force-pushed
the
alexweav/recording-rules-database
branch
from
eb81c51
to
e1248ab
Compare
| @@ -757,3 +766,26 @@ func GroupByAlertRuleGroupKey(rules []*AlertRule) map[AlertRuleGroupKey]RulesGro | |||
| } | |||
| return result | |||
| } | |||
|
|
|||
| // Record contains mapping information for Recording Rules. | |||
| type Record struct { | |||
There was a problem hiding this comment.
Bikeshedding nit: WDYT about Recording?
There was a problem hiding this comment.
The field name or the struct name? It's a field called Record, of type Record right now
There was a problem hiding this comment.
I think field name should remain (record also has meaning in prometheus). Struct name i'm totally good with changing if we want - but this can be done without DB migration anytime in future
|
Tested migration on all supported grafana databases, no issues |
What is this feature?
This defines two new rule fields to be used in recording rules.
metric: The name of the series to write tofrom: indicates which expression node is considered the result of the graph - the same thing as "condition" but for recording rules.These are stored inside a sub-struct encompassing all recording rule data, called
record.Let the naming bikeshed commence 😄
Which issue(s) does this PR fix?:
n/a
Special notes for your reviewer:
Please check that: