Adds support for line-column numbers for invalid rules, promtool #6533

Harkishen-Singh · 2019-12-30T16:00:51Z

Fixes: #1758 link

This PR uses the help of gopkg.in/yaml.v3 to implement this functionality.

Signed-off-by: Harkishen Singh <harkishensingh@hotmail.com>

brian-brazil

Can we do a straight switch everywhere to v3? It'd be better not to have two versions around.

brian-brazil · 2019-12-31T10:11:25Z

rules/manager_test.go

@@ -769,7 +769,7 @@ func reloadAndValidate(rgs *rulefmt.RuleGroups, t *testing.T, tmpFile *os.File,
 	_, err = tmpFile.Write(bs)
 	testutil.Ok(t, err)
 	err = ruleManager.Update(10*time.Second, []string{tmpFile.Name()}, nil)
-	testutil.Ok(t, err)
+	testutil.NotOk(t, err)


Why does this fail now?

Returned now is along with line and column number. I haven't been able to figure this out yet. Can you help me with this?

Not offhand, it'd take debugging.

@brian-brazil When we marshal a struct containing the yaml.Node type, the []byte received(shown below) contains additional information from the node. This is done in the reloadAndValidate(). This will create an error from the code since the output would be like:

groups: - name: test interval: 0s rules: - record: kind: 8 style: 0 tag: '!!str' value: job:http_requests:rate5m anchor: "" alias: null content: [] headcomment: "" linecomment: "" footcomment: "" line: 4 column: 15 expr: kind: 8 style: 0 tag: '!!str' value: sum by (job)(rate(http_requests_total[5m])) anchor: "" alias: null content: [] headcomment: "" linecomment: "" footcomment: "" line: 5 column: 13

Note: record and expr are yaml.Node.

Hence when we Parse this to rulefmt, this will return an error since record: and expr: are empty fields here and hence I used testutil.NotOk()

That sounds like we need a better test then, as this one is no longer doing what it's meant to be doing

I have fixed this by formatting the rules to the yaml v2 standards.

pkg/rulefmt/rulefmt.go

brian-brazil · 2019-12-31T10:12:46Z

pkg/rulefmt/rulefmt.go

 		if g.Name == "" {
-			errs = append(errs, errors.Errorf("Groupname should not be empty"))
+			errs = append(errs, errors.Errorf("Groupname should not be empty, line: %d, column: %d", grpsTmp.Groups[j].Line, grpsTmp.Groups[j].Column))


Do we have to add the line/col in every error?

I think, it would be good for the user to understand and find the invalid rule if suppose in a large rules file. For present, we can find both the line and column number.

Harkishen-Singh · 2020-01-02T13:10:08Z

Straight switch to v3 through the entire project will cause some test failures since the new package does not support UnmarshalStrict. Files using such will experience CI fails. @brian-brazil

Signed-off-by: Harkishen Singh <harkishensingh@hotmail.com>

slrtbtfs · 2020-01-02T14:02:18Z

@Harkishen-Singh FYI, when using the PromQL language server you already get some of the functionality of this PR.

Harkishen-Singh · 2020-01-02T14:15:53Z

@slrtbtfs I don't find anything related to rule checking and validation from the readme. Please correct me if I am wrong.

slrtbtfs · 2020-01-02T14:09:56Z

pkg/rulefmt/rulefmt.go

 		if g.Name == "" {
-			errs = append(errs, errors.Errorf("Groupname should not be empty"))
+			errs = append(errs, errors.Errorf("Groupname should not be empty, line: %d, column: %d", node.Groups[j].Line, node.Groups[j].Column))


Prometheus uses the <line>:<col>: <Error message> or <filename>:<line>:<col>: <Error message> format for errors in the expression browser now, since that format is used by many compilers. Also, when using that format, VS Code allows to just click on the message to get the cursor to the error position.

Might make sense to do the same here.

slrtbtfs · 2020-01-02T14:15:55Z

pkg/rulefmt/rulefmt.go

+		fmt.Println(r.Expr.Line)
+		errs = append(errs, errors.Errorf("field 'expr' must be set in rule, line: %d, column: %d", r.Expr.Line, r.Expr.Column))
+	} else if _, err := promql.ParseExpr(r.Expr.Value); err != nil {
+		errs = append(errs, errors.Wrapf(err, "could not parse expression, line: %d, column: %d", r.Expr.Line, r.Expr.Column))


This should end up looking quite strange, since the error returned by the parser will include position information, too.

Normally the parser returns a promql.ParseErr. This struct already includes the position information.

What the PromQL language server does here, is to add the line and column where the expression starts to the line and column in the error message and then simply convert the error message to a string

We need the position coordinates to make debugging rules easier for the user, in case of a larger file, increasing the convenience. Similar behaviour is noted in various format checkers as well.

That's clear.

The problem with the current approach is that the output will look roughly like
could not parse expression, line: 3, column: 10: 2:14: Unexpected identifier in label matcher, expected string.

It would be better if the output looked like:
rules.yaml:4:24: could not parse expression: Unexpected identifier in label matcher, expected string. Note that the positions have been added together here.

The current implement is, (from live example)

rules.yml: Groupname should not be empty, line: 2, column: 5 rules.yml: group "recording-node-exporter", rule 1, "heap": field 'expr' must be set in rule, line: 13, column: 12

is this acceptable? @brian-brazil

can you post your rules file here? @slrtbtfs

Thanks. Looks better now.

../../rules.yml: 2:5

That space should not be there and there should be a colon at the end of the position. To be consistent with conventions from various compilers the position should be of the form
../../rules.yml:2:5:

(nit)

../../rules.yml: 5:21 group "recording", rule 0, "foo": 4:19 could not parse expression: 1:14: parse error: could not parse remaining input "}\n"...

This one looks really confusing, since a single error message contains three different position coordinates and a verbal description of the position.

Ideally there would be only one position, directly at the beginning of the error line. Something like

../../rules.yml:5:28: group "recording", rule 0, "foo": could not parse expression: parse error: could not parse remaining input "}\n"...

Maybe even something more customized like:
../../rules.yml:5:28: Failed to parse PromQL expression: parse error: could not parse remaining input "}\n"...

As the design of the rulefmt, this looks tough. If we remove the parser checks and let it handle by itself as earlier, then this can be possible, but again on the right-hand side, towards the ending of the sentence.

It should be possible by just typecasting the err result of promql.ParseExpr() to a promql.ParseErr and using the fields in that data structure.

The space before the number is due to errors.Wrapf(). I don't think there is any way to remove this space. Any idea @slrtbtfs ?

Leaving that, rest I have given a new implementation of the logic. That solves most of what you said.

By not using Wrapf() but instead constructing a custom error message from the data contained in the error.

slrtbtfs · 2020-01-02T14:22:30Z

That's what is referred to as "YAML support" in the README.

However you're right that this should be explained better.

Relevant issue, tracking part of what is done in this PR, too: prometheus-community/promql-langserver#75

To be clear, ATM config file validation is still WIP.

Harkishen-Singh · 2020-01-02T14:29:35Z

@slrtbtfs I did not understand the purpose of the comment. Language server has a different purpose. Configuration checks by promtool are the general implementation which can be used by anyone, irrespective of the IDEs to ensure the proper functioning of the rules and alerts. Need not everyone to install the Prometheus language server.

slrtbtfs · 2020-01-02T14:41:57Z

Sure, this PR is useful independently of whether there exists a language server or not.

The point was mainly, that the use case of having better IDE support which was demoed in your screencast is also covered by the language server and pointing out that there exists other code that tries to achieve similar things.

Harkishen-Singh · 2020-01-02T14:47:31Z

Sure. Both have their own purpose. Having said that language-server is optional for a user, its good to have it.

Signed-off-by: Harkishen Singh <harkishensingh@hotmail.com>

pkg/rulefmt/rulefmt.go

brian-brazil · 2020-01-03T06:44:32Z

rules/manager_test.go

@@ -769,7 +769,7 @@ func reloadAndValidate(rgs *rulefmt.RuleGroups, t *testing.T, tmpFile *os.File,
 	_, err = tmpFile.Write(bs)
 	testutil.Ok(t, err)
 	err = ruleManager.Update(10*time.Second, []string{tmpFile.Name()}, nil)
-	testutil.Ok(t, err)
+	testutil.NotOk(t, err)


Not offhand, it'd take debugging.

Signed-off-by: Harkishen Singh <harkishensingh@hotmail.com>

Harkishen-Singh · 2020-01-07T15:31:25Z

@brian-brazil please review.

brian-brazil · 2020-01-08T10:04:56Z

pkg/rulefmt/rulefmt.go

-func (r *Rule) Validate() (errs []error) {
-	if r.Record != "" && r.Alert != "" {
-		errs = append(errs, errors.Errorf("only one of 'record' and 'alert' must be set"))
+func (r *RuleNode) Validate() (errs []WrapError) {


You only use this as an error

I am wrapping this error with the node values so that we can implement according to what slrbtfs said. Do you mean to remove the wrapper logic and use the earlier error logic?

No, I mean the return type here should be a slice of error.

@brian-brazil If I return a slice of error here, then the suggestion given by slrbtfs would not be possible. Please correct me if I am wrong.

@brian-brazil any suggestions?

Error is an interface, it's weird to refer to things as errors that don't meet that interface. So I suggest adjusting things so that your errors are errors.

@brian-brazil If I return a slice of error here, then the suggestion given by slrbtfs would not be possible. Please correct me if I am wrong.

It is possible.

You can return an error and then convert it back to the underlying concrete type when needed.

https://tour.golang.org/methods/15

Error is an interface, it's weird to refer to things as errors that don't meet that interface. So I suggest adjusting things so that your errors are errors.

With the current design, the errors are being returned after unwrapping, done by the Error interface function. Only after that, it is an error. I think the errs is leading to confusion even though its a slice of wrapped error along with its node value. Is this any case? @brian-brazil

brian-brazil · 2020-01-08T10:13:13Z

rules/manager_test.go

@@ -769,7 +769,7 @@ func reloadAndValidate(rgs *rulefmt.RuleGroups, t *testing.T, tmpFile *os.File,
 	_, err = tmpFile.Write(bs)
 	testutil.Ok(t, err)
 	err = ruleManager.Update(10*time.Second, []string{tmpFile.Name()}, nil)
-	testutil.Ok(t, err)
+	testutil.NotOk(t, err)


That sounds like we need a better test then, as this one is no longer doing what it's meant to be doing

Signed-off-by: Harkishen Singh <harkishensingh@hotmail.com>

Harkishen-Singh · 2020-01-11T20:42:03Z

@brian-brazil please have a look.

roidelapluie · 2020-01-11T20:53:12Z

Do we have the same behaviour as before? e.g. the UnmarshalStrict behaviour?

Harkishen-Singh · 2020-01-11T20:58:50Z

I do think yes. Since when we remove the yaml.Node then the entire thing works like the v2 yaml.

brian-brazil · 2020-01-13T10:00:04Z

pkg/rulefmt/rulefmt.go

 }

 // RuleGroups is a set of rule groups that are typically exposed in a file.
 type RuleGroups struct {
 	Groups []RuleGroup `yaml:"groups"`
 }

+// RuleGroupsTest for running tests over rules.
+type RuleGroupsTest struct {


This is unused.

This is actually used inside the manager_test.go. It helps to convert yaml.Node type into default strings type so that Marshalling can be done according to the aim of the tests.

If it's only used in tests, then it belongs in the test file.

Sure. I will update that.

This still seems to be here

brian-brazil · 2020-01-13T10:03:13Z

pkg/rulefmt/rulefmt.go

-func (r *Rule) Validate() (errs []error) {
-	if r.Record != "" && r.Alert != "" {
-		errs = append(errs, errors.Errorf("only one of 'record' and 'alert' must be set"))
+func (r *RuleNode) Validate() (errs []WrapError) {


Error is an interface, it's weird to refer to things as errors that don't meet that interface. So I suggest adjusting things so that your errors are errors.

…mbers

Signed-off-by: Harkishen Singh <harkishensingh@hotmail.com>

brian-brazil · 2020-01-15T17:24:20Z

pkg/rulefmt/rulefmt.go

+}
+
+// RuleGroupTest forms a testing struct for running tests over rules.
+type RuleGroupTest struct {


This one too.

@brian-brazil done. Please have a look.

Signed-off-by: Harkishen Singh <harkishensingh@hotmail.com>

brian-brazil · 2020-01-15T18:07:59Z

Thanks!

Harkishen-Singh added 4 commits December 30, 2019 19:05

base for support of lineand column numbers for invalid rules

29e47c2

Signed-off-by: Harkishen Singh <harkishensingh@hotmail.com>

version-1

b675068

Signed-off-by: Harkishen Singh <harkishensingh@hotmail.com>

support for line-column numbers for invalid rules promtool

0e193b4

Signed-off-by: Harkishen Singh <harkishensingh@hotmail.com>

support for line-column numbers for invalid rules promtool

33103f4

Signed-off-by: Harkishen Singh <harkishensingh@hotmail.com>

Harkishen-Singh changed the title ~~Adds support for Line-Column numbers for invalid rules during check, promtool~~ Adds support for line-column numbers for invalid rules, promtool Dec 30, 2019

Harkishen-Singh added 4 commits December 31, 2019 00:26

recovered mod & sum files

c7dcafa

Signed-off-by: Harkishen Singh <harkishensingh@hotmail.com>

recovered mod & sum files

d7fc74d

Signed-off-by: Harkishen Singh <harkishensingh@hotmail.com>

added gopkg/yaml.v3 to vendor

24da528

Signed-off-by: Harkishen Singh <harkishensingh@hotmail.com>

fix ineffectual assignment

428e53d

Signed-off-by: Harkishen Singh <harkishensingh@hotmail.com>

brian-brazil reviewed Dec 31, 2019

View reviewed changes

passing yaml node as params

42e31c9

Signed-off-by: Harkishen Singh <harkishensingh@hotmail.com>

slrtbtfs reviewed Jan 2, 2020

View reviewed changes

Harkishen-Singh added 2 commits January 2, 2020 22:29

updated the design

d75b092

Signed-off-by: Harkishen Singh <harkishensingh@hotmail.com>

formatted

1f873aa

Signed-off-by: Harkishen Singh <harkishensingh@hotmail.com>

brian-brazil reviewed Jan 3, 2020

View reviewed changes

Harkishen-Singh added 3 commits January 7, 2020 02:07

yamlv3 in manage_test

9d5c218

Signed-off-by: Harkishen Singh <harkishensingh@hotmail.com>

updated design

a7e08a9

Signed-off-by: Harkishen Singh <harkishensingh@hotmail.com>

appended testTemplateParsing errs

b4a7261

Signed-off-by: Harkishen Singh <harkishensingh@hotmail.com>

brian-brazil reviewed Jan 8, 2020

View reviewed changes

fixed tests. fixed missed alt condition

de7fe5f

Signed-off-by: Harkishen Singh <harkishensingh@hotmail.com>

brian-brazil reviewed Jan 13, 2020

View reviewed changes

Harkishen-Singh added 2 commits January 15, 2020 02:21

Merge https://github.com/prometheus/prometheus into fmt-rules-line-nu…

1151d53

…mbers

updated misleading err name

53e3eb7

Signed-off-by: Harkishen Singh <harkishensingh@hotmail.com>

brian-brazil reviewed Jan 15, 2020

View reviewed changes

placed test structs in proper files

2d3710a

Signed-off-by: Harkishen Singh <harkishensingh@hotmail.com>

brian-brazil merged commit 84e6459 into prometheus:master Jan 15, 2020

yeya24 mentioned this pull request Jan 23, 2020

Add query log thanos-io/thanos#1962

Closed

2 tasks

pstibrany mentioned this pull request Feb 6, 2020

Update prometheus to v2.16.0 cortexproject/cortex#2088

Merged

3 tasks

roidelapluie mentioned this pull request Mar 24, 2020

promtool check rules no longer identifies invalid fields as of 2.16.0 #7042

Closed

gotjosh mentioned this pull request Apr 28, 2020

WIP: Update to prometheus 2.18.0-rc0 thanos-io/thanos#2535

Closed

4 tasks

Dominik-K mentioned this pull request Feb 26, 2021

orb pack: Print file context if "An unexpected error occurred" CircleCI-Public/circleci-cli#519

Open

Nexucis mentioned this pull request Apr 6, 2021

As a third party, I can't validate the Rule using the package rulefmt #8694

Closed

Adds support for line-column numbers for invalid rules, promtool #6533

Adds support for line-column numbers for invalid rules, promtool #6533

Conversation

Harkishen-Singh commented Dec 30, 2019

brian-brazil 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

Harkishen-Singh Jan 7, 2020 • edited Loading

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

Harkishen-Singh Dec 31, 2019 • edited Loading

Choose a reason for hiding this comment

Harkishen-Singh commented Jan 2, 2020

slrtbtfs commented Jan 2, 2020

Harkishen-Singh commented Jan 2, 2020

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

Choose a reason for hiding this comment

slrtbtfs Jan 7, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

slrtbtfs commented Jan 2, 2020

Harkishen-Singh commented Jan 2, 2020

slrtbtfs commented Jan 2, 2020

Harkishen-Singh commented Jan 2, 2020

Choose a reason for hiding this comment

Harkishen-Singh commented Jan 7, 2020

Choose a reason for hiding this comment

Harkishen-Singh Jan 9, 2020 • edited Loading

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

Harkishen-Singh Jan 14, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Harkishen-Singh commented Jan 11, 2020

roidelapluie commented Jan 11, 2020

Harkishen-Singh commented Jan 11, 2020 • edited Loading

Choose a reason for hiding this comment

Harkishen-Singh Jan 13, 2020 • edited Loading

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

brian-brazil commented Jan 15, 2020

Harkishen-Singh Jan 7, 2020 •

edited

Loading

Harkishen-Singh Dec 31, 2019 •

edited

Loading

slrtbtfs Jan 7, 2020 •

edited

Loading

Harkishen-Singh Jan 9, 2020 •

edited

Loading

Harkishen-Singh Jan 14, 2020 •

edited

Loading

Harkishen-Singh commented Jan 11, 2020 •

edited

Loading

Harkishen-Singh Jan 13, 2020 •

edited

Loading