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 flaky TestManagedRegistry_externalLabels #2911

Merged
merged 5 commits into from
Sep 12, 2023
Merged

Fix flaky TestManagedRegistry_externalLabels #2911

merged 5 commits into from
Sep 12, 2023

Conversation

LasseHels
Copy link
Contributor

@LasseHels LasseHels commented Sep 11, 2023

What this PR does:
This pull request implements a solution to the flaky TestManagedRegistry_externalLabels test.

TestManagedRegistry_externalLabels is flaky since it relies on a specific label order which is not guaranteed. The solution is particularly tricky as neither sample order nor label order is guaranteed across the test suite, meaning collectRegistryMetricsAndAssert must assert agnostic of both sample and label order.

This pull request contains no functional changes to Tempo.

Which issue(s) this PR fixes:
Fixes #2881.

Checklist

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

Copy link
Member

@joe-elliott joe-elliott left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one has plagued us for awhile, but I've never had time to look at it.

One thing that might make it cleaner would be to sort by timestamp first? then at least you could compare expectedSamples[i] vs actualSamples[i]?

@LasseHels
Copy link
Contributor Author

One thing that might make it cleaner would be to sort by timestamp first? then at least you could compare expectedSamples[i] vs actualSamples[i]?

Good idea!

Action taken: instead of my clumsy nested loop, we now prefer a much cleaner implementation of sorting both slices, and then asserting equality on expected[i] and actual[i].

Sorting by timestamp was not sufficient, as samples can have identical timestamps. I'm using sample.String() to sort. This is probably fine, but means that the tests now have an implicit dependency on the String() method of sample; changing the String() method risks breaking the tests. I think I'm OK with this, as the alternative is a more complicated test, which is also not ideal.

Copy link
Member

@joe-elliott joe-elliott left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm. thanks for the test fix!

@joe-elliott joe-elliott merged commit 9b89020 into grafana:main Sep 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

TestManagedRegistry_externalLabels is flaky
2 participants