Alerting: Add recording rules to ruler API and validation #87779

alexweav · 2024-05-13T22:11:25Z

What is this feature?

Adds support for creating recording rules in the main API.

Recording rules have a new field, Record, and lack several of the other "statefulness" fields that only make sense for alerting rules.

Extends the validation logic to have two paths. Shared validation happens, then we try to detect if the rule is a recording or alerting rule, then validate the remaining fields as needed.

Why do we need this feature?

Allows users to manipulate recording rules over the API, and eventually the UI.

Which issue(s) does this PR fix?:

n/a

Special notes for your reviewer:

Please check that:

It works as expected from a user's perspective.
If this is a pre-GA feature, it is behind a feature toggle.
The docs are updated, and if this is a notable improvement, it's added to our What's New doc.

alexweav · 2024-05-15T14:45:57Z

pkg/services/ngalert/api/api_ruler.go

@@ -530,6 +530,7 @@ func toGettableExtendedRuleNode(r ngmodels.AlertRule, provenanceRecords map[stri
 			Provenance:           apimodels.Provenance(provenance),
 			IsPaused:             r.IsPaused,
 			NotificationSettings: AlertRuleNotificationSettingsFromNotificationSettings(r.NotificationSettings),
+			Record:               ApiRecordFromModelRecord(r.Record),


Always allow reading of recording rules regardless of feature toggle.

If we don't allow this, things get really weird in terms of consistency/ordering if you enable the toggle, create a RR, then disable the toggle. A more advanced solution seemed pointless as the toggle will go away in the future. Basically, if you do this, the group will still work and can be read, but can't be edited until the RR is removed.

alexweav · 2024-05-15T14:48:00Z

pkg/services/ngalert/api/api_ruler_validation.go

-	if len(ruleNode.GrafanaManagedAlert.Data) == 0 {
-		if canPatch {
-			if ruleNode.GrafanaManagedAlert.Condition != "" {
-				return nil, fmt.Errorf("%w: query is not specified by condition is. You must specify both query and condition to update existing alert rule", ngmodels.ErrAlertRuleFailedValidation)
-			}
-		} else {
-			return nil, fmt.Errorf("%w: no queries or expressions are found", ngmodels.ErrAlertRuleFailedValidation)
-		}
-	} else {
-		err = validateCondition(ruleNode.GrafanaManagedAlert.Condition, ruleNode.GrafanaManagedAlert.Data)
-		if err != nil {
-			return nil, fmt.Errorf("%w: %s", ngmodels.ErrAlertRuleFailedValidation, err.Error())
-		}
-	}


All this extra logic surrounding validateCondition is now shoved inside validateCondition. validateCondition now handles all these cases and can be directly called from any path.

You're referring to validateAlertingRule here right?

I mean the logic surrounding validateCondition (if canPatch { ...). It's put inside validateCondition now, so code can just call validateCondition straight up without needing to worry about this extra stuff that's also validating part of the condition.

Then on top of that, the call to validateCondition got put inside validateAlertingRule. but, the comment was referring to the surrounding logic being put inside validateCondition. That's why you don't see this whole block copied inside validateAlertingRule, just the call to validateCondition and nothing else.

alexweav · 2024-05-15T14:49:22Z

pkg/services/ngalert/api/api_ruler_validation.go

+	if canPatch {
+		// Patch requests may leave both query and condition blank. If a request supplies one, it must supply the other.
+		if len(queries) == 0 && condition == "" {
+			return nil
+		}
+		if len(queries) == 0 && condition != "" {
+			return fmt.Errorf("%w: query is not specified but condition is. You must specify both query and condition to update existing alert rule", ngmodels.ErrAlertRuleFailedValidation)
+		}
+		if len(queries) > 0 && condition == "" {
+			return fmt.Errorf("%w: condition is not specified but query is. You must specify both query and condition to update existing alert rule", ngmodels.ErrAlertRuleFailedValidation)
+		}
+	}


This is a less nested version of the if/else logic surrounding validateCondition's old call site. IMO it shows intent much better.

Agreed, much easier to follow 👍

alexweav · 2024-05-15T14:52:01Z

pkg/tests/api/alerting/api_alertmanager_test.go

+						Condition: "A",
+						Data:      []apimodels.AlertQuery{},


The rule in this test was invalid in more than one way. By reordering validation checks, it caused this test to no longer cover what it was supposed to cover.

By fixing condition, we now cover the intent of this test a bit better, and it's not so tied to implementation.

rwwiv

LGTM, left a note about side effects in the new validation functions but it isn't blocking.

rwwiv · 2024-05-20T19:23:49Z

pkg/services/ngalert/api/api_ruler_validation.go

@@ -145,6 +100,85 @@ func validateRuleNode(
 	return &newAlertRule, nil
 }

+func validateAlertingRule(in *apimodels.PostableExtendedRuleNode, newRule *ngmodels.AlertRule, canPatch bool) error {


The side effect modification of newRule isn't super obvious without reading the whole function, WDYT about this and validateRecordingRule either returning the validated rule or adding docstrings to mention that field is modified?

Yeah that's fair. The main reason I didn't return a new object was that it wouldn't be fully formed yet. The result of the function wouldn't be a usable alert rule, it would be missing many shared fields like Title. I thought this captured the idea better that "this function has to be used alongside other things"

I thought about instead inverting this - where the shared validations are also extracted to a function. Then, the top level would call just validateAlertingRule, which would then call sharedValidations and then all the alert specific validations. Then, it could more stably return a fully-formed rule.

I did the comment approach for now, and I also renamed the functions to validate<X>RuleFields to better express that they only validate a subset of the rule.

I think that's probably enough, if you disagree i could go the fully separated route with a function for common validation as well. The cost of this is more indirection in the code, though.

Discussed offline, pushed another update which modifies without a reference and returns the result. This also changed the call site a bit

rwwiv · 2024-05-20T19:24:20Z

pkg/services/ngalert/api/api_ruler_validation.go

+	if canPatch {
+		// Patch requests may leave both query and condition blank. If a request supplies one, it must supply the other.
+		if len(queries) == 0 && condition == "" {
+			return nil
+		}
+		if len(queries) == 0 && condition != "" {
+			return fmt.Errorf("%w: query is not specified but condition is. You must specify both query and condition to update existing alert rule", ngmodels.ErrAlertRuleFailedValidation)
+		}
+		if len(queries) > 0 && condition == "" {
+			return fmt.Errorf("%w: condition is not specified but query is. You must specify both query and condition to update existing alert rule", ngmodels.ErrAlertRuleFailedValidation)
+		}
+	}


Agreed, much easier to follow 👍

pkg/services/ngalert/api/tooling/api.json

…ion logic

grafana-delivery-bot bot added this to the 11.1.x milestone May 13, 2024

grafana-pr-automation bot added the area/backend label May 13, 2024

alexweav force-pushed the alexweav/recording-rule-main-api branch from 3dbf09d to be43867 Compare May 14, 2024 20:41

alexweav added no-changelog Skip including change in changelog/release notes no-backport Skip backport of PR area/alerting Grafana Alerting labels May 14, 2024

grafana-pr-automation bot added the area/frontend label May 14, 2024

alexweav marked this pull request as ready for review May 15, 2024 14:36

alexweav requested review from a team as code owners May 15, 2024 14:36

alexweav requested review from jtheory, rwwiv, JacobsonMT, yuri-tceretian and grobinson-grafana and removed request for a team May 15, 2024 14:36

alexweav commented May 15, 2024

View reviewed changes

stevesg self-requested a review May 15, 2024 14:58

rwwiv approved these changes May 20, 2024

View reviewed changes

alexweav added 11 commits May 21, 2024 10:46

Read path, main API

421f44b

Define record field for incoming requests

adda669

Refactor several alerting specific validators into two paths

2fcb7df

Refactor validateCondition actually contain all the condition validat…

875e206

…ion logic

Move condition validation inside rule path

635bfd5

Validators for recording rules

aa43025

Wire feature flag through to validators

8de9128

Test for accepting a valid recording rule

29d0250

Tests for negative case, no UID

ea16a53

Test for ignoring alerting fields

937979b

Build conditions based on recording rules as well

c5805d1

alexweav added 6 commits May 21, 2024 10:47

Regenerate swagger docs

10727b9

Fix CRUD test to cover the right thing

c478284

Re-generate swagger docs with backdated v0.30.2 version

b642bd4

Regenerate base spec

a37d7dc

Regenerate ngalert specs

fa3a253

Regenerate top level specs

716ca2e

alexweav force-pushed the alexweav/recording-rule-main-api branch from e2a4dcf to 716ca2e Compare May 21, 2024 15:56

alexweav added 2 commits May 21, 2024 11:08

Comment and rename

f84005b

Return struct instead of modifying ref

4498325

alexweav merged commit 49c8deb into main May 21, 2024
12 checks passed

alexweav deleted the alexweav/recording-rule-main-api branch May 21, 2024 19:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Alerting: Add recording rules to ruler API and validation #87779

Alerting: Add recording rules to ruler API and validation #87779

alexweav commented May 13, 2024 •

edited

alexweav May 15, 2024 •

edited

alexweav May 15, 2024

rwwiv May 20, 2024

alexweav May 20, 2024

alexweav May 15, 2024

rwwiv May 20, 2024

alexweav May 15, 2024

rwwiv left a comment

rwwiv May 20, 2024

alexweav May 21, 2024

alexweav May 21, 2024

alexweav May 21, 2024

rwwiv May 20, 2024

Alerting: Add recording rules to ruler API and validation #87779

Alerting: Add recording rules to ruler API and validation #87779

Conversation

alexweav commented May 13, 2024 • edited

alexweav May 15, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rwwiv left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexweav commented May 13, 2024 •

edited

alexweav May 15, 2024 •

edited