Fix support for keep_firing_for field in alert rules #5823
Conversation
Signed-off-by: Mustafain Ali Khan <mustalik@amazon.com>
There was a problem hiding this comment.
Thanks this is nice catch.
@mustafain117 I wonder if we can also update the integration test to make sure this feature really works? https://github.com/cortexproject/cortex/blob/master/integration/ruler_test.go
Unit tests probably not the best place to capture that.
pkg/ruler/api.go
Outdated
| Annotations labels.Labels `json:"annotations"` | ||
| State string `json:"state"` | ||
| ActiveAt *time.Time `json:"activeAt"` | ||
| KeepFiringSince *time.Time `json:"keepFiringSince"` |
There was a problem hiding this comment.
To match prometheus I think this needs to be json:"keepFiringSince,omitempty
There was a problem hiding this comment.
Yes, thanks for catching this. Updated.
I also verified the API response matches prometheus.
keepFiringSince is only included in the response when expected:
Response when alert is not resolved/active (keep firing for value not used):
"groups": [
{
"name": "keep-firing2",
"file": "rg-keep-firing",
"rules": [
{
"state": "firing",
"name": "alerting_rule_keepfiring60mins2",
"query": "absent(recording_rule)",
"duration": 0,
"keepFiringFor": 3600,
"labels": {},
"annotations": {},
"alerts": [
{
"labels": {
"alertname": "alerting_rule_keepfiring60mins2"
},
"annotations": {},
"state": "firing",
"activeAt": "2024-03-21T08:22:03.514363333Z",
"value": "1e+00"
}
],
"health": "ok",
"lastError": "",
"type": "alerting",
"lastEvaluation": "2024-03-21T08:26:03.515540067Z",
"evaluationTime": 0.002836834
},
...]
Response when alert expression is resolved but keep firing for value is being used:
"groups": [
{
"name": "keep-firing2",
"file": "rg-keep-firing",
"rules": [
{
"state": "firing",
"name": "alerting_rule_keepfiring60mins2",
"query": "absent(recording_rule)",
"duration": 0,
"keepFiringFor": 3600,
"labels": {},
"annotations": {},
"alerts": [
{
"labels": {
"alertname": "alerting_rule_keepfiring60mins2"
},
"annotations": {},
"state": "firing",
"activeAt": "2024-03-21T08:22:03.514363333Z",
"keepFiringSince": "2024-03-21T08:27:03.514363333Z",
"value": "1e+00"
}
],
"health": "ok",
"lastError": "",
"type": "alerting",
"lastEvaluation": "2024-03-21T08:29:03.515574705Z",
"evaluationTime": 0.002663176
},
...]
Signed-off-by: Mustafain Ali Khan <mustalik@amazon.com>
mustafain117
force-pushed
the
keep-firing-for
branch
from
84d3a62 to
7140b26
Compare
|
|
||
| ruleDesc := desc.Rules[0] | ||
|
|
||
| assert.Equal(t, "test_rule", ruleDesc.Alert) |
There was a problem hiding this comment.
should we also verify the keep firing for in alerts field?
There was a problem hiding this comment.
Added an integration test that validates the KeepFiringSince field in alerts
Added an integration test for this feature |
Signed-off-by: Mustafain Ali Khan <mustalik@amazon.com>
mustafain117
force-pushed
the
keep-firing-for
branch
from
eaa3a2c to
bb37d29
Compare
| Value string `json:"value"` | ||
| } | ||
|
|
||
| func alertRuleWithKeepFiringFor(groupName string, ruleName string, expression string, keepFiring model.Duration) rulefmt.RuleGroup { |
There was a problem hiding this comment.
I'd love to see we don't duplicate code for this since it is almost the same functionality for ruleGroupWithRule.
But it is not a blocker. We can address later to use builder pattern.
What this PR does:
This field was added in prometheus prometheus/prometheus#11827
And was pulled into Cortex 1.15.0 but currently does not work, this PR fixes this.
Which issue(s) this PR fixes:
Fixes #
Checklist
CHANGELOG.mdupdated - the order of entries should be[CHANGE],[FEATURE],[ENHANCEMENT],[BUGFIX]