fix(prometheus): use lezer tree instead of greedy regex for range duration - #86
fix(prometheus): use lezer tree instead of greedy regex for range duration#86algojogacor wants to merge 1 commit into
Conversation
…e duration
The PromQL query builder used /\[(.+)\]/ to extract range duration
from function calls. This greedy regex matched from the first '[' to
the last ']', breaking when label filter values contained brackets.
Example: rate(metric{label=~"value[0-9]"}[5m]) would capture
'0-9]"}[5m' instead of '5m'.
Replace the regex with lezer syntax tree traversal using MatrixSelector
and NumberDurationLiteral nodes, which correctly identify the range
duration regardless of brackets in label values.
Fixes: grafana/grafana#124522
Signed-off-by: Arya Rizky <arya@algojogacor.dev>
|
|
2 similar comments
|
|
|
|
|
@algojogacor Thank you for your contribution. I'm reviewing it but I won't be able to merge it unless you sign your commits. Our CI dictates that all commits must be signed. Also you have to sign CLA: #86 (comment) |
itsmylife
left a comment
There was a problem hiding this comment.
Thanks for working on this. Here is my review: The bug is real and your approach of using the lezer tree instead of regex is the right direction.
But there is a critical mistake in the implementation. The PR uses getChild(NumberDurationLiteral) to find the duration inside MatrixSelector, but if you look at the lezer-promql grammar, the correct node type inside a MatrixSelector is NumberDurationLiteralInDurationContext, not NumberDurationLiteral:
MatrixSelector {
expr "[" NumberDurationLiteralInDurationContext "]"
}
These are two different node IDs (140 vs 141). The getChild() call will silently return null for every valid duration literal, so interval stays '' and params is never populated. This means the fix would actually break parsing of all range functions, not just the ones with [ in label values.
Also, the original code has a comment explaining it intentionally uses the regex shortcut to handle Grafana template variables like $__rate_interval, because the lezer parser produces an error node for those (they are not valid PromQL). The PR does not address this case at all. A pure tree-traversal approach needs a fallback for when the MatrixSelector child is an error node.
Please add a test case for rate(metric{label=~"value[0-9]"}[5m]) and verify it produces params: ['5m'] and not an empty array.
Thanks a lot.
|
This pull request has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in 2 weeks if no further activity occurs. Please feel free to give a status update or ping for review. Thank you for your contributions! |
Summary
Fixes #97
The PromQL query builder used the greedy regex
/\[(.+)\]/to extract range duration (e.g.,5minrate(metric[5m])) from function call text. This broke when label filter values contained[characters, because the regex matched from the first[(inside the label value) to the last](the duration bracket).Root Cause
parsing.tsline 254:getString(expr, node).match(/\[(.+)\]/)extracts the full function call text and applies a greedy regex. For a query likerate(metric{label=~"value[0-9]"}[5m]), the regex captures0-9]"}[5minstead of5m.Fix
Replace the regex with lezer syntax tree traversal. The
FunctionCallBodyalready has a parsedMatrixSelectorchild node. From theMatrixSelector, extract theNumberDurationLiteralchild to get the exact duration text.MatrixSelectorfrom@prometheus-io/lezer-promqlbody.getChild(MatrixSelector)→getChild(NumberDurationLiteral)getString(expr, durationNode)patternChanges
packages/grafana-prometheus/src/querybuilder/parsing.ts: Replace greedy regex with lezer tree traversal (+11 -6lines)packages/grafana-prometheus/src/querybuilder/parsing.test.ts: Add test for[in label filter values (+23 -0lines)Testing
[in label value:rate(metric{label=~"value[0-9]"}[5m])→ correctly parses duration as5m✅[in label (existing test preserved):rate(counters_logins{app="frontend"}[5m])→ still works ✅avg(rate(...)[$__rate_interval])→ still works ✅