report: HDR Histogram plot output format #413
Conversation
| if err := hist.Buckets.UnmarshalText([]byte(typ[4:])); err != nil { | ||
| return err | ||
| } | ||
| rep, report = vegeta.NewHistogramReporter(&hist), &hist |
There was a problem hiding this comment.
ineffectual assignment to rep (from ineffassign)
|
|
||
| var hdrHeader = []byte("Value Percentile TotalCount 1/(1-Percentile)\n\n") | ||
|
|
||
| var logarithmic = []float64{ |
There was a problem hiding this comment.
Can you explain how these magic values were chosen (preferably in a code comment)?
If they come from hdrhistogram it should be ok to keep them (with a comment), but if you picked them yourself I think it would be nicer to calculate them on startup.
There was a problem hiding this comment.
I based this code on https://github.com/tylertreat/hdrhistogram-writer/blob/master/percentiles.go#L8
| if q < 1.0 { | ||
| return 1 / (1 - q) | ||
| } | ||
| return float64(10000000) |
There was a problem hiding this comment.
Where's this constant coming from? (Comment would be helpful)
There was a problem hiding this comment.
I am not sure. https://github.com/tylertreat/hdrhistogram-writer/blob/master/writer.go#L65
I assume that's a constant used to indicate the upper bound of the 1 by quantile column.
| default: | ||
| switch { |
There was a problem hiding this comment.
I do not understand why this change is being made as part of this PR?
There was a problem hiding this comment.
We no longer want to assume a length prefix of the typ variable.
report.go
Outdated
| return err | ||
| } | ||
| rep, report = vegeta.NewHistogramReporter(&hist), &hist | ||
| } | ||
| return fmt.Errorf("unknown report type: %q", typ) |
There was a problem hiding this comment.
With this change you'll always return the error for -typ=hist, as you have no return after the rep, report = line.
tsenart
force-pushed
the
core/hdr-reporter
branch
2 times, most recently
from
196a9b5 to
342105a
Compare
tsenart
force-pushed
the
core/hdr-reporter
branch
from
342105a to
cbbec37
Compare
tsenart
force-pushed
the
core/hdr-reporter
branch
from
a870361 to
89e82de
Compare
|
@fxkr: Please take another look. |
|
Looking forward to have this released! @tsenart many thanks for doing this 😃 |
|
It’s released!
Sent via Superhuman iOS ( https://sprh.mn/[email protected] )
…On Fri, Jul 19 2019 at 1:45 PM, < ***@***.*** > wrote:
Looking forward to have this released!
@tsenart ( https://github.com/tsenart ) many thanks for doing this 😃
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub (
#413?email_source=notifications&email_token=AAAQPDY223BDDIGHY2S76ZLQAGSNHA5CNFSM4HYZNWU2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD2LMYIA#issuecomment-513199136
) , or mute the thread (
https://github.com/notifications/unsubscribe-auth/AAAQPD77TBILU5MBMGQYTZ3QAGSNHANCNFSM4HYZNWUQ
).
|
Plottable by https://hdrhistogram.github.io/HdrHistogram/plotFiles.html