HdrHistogram / HdrHistogramJS

TypeScript port of HdrHistogram
BSD 2-Clause "Simplified" License
123 stars 21 forks source link

Loss of precision for values near lowestDiscernibleValue #35

Closed jberryman closed 3 years ago

jberryman commented 3 years ago

With small values we lose precision regardless of numberOfSignificantValueDigits

var hdrHistogramJs = require("hdr-histogram-js")

x = hdrHistogramJs.build()
x.recordValue(2.33)
x.recordValue(2.69)
x.recordValue(10.1)
x.recordValue(50)
x.toJSON()

# => Object {p50: 2, p75: 10, p90: 50, p97_5: 50, p99: 50, p99_9: 50, p99_99: 50, p99_999: 50, max: 50, totalCount: 4}

Whereas if I move the decimal point to the right two places I get the expected:

Object
p50: 269
p75: 1010
p90: 5000
p97_5: 5000
p99: 5000
p99_9: 5000
p99_99: 5000
p99_999: 5000
max: 5000
totalCount: 4

Is this a bug or just a property of HdrHistogram? I tried setting lowestDiscernibleValue: 0.001 to see if that would fix things but there's a static check for >= 1

alexvictoor commented 3 years ago

Hello Brandon, I guess there is room for improvement in the documentation: You cannot record non-integer values. In the original Java implementation, there is a DoubleHistogram class that allows to record non-integer values but to be honest I have never found any use for it. Let me know if you want to see it in this lib (the implementation requires a significant amount of work)

On Mon, Jun 21, 2021 at 10:55 PM Brandon Simmons @.***> wrote:

With small values we lose precision regardless of

var hdrHistogramJs = require("hdr-histogram-js")

x = hdrHistogramJs.build() x.recordValue(2.33) x.recordValue(2.69) x.recordValue(10.1) x.recordValue(50) x.toJSON()

=> Object {p50: 2, p75: 10, p90: 50, p97_5: 50, p99: 50, p99_9: 50, p99_99: 50, p99_999: 50, max: 50, totalCount: 4}

Whereas if I move the decimal point to the right two places I get the expected:

Object p50: 269 p75: 1010 p90: 5000 p97_5: 5000 p99: 5000 p99_9: 5000 p99_99: 5000 p99_999: 5000 max: 5000 totalCount: 4

Is this a bug or just a property of HdrHistogram? I tried setting lowestDiscernibleValue: 0.001 to see if that would fix things but there's a static check for >= 1

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/HdrHistogram/HdrHistogramJS/issues/35, or unsubscribe https://github.com/notifications/unsubscribe-auth/AADKXNF5JDIQUYZZ2KOS5MTTT6RK3ANCNFSM47CKEMIA .

jberryman commented 3 years ago

Ah, I see in the docs:

The number passed as a parameter is expected to be an integer. If it is not the case, the decimal part will be ignored.

...so my bad.

My workaround is going to be to convert ms to μs before inserting and then convert the output of the histogram back to ms; one possible improvement would be for the library to offer this scaling factor at the perimeter in the API (this could be implicit in the existing lowestDiscernibleValue param). Presumably that could live at the perimeter of the code you have now.