Address misreported value on exemplars for counters.#776
Open
jdmarshall wants to merge 1 commit into
Open
Conversation
Fixes prometheus#621 Signed-off-by: Jason Marshall <jdmarshall@users.noreply.github.com>
c6ae2cf to
9df3fc8
Compare
Contributor
Author
|
I initially had planned to rename the variables to make the distinction clear, but unfortunately the 'value' is exposed as part of the public API and I don't want to break existing code. Even though technically any existing code is already broken. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
After one AI user filed a PR to fix this the wrong way, I finally went tracing through the code to understand what the heck is wrong as implemented. The real issue turned out to be pretty dumb: mislabeled variables resulting in mischaracterization of the data it represents.
We're calling the increment step 'value' but we use the 'value' to call
setDeltainincWithoutExemplars()and delegating to it fromincWithExemplars()but then using the passed-in delta as a 'value' again when updating the 'value' field in the exemplar, which is not the same meaning of 'value'.Within minutes of updating the ticket to set the labels and the problem, a completely separate user filed a PR. Maybe I should not have set helpWanted when a PR was already open. However, both PRs had an identical new unit test, and both of them were identically asserting the wrong thing, and solving the issue the wrong way.
At this point I had invested more time in both PRs than the authors had. The entire problem was front-of mind, and the existing test is wrong but written in a way where either interpretation would be correct. Clearly they are not.
Fixes #621