-
Notifications
You must be signed in to change notification settings - Fork 23
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
Fixing out of index error during metric sorting #398
Conversation
c69ad50
to
c7b349a
Compare
I was thinking about the case in which we had two other metrics:
With current implementation, these two results are not sorted out by the last part. |
@emadolsky : as you can see sorting only happening in case of globs. Which query you want to test for these metrics? |
@grzkv @emadolsky : I wrote split function and test, please re-review. |
expr/sort.go
Outdated
if part < len(parts) { | ||
return parts[part] | ||
} | ||
// TODO: that should never happen, maybe we should log it |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it's clear that this execution path is wrong and shouldn't happen, it makes sense to return an error. That would be safer: we will make the unexpected behavior evident, and the error will clearly indicate what is wrong.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea, but I'm not sure how to do so - as you can see getPart
is used in sort.Stable(AlphabeticallyByPart(metrics, i))
and sort.Stable
do not generate or propagate errors. I will mention that in TODO, though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approved, even though there are some minor comments on my side.
@deniszh It's your call whether to address them now or later.
What issue is this change attempting to solve?
It's related to this PR go-graphite/carbonapi#128. Before this change it's possible that query can have more dots than requested metric. Obvious example:
Metrics:
Query:
a.{*.b1,*}.c.*
Query looks strange, but it's valid and it was taken from real example.
So, in the case above sorting will cause out of index error in
getPart()
function (and caused panic, intercepted only in http module, which is not good).How does this change solve the problem? Why is this the best approach?
Instead of splitting query by dots using simple
strings.Split()
I'm splitting it by using tricky regex, which will ignore dots inside square or curly bracket pairs. In that case sort will work properly and will not cause errors in queries above.I also included range protection in getPart() but proper splitting should fix underlying issue and not just mask it, as range protection alone doing. Performance wise that should not do much hurt - regex compiles once, split will be done once for every query, only if it contains globs.
How can we be sure this works as expected?
I added 2 more test cases for sort - one for error which I fixed and another to test that sort by glob also happening.