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

[BUGFIX] Alerts: don't reuse payload after relabeling. #13735

Merged
merged 2 commits into from Mar 8, 2024

Conversation

bboreham
Copy link
Member

@bboreham bboreham commented Mar 8, 2024

Fixes #13720

This PR has two commits: the first fixes the bug and the second cleans up something that I first thought could cause the bug.
On closer inspection I think all the variables captured by closure are local to the loop, but I think it's best to rewrite it in a transparently safer fashion.

I removed a comment referring to a desired future optimisation, since it does not add clarity.
If performance here is a problem, this will be found via profiling.

Also clarify why these variables are being cleared.

Signed-off-by: Bryan Boreham <bjboreham@gmail.com>
Avoids possible false sharing between loops.

Plausibly there is no problem in the current code, but it's easy enough to write it more safely.

Signed-off-by: Bryan Boreham <bjboreham@gmail.com>
Copy link
Contributor

@nabokihms nabokihms left a comment

Choose a reason for hiding this comment

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

Lgtm

@machine424
Copy link
Collaborator

Thanks,
I see the test was passing sometimes, I think it's better to have one that reproduces the issue all the time.

@bboreham
Copy link
Member Author

bboreham commented Mar 8, 2024

I see the test was passing sometimes, I think it's better to have one that reproduces the issue all the time.

Agreed in principle, but the order in which things happen depends on map iteration, which is randomised.
I can't offhand think of a way to guarantee a bad ordering.

@bboreham bboreham merged commit e8bf2ce into prometheus:main Mar 8, 2024
24 checks passed
@bboreham bboreham deleted the fix-notifier-relabel branch March 8, 2024 12:29
@machine424
Copy link
Collaborator

I see, maybe in this case, it's better to ensure deterministic ordering.
But let's wait for that rewrite/refactoring.

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.

TestHandlerSendAllRemapPerAm is flaky
4 participants