Naïve fix for optimize bug with line_format and json expression parser

[dannykopping]

Signed-off-by: Danny Kopping [email protected]

What this PR does / why we need it:
This PR implements a naïve fix for the bug listed below.

Which issue(s) this PR fixes:
Fixes #6314

Special notes for your reviewer:
The more correct way will be to unify the JSONExpressionParser and LabelParserExpr.

func (e *LabelParserExpr) Stage() (log.Stage, error) {
	switch e.Op {
	case OpParserTypeJSON:
		return log.NewJSONParser(), nil
	case OpParserTypeLogfmt:
		return log.NewLogfmtParser(), nil
	case OpParserTypeRegexp:
		return log.NewRegexpParser(e.Param)
	case OpParserTypeUnpack:
		return log.NewUnpackParser(), nil
	case OpParserTypePattern:
		return log.NewPatternParser(e.Param)
	default:
		return nil, fmt.Errorf("unknown parser operator: %s", e.Op)
	}
}

Ultimately the JSONExpressionParser should be instantiated in here, since this is how all other parsers work. I must've missed this somehow when initially implementing the JSONExpressionParser.

It'll take a bit of work because in pkg/logql/syntax/expr.y

labelParser:
    JSON           { $$ = newLabelParserExpr(OpParserTypeJSON, "") }
  | LOGFMT         { $$ = newLabelParserExpr(OpParserTypeLogfmt, "") }
  | REGEXP STRING  { $$ = newLabelParserExpr(OpParserTypeRegexp, $2) }
  | UNPACK         { $$ = newLabelParserExpr(OpParserTypeUnpack, "") }
  | PATTERN STRING { $$ = newLabelParserExpr(OpParserTypePattern, $2) }
  ;

all of the LabelParserExprs are expecting a string argument, while the JSONExpressionParser expects a slice of log.JSONExpression.

I took a swing at this last week but there's a bit of complexity here which I don't have time right now to address, so in the interim I've implemented this simple fix which will make the problem go away, however hacky it may be.

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.1%
+               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.1%
+               loki	0.7%

[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.1%
+               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.3%
+            querier	0%
+ querier/queryrange	0%
+               iter	0%
+            storage	0%
+           chunkenc	0%
+              logql	0.1%
+               loki	0%

[chaudum]

Think it's ok to do the quick fix now. Have a suggestion for the changelog entry, though.

[dannykopping]

Think it's ok to do the quick fix now. Have a suggestion for the changelog entry, though.

That's indeed more clear, thanks @chaudum 👍
Can you approve pls?

[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.1%
+               loki	0%

[chaudum]

🚢