JSON label extraction field validation expression overly strict

[splitice]

What this PR does / why we need it:

A valid query started to fail. I can't share the exact query as it was from production, but the error message (slightly adjusted) is as follows).

level=error ts=2022-06-12T03:35:08.406753057Z caller=retry.go:73 org_id=fake msg="error processing request" try=0 err="rpc error: code = Code(500) desc = 3 errors: rpc error: code = Unknown desc = parse error : stage '| json layer7_something_detail=\"layer7_something_detail\"' : cannot parse expression [layer7_something_detail]: non-integer value: _; rpc error: code = Unknown desc = parse error : stage '| json layer7_something_detail=\"layer7_something_detail\"' : cannot parse expression [layer7_something_detail]: non-integer value: _; rpc error: code = Unknown desc = parse error : stage '| json layer7_something_detail=\"layer7_something_detail\"' : cannot parse expression [layer7_something_detail]: non-integer value: _\n"

Afer some experimentation the problem was found to be the number in the fields. Strangely it only affects some queries that use json fields.

Which issue(s) this PR fixes:
Fixes #

Checklist

• Documentation added
• Tests updated
• Is this an important fix or new feature? Add an entry in the CHANGELOG.md.
• Changes that require user attention or interaction to upgrade are documented in docs/sources/upgrading/_index.md

[grafanabot]

./tools/diff_coverage.sh ../loki-main/test_results.txt test_results.txt ingester,distributor,querier,querier/queryrange,iter,storage,chunkenc,logql,loki

Change in test coverage per package. Green indicates 0 or positive change, red indicates that test coverage for a package fell.

+           ingester	0%
+        distributor	0%
+            querier	0%
+ querier/queryrange	0%
+               iter	0%
+            storage	0%
+           chunkenc	0%
+              logql	0%
+               loki	0%

[grafanabot]

./tools/diff_coverage.sh ../loki-main/test_results.txt test_results.txt ingester,distributor,querier,querier/queryrange,iter,storage,chunkenc,logql,loki

Change in test coverage per package. Green indicates 0 or positive change, red indicates that test coverage for a package fell.

+           ingester	0%
+        distributor	0%
+            querier	0%
+ querier/queryrange	0%
+               iter	0%
+            storage	0%
+           chunkenc	0%
+              logql	0%
+               loki	0%

[splitice]

I'm not certain why the tests I added don't fail (without the fix). None the less they extend the test suite.

I've found what I beleive to be the issue and have gone about fixing it.

[grafanabot]

./tools/diff_coverage.sh ../loki-main/test_results.txt test_results.txt ingester,distributor,querier,querier/queryrange,iter,storage,chunkenc,logql,loki

Change in test coverage per package. Green indicates 0 or positive change, red indicates that test coverage for a package fell.

+           ingester	0%
+        distributor	0%
+            querier	0%
+ querier/queryrange	0%
+               iter	0%
+            storage	0%
+           chunkenc	0%
+              logql	0%
+               loki	0%

[splitice]

Confirmed fix of discovered issue with 6cdbaec

[chaudum]

Thanks for the contribution. Looks good to me! I think this change deserves a changelog entry :)

[chaudum]

I'm not certain why the tests I added don't fail (without the fix). None the less they extend the test suite.

We should investigate this before we merge :)

[dannykopping]

Thanks for the contribution @splitice - this looks great.

The reason why you can't replicate the issue in pkg/logql/syntax/parser_test.go is that it doesn't actually invoke the JSONExpressionParser I believe, it merely parses the expression.

You'll need a test in pkg/logql/log/jsonexpr/jsonexpr_test.go which will expose the issue:

I added

{
	"identifier with number",
	`utf8`,
	[]interface{}{"utf8"},
	nil,
},

under TestJSONExpressionParser and was able to replicate the issue and confirm your fix.

[splitice]

@dannykopping I figured it was something like that. Unfortunately had production to fix and it was easier to just test the known failing query against a build.

I'll integrate feedback and that test case.

[splitice]

@dannykopping @chaudum review feedback integrated & resosolved merge conflicts

[grafanabot]

./tools/diff_coverage.sh ../loki-main/test_results.txt test_results.txt ingester,distributor,querier,querier/queryrange,iter,storage,chunkenc,logql,loki

Change in test coverage per package. Green indicates 0 or positive change, red indicates that test coverage for a package fell.

+           ingester	0%
-        distributor	-0.3%
+            querier	0%
+ querier/queryrange	0%
+               iter	0%
+            storage	0%
+           chunkenc	0%
+              logql	0%
+               loki	0%

[grafanabot]

./tools/diff_coverage.sh ../loki-main/test_results.txt test_results.txt ingester,distributor,querier,querier/queryrange,iter,storage,chunkenc,logql,loki

Change in test coverage per package. Green indicates 0 or positive change, red indicates that test coverage for a package fell.

+           ingester	0%
+        distributor	0%
+            querier	0%
+ querier/queryrange	0%
+               iter	0%
+            storage	0%
+           chunkenc	0%
+              logql	0%
+               loki	0.7%

[dannykopping]

LGTM