search

deadtrickster / prometheus.erl

Prometheus.io client in Erlang
MIT License
341 stars 117 forks source link

Add quantiles to summary using quantiles_estimator package. #116

Closed hairyhum closed 3 years ago

hairyhum commented 3 years ago

Trying to address #18 Using https://github.com/odo/quantile_estimator to calculate quantiles.

The merge function is not ideal, but works well enough on simple samples.

In terms of backwards compatibility I can create a separate module for summaries with quantiles or add a config to be able to enable/disable them.

deadtrickster commented 3 years ago

Hi, thank you for looking into this. I will be more comfortable if summary with quantiles will be put in the new module.

deadtrickster commented 3 years ago

@essen @gerhard what you think?

gerhard commented 3 years ago

Nice to bump into you @hairyhum 🙂

This does look useful & I like the clean separation - good call @deadtrickster.

I am somewhat concerned about the lack of change in https://github.com/odo/quantile_estimator. Not a problem in itself, but do we know if it works on OTP 23? How about OTP 24? While 24 is not GA yet - RC1 shipped a few weeks ago -, I am wondering how much work there will be to get this new library working in this very important upcoming version.

Do we know why the Travis CI builds are failing?

hairyhum commented 3 years ago

I'm running my tests on OTP23. I can run with 24RC as well.

gerhard commented 3 years ago

OK, I am assuming that all tests are passing on 23.

Yes please for 24.

hairyhum commented 3 years ago

I ran a quick test on 24.0-rc1 with rebar3 eunit and there is no issues other than compile warnings about bifs.

deadtrickster commented 3 years ago

Merged, thanks! I will let it live in master for some time before cutting release, want to play with 24 myself.