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

Add validation for brackets in plain metrics and regex error parsing for tagged metrics #284

Merged

Conversation

mchrome
Copy link
Collaborator

@mchrome mchrome commented Jul 3, 2024

If an incorrect regex is sent to clickhouse, graphite-clickhouse will return 500: Storage error which is not very informative. This PR adds validation before that can happen to avoid making unnecessary requests to clickhouse and makes graphite-clickhouse return an error that explains the reason why.

For plain metrics: check if all brackets are closed correctly within each node of the path.
Example: a.b.c{d,e}}.f would now return 400: query has unmatched brackets

For tagged metrics: parse regex error from clickhouse.
Example: seriesByTag('name=myMetric', 'tag=*abc*') would now return 400: Incorrect regex syntax

@mchrome mchrome changed the title Add validation for plain and tagged metrics Add validation for brackets in plain metrics and regex error parsing for tagged metrics Oct 28, 2024
@mchrome mchrome marked this pull request as ready for review October 28, 2024 08:58
pkg/where/match.go Outdated Show resolved Hide resolved
if len(stack) == 0 {
return true
}
if stack[len(stack)-1] == matchingBracket[c] {
Copy link
Contributor

Choose a reason for hiding this comment

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

more readable:

if stack[len(stack)-1] != matchingBracket[c] {
    return true
}

stack = stack[:len(stack)-1]

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done in 38f3ea7

@mchrome mchrome merged commit c9cbfa6 into go-graphite:master Oct 30, 2024
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants