Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix scientific format in cumulative counters of histograms #131

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

surik
Copy link

@surik surik commented Aug 4, 2021

@ezh
Copy link

ezh commented Oct 29, 2021

@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
Copy link

@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
Copy link
Author

surik commented Oct 29, 2021

@SergeTupchiy good point, changed to 10

@Virviil
Copy link

Virviil commented Apr 5, 2022

@essen hi!

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

Thank you very much!

@essen
Copy link
Collaborator

essen commented Apr 5, 2022

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
Copy link
Collaborator

essen commented Apr 5, 2022

Also please rebase while at it! Thanks.

@surik
Copy link
Author

surik commented Apr 5, 2022

Added test and rebased.

@Virviil
Copy link

Virviil commented Apr 5, 2022

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

@essen essen self-requested a review April 5, 2022 19:18
@essen
Copy link
Collaborator

essen commented Apr 5, 2022

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

@Virviil
Copy link

Virviil commented Apr 6, 2022

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

So, we are ready for a merge, innit?

@essen
Copy link
Collaborator

essen commented Apr 7, 2022

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants