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 hackeny.POOL.queue_count metric name #583

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

GregMefford
Copy link
Contributor

It looks like the code was actually populating metrics under the queue_count name, but declaring the histogram as queue_counter. I didn't actually test that this fixes it, but wanted to get a PR opened to see whether this is the name to use or not. It seems like this would be more consistent with the other metric names.

@benoitc
Copy link
Owner

benoitc commented Jul 25, 2019

main difficulty with it is that it's introducing a breaking change for those who're using them , so it will have to be in the next major release I guess. Otherwise I agree with the change. Let's keep it that PR around

@GregMefford
Copy link
Contributor Author

Actually I think the way this PR is currently written, it should be a bugfix instead of a breaking change, right? The code currently uses the queue_count name when it updates the histogram:

It just never declares that metric because it declares it as queue_counter. I think this means that the metric already won't show up in metrics systems where you need to pre-declare your metrics before using them. So the only breaking change is that it won't declare the queue_counter metric, which is never actually populated with data anyway.

@nkmanolovsumup
Copy link

@GregMefford @benoitc Wouldn't changing the update statement to queue_counter be more appropriate rather than renaming the metric itself? In that case it won't be a breaking change, the queue_counter histogram will actually work and you can change it to queue_count in the next major release if you find the name more appropriate.

@GregMefford
Copy link
Contributor Author

I might be misunderstanding something, but it looks like in the code I linked above, it already is sending the metric updates as queue_count, it just incorrectly declares the metric as queue_counter.

@nkmanolovsumup
Copy link

@GregMefford yeah, sorry, for some reason I thought the opposite was done. I also think you are right and don't see how this could be a breaking change. Surely no one is relying on an empty histogram, right?

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