deadtrickster / prometheus.erl

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

Fix scientific format in cumulative counters of histograms #131

Open surik opened 3 years ago

surik commented 3 years ago

Fixes:

ezh commented 2 years ago

@deadtrickster We also have a similar issue with our application. It wouldn't require a lot of resources to review this PR. I see that CI is stuck. It would be great if someone would help to push the fix forward. 👀

Could we help somehow?

SergeTupchiy commented 2 years ago

@surik, @deadtrickster, This fix works fine, but I think it's worth decreasing the number of digits from to 20 to, say, 10. 20 digits is big enough and can't remove some floating-point imprecision...

3> float_to_list(0.2,[{decimals, 20},compact]).
"0.2000000000000000111"
4> float_to_list(0.1,[{decimals, 20},compact]).
"0.10000000000000000555"
<0.5922.1>) call prometheus_text_format:bound_to_label_value(0.1) ({prometheus_text_format,
                                                                     emit_histogram_bucket,
                                                                     4})
(<0.5922.1>) returned from prometheus_text_format:bound_to_label_value/1 -> "0.10000000000000000555"
(<0.5922.1>) call prometheus_text_format:bound_to_label_value(0.2) ({prometheus_text_format,
                                                                     emit_histogram_bucket,
                                                                     4})
(<0.5922.1>) returned from prometheus_text_format:bound_to_label_value/1 -> "0.2000000000000000111"
surik commented 2 years ago

@SergeTupchiy good point, changed to 10

Virviil commented 2 years ago

@essen hi!

It seems that you can review and help us to merge this PR!

Thank you very much!

essen commented 2 years ago

Sounds sensible. Can we take this opportunity to add a test in https://github.com/deadtrickster/prometheus.erl/blob/master/test/eunit/format/prometheus_text_format_tests.erl ? For example the buckets from the original ticket could be used in a histogram test similar to https://github.com/deadtrickster/prometheus.erl/blob/master/test/eunit/format/prometheus_text_format_tests.erl#L151-L175 (please add as a new test)

essen commented 2 years ago

Also please rebase while at it! Thanks.

surik commented 2 years ago

Added test and rebased.

Virviil commented 2 years ago

@surik great job! @essen @deadtrickster it seems that we are ready for merge?

essen commented 2 years ago

Approved, running CI, I'll be back Thursday.

Virviil commented 2 years ago

@essen @deadtrickster @surik It seems that all CI is successfully passed! Good job!

So, we are ready for a merge, innit?

essen commented 2 years ago

I can't merge personally because Travis didn't run. I don't know why. I'm afraid @deadtrickster will have to take a look and/or merge directly. Cheers.