search

benoitc / hackney

simple HTTP client in Erlang
Other
1.34k stars 427 forks source link

Fix hackeny.POOL.queue_count metric name #583

Open GregMefford opened 5 years ago

GregMefford commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

@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 commented 5 years ago

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 commented 5 years ago

@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?