Emit error when querying native histograms with extended functions #322
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
We saw panics on our setup because users tried to use the extended functions with native histograms, which led to rates calculated with only one sample in some windows. In this PR I am introducing a safeguard against this type of situation and emiting a warn when users try to query native histograms with extended functions. Signed-off-by: Pedro Tanaka <[email protected]> Fixing test case Signed-off-by: Pedro Tanaka <[email protected]> removing dead code Signed-off-by: Pedro Tanaka <[email protected]> Return error instead of warn Signed-off-by: Pedro Tanaka <[email protected]> fixing linting Signed-off-by: Pedro Tanaka <[email protected]>
fpetkovski
reviewed
Oct 25, 2023
fpetkovski
approved these changes
Oct 25, 2023
Co-authored-by: Filip Petkovski <[email protected]> Signed-off-by: Pedro Tanaka <[email protected]>
GiedriusS
reviewed
Oct 25, 2023
execution/scan/matrix_selector.go
Outdated
| @@ -351,8 +353,9 @@ loop: | |||
| // into the [mint, maxt] range are retained; only points with later timestamps | |||
| // are populated from the iterator. | |||
| // TODO(fpetkovski): Add max samples limit. | |||
| func selectExtPoints(it *storage.BufferedSeriesIterator, mint, maxt int64, out []Sample, extLookbackDelta int64, metricAppearedTs **int64) ([]Sample, error) { | |||
| func selectExtPoints(ctx context.Context, it *storage.BufferedSeriesIterator, mint, maxt int64, out []Sample, extLookbackDelta int64, metricAppearedTs **int64) ([]Sample, error) { | |||
There was a problem hiding this comment.
Why this context was added here? Seems like an unrelated change
There was a problem hiding this comment.
Good catch, I was using the context before to attach warns, but we saw it was better to return an error altogether.
Signed-off-by: Pedro Tanaka <[email protected]>
|
Seems like we got a flaky test, |
GiedriusS
approved these changes
Oct 30, 2023
pedro-stanaka
force-pushed
the
fix-extended-functions-native-histogram
branch
from
d88151d to
2e22f8f
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
We saw panics on our setup because users tried to use the extended functions with native histograms, which led to rates calculated with only one sample in some windows.
In this PR I am introducing a safeguard against this type of situation and emiting an error when users try to query native histograms with extended functions.