Jimdo / prometheus_client_php

Prometheus instrumentation library for PHP applications
https://prometheus.io/docs/concepts/metric_types/
Apache License 2.0
281 stars 213 forks source link

Incorporate feedback from mailing list #6

Closed bracki closed 8 years ago

bracki commented 8 years ago

In response to https://groups.google.com/forum/#!topic/prometheus-developers/AgnAsR5qDiw we got the following feedback:

On 4 July 2016 at 08:26, <jan.b...@jimdo.com> wrote:

> Hi,
>
> we just released https://github.com/Jimdo/prometheus_client_php/.
>

I'd recommend reading
https://prometheus.io/docs/instrumenting/writing_clientlibs/ There's a few
things like label handling, method naming and use of floats that could be
improved.

>
> It's a PHP implementation of the Prometheus client library.
> It currently supports Counters, Gauges and Histograms. For expositing
> metrics we currently support the text/HTTP output.
>
> It should run on PHP>=5.3. For storing metrics in between requests, we
> chose to use Redis.
> It's fairly lightweight and easy to deploy. While it might seem a quite
> big dependency, we chose it for ease of implementation. Storing metrics in
> http://php.net/manual/en/book.apcu.php turned out to be cumbersome (not
> saying it couldn't be done).
>

Any network dependency is big, and makes things very complex. For example
you're presuming that Gauges are only set (
https://github.com/Jimdo/prometheus_client_php/blob/9e644b5ca7d9b91be58f6b9b121e5bef17a6b78f/src/Prometheus/RedisAdapter.php#L74)
which is not the only way they can be used.

Brian
schnipseljagd commented 8 years ago

https://prometheus.io/docs/instrumenting/writing_clientlibs/ According to the documentation I think these are MUSTs we have to do:

The redis protocol does not support doubles natively.

The current field content or the specified increment are not parsable as a double precision floating point number.

http://redis.io/commands/hincrbyfloat However AFAIK redis stores the values internally as double so this shouldn't be a problem. Just take a look into the java world: https://github.com/xetorthio/jedis/blob/master/src/main/java/redis/clients/jedis/Jedis.java#L596

schnipseljagd commented 8 years ago

I changed the naming to be more similar to the existing libraries, according to the style guide. I updated the readme and added some usage examples.

bracki commented 8 years ago

I consider this done with the changes from #12 and #13.