Skip to content

Conversation

charleskorn
Copy link
Contributor

@charleskorn charleskorn commented Nov 25, 2024

What this PR does

This PR extends MQE's tests to aid in catching native histogram use-after-return bugs, and fixes some issues they've uncovered.

Now, during tests in the streamingpromql package, any histograms in slices returned to the HPoint or FloatHistogram pools will immediately be mutated. This ensures that any possible use-after-return bugs are triggered deterministically.

Which issue(s) this PR fixes or relates to

(none)

Checklist

  • Tests updated.
  • [n/a] Documentation added.
  • CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX].
  • [n/a] about-versioning.md updated with experimental features.

@charleskorn charleskorn force-pushed the charleskorn/mqe-nh-bugs branch from fad9a56 to d59e928 Compare November 25, 2024 05:12
@charleskorn charleskorn marked this pull request as ready for review November 25, 2024 05:28
@charleskorn charleskorn requested a review from a team as a code owner November 25, 2024 05:28

h.ZeroCount = 12345678
h.Count = 12345678
h.Sum = 12345678
Copy link
Contributor

Choose a reason for hiding this comment

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

(nit) it's very unlikely, but if we write a test with these values elsewhere the mangle won't work. Maybe for just one of these (eg Sum) we multiply it or similar.

Suggested change
h.Sum = 12345678
h.Sum = h.Sum * 9.87654321

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm keen to keep this as simple as possible and to make the resulting histogram easy to identify. I think the likelihood of us using a histogram with these values elsewhere and also having a bug that is then hidden because of that is very, very unlikely, as you say.

Copy link
Contributor

Choose a reason for hiding this comment

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

Fair enough, I don't hold this strongly.

Copy link
Contributor

@jhesketh jhesketh left a comment

Choose a reason for hiding this comment

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

lgtm, sans a suggestion

@charleskorn charleskorn enabled auto-merge (squash) November 26, 2024 04:12
@charleskorn charleskorn merged commit a9a5ff3 into main Nov 26, 2024
29 checks passed
@charleskorn charleskorn deleted the charleskorn/mqe-nh-bugs branch November 26, 2024 04:25
@jhesketh jhesketh mentioned this pull request Dec 2, 2024
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.

2 participants