search

PromPHP / prometheus_client_php

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

fix: Correctly register histograms with Redis #47

Closed ngrewe closed 3 years ago

ngrewe commented 3 years ago

Hi folks,

I noticed that the LUA script for updating histograms currently does not work in many cases because it expected HINCRBYFLOAT to return the increment instead of the new value of field in the hash. It would also fail if the value returned by HINCRBYFLOAT differed in formatting from the input value. This PR fixes the issue.

Thanks,

Niels

LKaemmerling commented 3 years ago

Hey @ngrewe,

thank you for your contribution! Would it be possible for you to add a specific test? I'm not feeling well merging this without good test coverage.

Cheers,

ngrewe commented 3 years ago

Thanks Lukas! I've added a test, which was also very instructive because it revealed that the problem is only triggered under some very unusual circumstances (very high setting for serialize_precision in php.ini, values that happen to fall through the cracks of IEEE 754).

We also noticed that all hell breaks loose if the PHP process doing the collection has a different value for serialize_precision as the one observing the metric (essentially the collector process will see most values as 0 because the generated string for the buckets does not agree with what the observing process wrote to Redis). Not sure whether defending against that is worthwhile though…

ngrewe commented 3 years ago

Hi @LKaemmerling,

just checking: Is there anything else that you need me to do here?

Thanks,

Niels

LKaemmerling commented 3 years ago

Hey @ngrewe,

sorry, i completely missed it while being on vacation and then the easter holiday.

Everything is fine for me, looks good! Thank you! Good job!