benoitc
Fix hackeny.POOL.queue_count metric name
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.
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
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:
- https://github.com/benoitc/hackney/blob/74159da340a03f3a3a0cc944814ff4338ab07ce6/src/hackney_pool.erl#L274
- https://github.com/benoitc/hackney/blob/74159da340a03f3a3a0cc944814ff4338ab07ce6/src/hackney_pool.erl#L382
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.
@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.
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.
@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?