Unit testing for rules by codesome · Pull Request #4350 · prometheus/prometheus

codesome · 2018-07-04T21:38:30Z

Issue: #1695

Design Doc: Prometheus - Unit testing for Rules

Usage: ./promtool test rules test1.yml test2.yml test3.yml ...

Example for valid test file: Prometheus - Unit Testing for Rules - Example test files

Signed-off-by: Ganesh Vernekar <cs15btech11018@iith.ac.in>

…rules Signed-off-by: Ganesh Vernekar <cs15btech11018@iith.ac.in>

gouthamve

Looks good for the first version. But ideally, I'd like to remove the custom parsing done by the promql tests altogether and replace it with this YAML format.

But that should be the next PR.

gouthamve · 2018-08-20T07:05:12Z

cmd/promtool/unittest.go

+
+// test performs the unit tests.
+func (tg *testGroup) test(mint, maxt time.Time, evalInterval time.Duration, groupOrderMap map[string]int, ruleFiles ...string) []error {
+


Extra whitespace

gouthamve · 2018-08-20T07:06:46Z

cmd/promtool/unittest.go

+	presentInTest := make(map[time.Duration]map[string]struct{})
+	// Map of all the unit tests for given eval_time.
+	alertTests := make(map[time.Duration][]alertTestCase)
+	for _, art := range tg.AlertRuleTest {


alert would be better than art

gouthamve · 2018-08-20T07:45:22Z

cmd/promtool/unittest.go

+type testGroup struct {
+	Interval       time.Duration    `yaml:"interval"`
+	InputSeries    []series         `yaml:"input_series"`
+	AlertRuleTest  []alertTestCase  `yaml:"alert_rule_test,omitempty"`


Plural AlertRuleTests and PromqlExprTests :)

gouthamve · 2018-08-20T08:01:06Z

cmd/promtool/unittest.go

+			}
+			return len(alertEvalTimes)
+		}()
+		alertEvalTimes = append(alertEvalTimes[:pos], append([]time.Duration{art.EvalTime}, alertEvalTimes[pos:]...)...)


Can we not sort the alertEvalTimes via: sort.Slice() after getting an unsorted slice? That looks more readable.

gouthamve · 2018-08-20T08:03:28Z

cmd/promtool/unittest.go

+	curr := 0
+
+	var errs []error
+	for ts := mint; maxt.Sub(ts) > 0; ts = ts.Add(evalInterval) {


ts.Before(maxt) is better

gouthamve · 2018-08-20T08:07:28Z

cmd/promtool/unittest.go

+						}
+					}
+
+					if _, ok := got[ar.Name()]; ok {


You could simply do: got[ar.Name()] = append(got[ar.Name()], alerts...)

gouthamve · 2018-08-20T08:08:44Z

cmd/promtool/unittest.go

+		}
+		presentInTest[art.EvalTime][art.Alertname] = struct{}{}
+
+		if _, ok := alertTests[art.EvalTime]; ok {


Could just be: alertTests[art.EvalTime] = append(alertTests[art.EvalTime], art)

gouthamve · 2018-08-20T08:10:11Z

cmd/promtool/unittest.go

+
+			for _, testCase := range alertTests[t] {
+				// Checking alerts.
+				gotAlerts, ok := got[testCase.Alertname]


Just gotAlerts := got[testCase.Alertname] should be fine.

gouthamve · 2018-08-20T08:10:30Z

cmd/promtool/unittest.go

+				}
+			}
+
+			for _, testCase := range alertTests[t] {


testcase should be fine. No need to camelcase that :)

gouthamve · 2018-08-20T08:12:55Z

cmd/promtool/unittest.go

+func (la labelsAndAnnotations) Len() int      { return len(la) }
+func (la labelsAndAnnotations) Swap(i, j int) { la[i], la[j] = la[j], la[i] }
+func (la labelsAndAnnotations) Less(i, j int) bool {
+	l1h, l2h := la[i].Labels.Hash(), la[j].Labels.Hash()


labels.Compare is better here.

Signed-off-by: Ganesh Vernekar <cs15btech11018@iith.ac.in>

…rules Signed-off-by: Ganesh Vernekar <cs15btech11018@iith.ac.in>

codesome · 2018-08-23T17:04:26Z

@gouthamve All the reviews fixed. Learn few stuff about Golang :)

gouthamve · 2018-08-27T09:37:15Z

cmd/promtool/main.go

 		"The rule files to check.",
 	).Required().ExistingFiles()

+	ruleTestCmd := testCmd.Command("rules", "Unit tests for rules.")


Could you move this to the end? (let all check* commands be next to each other). Further it should be named testRuleCmd for consistency.

gouthamve · 2018-08-27T10:17:53Z

cmd/promtool/unittest.go

+// mentioned by `groupOrderMap`. The returned slice starts with the groups that need to be in order,
+// followed by rest of the groups in undetermined order.
+func orderedGroups(groupsMap map[string]*rules.Group, groupOrderMap map[string]int) []*rules.Group {
+	var groups []*rules.Group


Okay, this is too complex. Can we do this instead:

groups := make([]*rules.Group, 0, len(groupsMaps)) for _, g := range groupsMap { groups = append(groups, g) } sort.Slice(groups, func(i, j int) bool { return groupOrderMap[groups[i].Name()] < groupOrderMap[groups[j].Name()] }) return groups

Didn't think of that, thanks! I have updated.

codesome · 2018-08-30T07:12:32Z

@brian-brazil PTAL when you get free. Also, where would the docs for this go in?

brian-brazil · 2018-08-30T07:17:42Z

I won't get to look until next week. A new sibling page to Recording/Alerting rules is probably the right place.

codesome · 2018-08-30T07:19:29Z

Sure, thanks.

brian-brazil · 2018-09-05T12:58:39Z

cmd/promtool/unittest.go

Why do you need this?

https://github.com/codesome/prometheus/blob/5576d8e6312d07431d2db5e8d76ac78908510968/cmd/promtool/unittest.go#L192

If we don't add some extra time, the above loop can miss the last eval if maxt happens to be exactly the last eval. I think adding one evalInterval would make more sense than rounding off.

brian-brazil · 2018-09-05T13:13:04Z

cmd/promtool/unittest.go

All this preparation is so that we can test alerts as we evaluate the rules. This avoids storing them in memory, as the number of evals might be high.

brian-brazil · 2018-09-05T13:19:04Z

cmd/promtool/unittest.go

brian-brazil · 2018-09-05T13:35:46Z

cmd/promtool/unittest.go

This is a bit long of a for expression, I'd suggest moving it into an if inside the for

cmd/promtool/unittest.go

brian-brazil · 2018-09-05T13:41:16Z

cmd/promtool/unittest.go

Why not break out here, rather than adding a new bool?

So that I can skip checking for got == exp here in case it was an error. So I am using a bool to keep track of that.

If you break out of the outer for, you don't need that

I have made the changes to continue to the outer for.

brian-brazil · 2018-09-05T13:41:45Z

cmd/promtool/unittest.go

Do you have rights to use this code as apache2?

I am unaware of licenses, but what I see is, if I remove that comment, the code is general enough that it can be used. Maybe change it a bit you say?

codesome · 2018-09-21T10:23:17Z

@brian-brazil PTAL.

brian-brazil · 2018-09-21T12:16:34Z

cmd/promtool/unittest.go

We should add proper docs for this

Yes. I will add docs for this. So I will remove that comment from here as docs will be added for this.

Any more changes/additions do you feel is required?

I'm not seeing the docs in the PR

Oh, I was trying to add docs in https://github.com/prometheus/docs. Just found that there is a docs/ directory in this repo. I will add them.

I have added the docs.

Signed-off-by: Ganesh Vernekar <cs15btech11018@iith.ac.in>

brian-brazil · 2018-09-25T11:39:46Z

docs/configuration/unit_testing_rules.md

+
+# This uses expanding notation.
+# Expanding notation: 
+#     a+bxc ~ a a+b a+(2*b) a+(3*b) … a+(c*b)


The meaning of the ~ here is not clear, I'd suggest using becomes instead

brian-brazil · 2018-09-25T11:41:30Z

docs/configuration/unit_testing_rules.md

+
+### `<alert>`
+
+Remember, this alert shoud be firing.


This comes across weirdly, I'd remove it.

brian-brazil · 2018-09-25T11:42:18Z

docs/configuration/unit_testing_rules.md

+
+```yaml
+# Labels of the sample in series notation.
+labels: <series_notation>


What's series notation?

I updated it with examples. I meant to say <metric name>{<label name>=<label value>, ...}.

brian-brazil · 2018-09-25T14:06:36Z

docs/configuration/unit_testing_rules.md

Signed-off-by: Ganesh Vernekar <cs15btech11018@iith.ac.in>

brian-brazil · 2018-09-25T16:06:33Z

Thanks!

Unit testing for rules

229fd7e

Signed-off-by: Ganesh Vernekar <cs15btech11018@iith.ac.in>

codesome force-pushed the unit-testing-for-rules branch 2 times, most recently from 4156e66 to 6500dc6 Compare July 23, 2018 16:21

Specifying order of group evaluation in unit tests

5d35364

Signed-off-by: Ganesh Vernekar <cs15btech11018@iith.ac.in>

codesome force-pushed the unit-testing-for-rules branch from 6500dc6 to 5d35364 Compare July 24, 2018 12:14

codesome changed the title ~~[WIP] Unit testing for rules~~ Unit testing for rules Jul 24, 2018

Merge remote-tracking branch 'upstream/master' into unit-testing-for-…

afd433e

…rules Signed-off-by: Ganesh Vernekar <cs15btech11018@iith.ac.in>

codesome force-pushed the unit-testing-for-rules branch from 2bbd264 to afd433e Compare July 25, 2018 09:30

gouthamve reviewed Aug 20, 2018

View reviewed changes

Ganesh Vernekar added 2 commits August 23, 2018 22:23

Fix review comments

41f0813

Signed-off-by: Ganesh Vernekar <cs15btech11018@iith.ac.in>

Merge remote-tracking branch 'upstream/master' into unit-testing-for-…

0abebf4

…rules Signed-off-by: Ganesh Vernekar <cs15btech11018@iith.ac.in>

gouthamve reviewed Aug 27, 2018

View reviewed changes

codesome force-pushed the unit-testing-for-rules branch from f85ea9d to 5576d8e Compare August 27, 2018 10:36

brian-brazil reviewed Sep 5, 2018

View reviewed changes

codesome force-pushed the unit-testing-for-rules branch 2 times, most recently from 556e729 to 348284d Compare September 6, 2018 11:50

gouthamve mentioned this pull request Sep 17, 2018

Feedback for the functionality upstream kevinjqiu/pat#16

Open

brian-brazil reviewed Sep 21, 2018

View reviewed changes

Fix review comments

9bf78e7

Signed-off-by: Ganesh Vernekar <cs15btech11018@iith.ac.in>

codesome force-pushed the unit-testing-for-rules branch from 348284d to 9bf78e7 Compare September 21, 2018 12:51

Add docs

f9701f7

Signed-off-by: Ganesh Vernekar <cs15btech11018@iith.ac.in>

codesome force-pushed the unit-testing-for-rules branch from 3c1b74c to f9701f7 Compare September 25, 2018 10:47

brian-brazil reviewed Sep 25, 2018

View reviewed changes

codesome force-pushed the unit-testing-for-rules branch from e56d1b9 to 5944b28 Compare September 25, 2018 13:01

brian-brazil reviewed Sep 25, 2018

View reviewed changes

Fix review comments

5215e79

Signed-off-by: Ganesh Vernekar <cs15btech11018@iith.ac.in>

codesome force-pushed the unit-testing-for-rules branch from 5944b28 to 5215e79 Compare September 25, 2018 14:15

brian-brazil merged commit 5790d23 into prometheus:master Sep 25, 2018

codesome deleted the unit-testing-for-rules branch September 25, 2018 16:25

codesome mentioned this pull request Sep 25, 2018

General purpose rule/alert testing tool #1695

Closed


		// test performs the unit tests.
		func (tg *testGroup) test(mint, maxt time.Time, evalInterval time.Duration, groupOrderMap map[string]int, ruleFiles ...string) []error {

Conversation

codesome commented Jul 4, 2018

Uh oh!

gouthamve left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codesome commented Aug 23, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codesome commented Aug 30, 2018

Uh oh!

brian-brazil commented Aug 30, 2018

Uh oh!

codesome commented Aug 30, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codesome commented Sep 21, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment