-
Notifications
You must be signed in to change notification settings - Fork 522
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: exemplars for compare function #4292
base: main
Are you sure you want to change the base?
fix: exemplars for compare function #4292
Conversation
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'm not convinced this PR is addressing the irregular sampling behaviour of the compare()
function.
Running the query range e2e test, I get very similar results to before, only with exemplars returned in a different series.
IMO, the first step should be extending that test or creating a new one to be able to verify the behaviour we're looking for: evenly distributed exemplars across all series.
Previously only the series labeled with I'll work on improving the test |
Right, these changes add exemplars to more series, but they don't address the core issues described in #4284—which are that sampling is not working as expected and it's returning too few exemplars in general. It's a frequent occurence that in a Also, I'm not sure that adding exemplars to |
What this PR does:
It fixes the exemplars selected for the compare function by adding the exemplars to the
baseline
andselection
series instead ofbaseline_total
andselection_total
Notes
Current implementation of the SimpleCombiner that we use for the second layer of aggregation of the compare function uses only one Exemplar bucket. This bucket does not discriminate between
baseline
andselection
, it bases its decision of dropping or not the exemplar just on the interval. That means that we can end with intervals without exemplars for eitherbaseline
orselection
. If we want to correct this we would need two bucketsWhich issue(s) this PR fixes:
Fixes #4284
Checklist
CHANGELOG.md
updated - the order of entries should be[CHANGE]
,[FEATURE]
,[ENHANCEMENT]
,[BUGFIX]