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 bug corrupting Prometheus timeseries __name__ label #277

Merged
merged 1 commit into from
Nov 6, 2023

Conversation

alibresco
Copy link
Contributor

@alibresco alibresco commented Nov 5, 2023

I have also been experiencing issues like the ones described in #259 and #169 where Prometheus outputs are sometimes dropped or incorrect.

I believe this is due to a bug with how the __name__ label gets added to the list of labels when constructing a timeseries. The __name__ label is appended without a copy or assignment, meaning it will overwrite whatever was appended previously, so long as there is space remaining in the array. If GetLabels returns a slice with length == capacity, the bug does not seem to occur because append forces a copy.

A consequence of this bug is that a value can get written to the wrong timeseries.

I've written a test that reproduces this behavior. The test fails without the fix and passes with the fix.

I don't write a lot of Go, so let me know if I've gotten anything wrong here or there's a better way to solve this. I stumbled upon this issue while investigating incorrect gNMIC Prometheus writes that I was observing.

Copy link

google-cla bot commented Nov 5, 2023

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@spolack
Copy link

spolack commented Nov 6, 2023

Can you please provide a amd64 build? I can confirm whether this fixes #259

@karimra
Copy link
Collaborator

karimra commented Nov 6, 2023

@alibresco This makes sense, I will try it out and merge if all good.

@karimra
Copy link
Collaborator

karimra commented Nov 6, 2023

Thanks for the contribution and for writing tests.

@karimra karimra merged commit f4c7436 into openconfig:main Nov 6, 2023
2 checks passed
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.

3 participants