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

fix: exemplars for compare function #4292

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

javiermolinar
Copy link
Contributor

@javiermolinar javiermolinar commented Nov 6, 2024

What this PR does:

It fixes the exemplars selected for the compare function by adding the exemplars to the baseline and selection series instead of baseline_total and selection_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 and selection, 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 either baseline or selection. If we want to correct this we would need two buckets

Which issue(s) this PR fixes:
Fixes #4284

Checklist

  • Tests updated
  • CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

@javiermolinar javiermolinar marked this pull request as ready for review November 6, 2024 14:20
@javiermolinar javiermolinar added the type/bug Something isn't working label Nov 13, 2024
Copy link
Member

@mapno mapno left a 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.

@javiermolinar
Copy link
Contributor Author

javiermolinar commented Nov 13, 2024

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 baseline_total and selection_total had exemplars. Now all can have exemplars. This doesn't mean that all the series are going to have at least one exemplar due to the way the bucket works. Sampling by interval only, fixing that will require much more work.

I'll work on improving the test

@mapno
Copy link
Member

mapno commented Nov 13, 2024

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 baseline_total and selection_total had exemplars. Now all can have exemplars. This doesn't mean that all the series are going to have at least one exemplar due to the way the bucket works. Sampling by interval only, fixing that will require much more work.

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 compare() query most series (_totals or not) don't have exemplars, even though there are traces that match those series.

Also, I'm not sure that adding exemplars to baseline and selection series is an improvement over baseline_total and selection_total, as the latter are supersets of the former.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type/bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

compare() returns fewer exemplars than expected
2 participants