-
Notifications
You must be signed in to change notification settings - Fork 1
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
Check validity of consolidation function in sortBy #102
Conversation
This comment has been minimized.
This comment has been minimized.
can you add a regression test for this case? |
b6ab967
to
2315796
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
@@ -112,6 +112,9 @@ func (f *smartSummarize) Do(ctx context.Context, e parser.Expr, from, until int6 | |||
} | |||
|
|||
if t >= bucketEnd { | |||
if err := consolidations.CheckValidConsolidationFunc(summarizeFunction); err != nil { |
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.
I think it's better if we do this only once, and just after the summarizeFunction
is obtained, in line 53
@@ -124,6 +127,9 @@ func (f *smartSummarize) Do(ctx context.Context, e parser.Expr, from, until int6 | |||
|
|||
// last partial bucket | |||
if bucketItems > 0 { | |||
if err := consolidations.CheckValidConsolidationFunc(summarizeFunction); err != nil { |
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.
Ditto
@@ -59,6 +59,9 @@ func (f *legendValue) Do(ctx context.Context, e parser.Expr, from, until int64, | |||
nameBuf.Grow(len(r.Name) + len(methods)*5) | |||
nameBuf.WriteString(r.Name) | |||
for _, method := range methods { | |||
if err := consolidations.CheckValidConsolidationFunc(method); err != nil { |
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.
nit: we could move this check to line 51 so it's done at the beginning and some of the allocations are avoided
expr/functions/summarize/function.go
Outdated
@@ -157,6 +160,9 @@ func (f *summarize) Do(ctx context.Context, e parser.Expr, from, until int64, va | |||
|
|||
// last partial bucket | |||
if bucketItems > 0 { | |||
if err := consolidations.CheckValidConsolidationFunc(summarizeFunction); err != nil { |
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.
Same as for the smartSummarize
, we can move these checks just after the summarizeFunction
is obtained from the arguments
This comment has been minimized.
This comment has been minimized.
b89ca07
to
a4053e5
Compare
This comment has been minimized.
This comment has been minimized.
if err != nil { | ||
return ErrInvalidConsolidationFunc.WithMessage("invalid consolidation " + functionName) | ||
} |
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.
This can be deleted.
Also nitnit: the else branch is not necessary
5db03b4
to
6e4ee3b
Compare
This comment has been minimized.
This comment has been minimized.
return nil | ||
} | ||
|
||
return ErrInvalidConsolidationFunc.WithMessage("invalid consolidation " + functionName) |
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.
Sorry, I meant the whole return statement isn't needed, since it would just return at line 55
6e4ee3b
to
d34d68e
Compare
Go coverage report: Click to expand.
Go lint report: No issues found. 😎 |
…izer (#102) * Check validity of consolidation function in sortBy * Add test for checking validity of consolidation functions * Update validity checker function and add tests
This PR fixes a bug found in the sortBy() function, in which the specified consolidation function was not checked to verify that it was a valid function. This would cause a panic to occur, because in consolidations.SummarizeValues, the default case in the switch statement involves a string split which assumes that the consolidation function is 'p50'-'p999'. If the specified consolidation function was not 'p50'-'p999', then the panic would occur on the string split.