Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix the parsing of threshold name tags containing tokens #2515

Merged
merged 3 commits into from
May 5, 2022

Conversation

oleiade
Copy link
Member

@oleiade oleiade commented May 5, 2022

@efdknittlfrank pointed out that v0.38.0 presented a bug when thresholds were defined following the URL grouping documentation.

Indeed, using colons and curly braces inside a thresholds tags definition would confuse the parser, and
lead to an unclear error message. This PR modifies the parser to handle curly braces and colons accordingly to the pointed out use case. Namely, the parser makes fewer assumptions as to the tags definitions' content, and allows for a wider range of symbols that would have conflicted with the parsing before {, }, :.

A threshold expression such as http_req_duration{name:http://${}.com} is now valid.

Notable changes:

  • closing curly brace is now parsed from the right
  • we ensure the closing curly brace is the last character in the string.
  • knowing the position of the opening and closing tag definition curly
    braces, we extract the whole definition's substring based on that.

closes #2512

@efdknittlfrank pointed out that v0.38.0 presented a bug when thresholds
were defined for URL grouping. Indeed, using colons and curly braces
inside of a thresholds tags definition would confuse the parser, and
lead to an unclear error.

This commit ensures that we parse thresholds tags properly:
* closing curly brace is now parsed from the right
* we ensure the closing curly brace is the last character in the string.
* knowing the position of the opening an closing tag definition curly
braces, we extract the whole definition's substring based on that.
@oleiade oleiade force-pushed the hotfix/0.38.0/thresholds-url-grouping-parsing branch from 39828ee to 7b57242 Compare May 5, 2022 14:14
@oleiade oleiade requested a review from na-- May 5, 2022 14:14
@oleiade oleiade force-pushed the hotfix/0.38.0/thresholds-url-grouping-parsing branch from 4caf161 to 652f7be Compare May 5, 2022 14:32
Copy link
Contributor

@codebien codebien left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@oleiade oleiade merged commit 0c5aab3 into master May 5, 2022
@oleiade oleiade deleted the hotfix/0.38.0/thresholds-url-grouping-parsing branch May 5, 2022 15:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Threshold sub-metric selectors containing reserved symbols would fail
3 participants