feat(subscriber): Record and send poll times with HdrHistogram #47
Conversation
There was a problem hiding this comment.
thanks for working on this, this is really cool!
i had some thoughts on how we could potentially save some data on the wire by only sending the complete histogram on-demand. although hdrhistogram is pretty space-efficient, sending one for each task is still potentially a lot of data in a production system, where there might be a very large number of tasks.
let me know what you think of my suggestions?
|
Thanks for the screenshot, that looks super cool! What exactly is the graph showing? |
|
It should show... the entire histogram 😅 with the bucket count equal to the number of horizontal pixels in the render area. It looks like tui's I also played with
Also, there may be better ways to visualize this since it's a latency histogram. So I'm okay with taking it out. |
|
Sideline comment — definitely provide an option to render a CDF if you can. It's a little harder to read than a histogram, but provides much more data (e.g., what is the 95th percentile latency) at a glance. |
oguzbilgener
force-pushed
the
poll-histogram
branch
from
df49e26 to
924e175
Compare
console/src/view/task.rs
Outdated
| .map(|pairs| { | ||
| pairs.into_iter().map(|pair| { | ||
| format!( | ||
| "p{}: {:.prec$?}", |
There was a problem hiding this comment.
nit: it would be nice to bold the percentile label ("pNN") like we do for labels elsewhere?
There was a problem hiding this comment.
Updated:
To add more percentiles with different lengths like p5 or p99.9 I need to account for the spacing better, but not sure if we care about those at the moment? It would be nice to fill up those two columns of percentiles though 😄
There was a problem hiding this comment.
alternatively, we could just make the poll times section a couple lines longer, and make one column of percentiles. then, the percentiles box wouldn't have to be 50% of the section, and we could make the histogram wider...and get more vertical space, which would (hopefully) allow us to draw the histogram with a little more vertical resolution.
but, we can tweak the layout and stuff in subsequent PRs. the important part for this branch, IMO, is getting the histogram wired through from the subscriber to the console, and having a basic setup for drawing it. which we've done! so, let's just merge this and keep working on making it look nice as a follow-up. :)
| } | ||
| } | ||
| } else { | ||
| // TODO: handle connection error, print details somewhere? Related to Issue #30 |
There was a problem hiding this comment.
I can make another small PR to address this after #57
oguzbilgener
force-pushed
the
poll-histogram
branch
from
fd558dc to
992f01c
Compare
There was a problem hiding this comment.
this looks good to me! would be nice to get a 👍 from @seanmonstar as well, if you have a minute to look it over?
|
yay! thanks for all your hard work on this, @oguzbilgener! |
Closes #36
As suggested in the issue, this PR uses
HdrHistogramto record task poll times as nanoseconds. The histogram is serialized into bytes using theV2Serializer.The subscriber now offers a second RPC stream called TaskDetails for a given task ID.
The console app starts a details stream when you view the details of a task and stops the stream when you leave it.
To demonstrate that it works, I also added some stats and a small chart to the task view using the histogram data.
But I won't insist on merging this part, since you may have other plans for the task details view.
I've got one TODO left and I would appreciate your suggestions on it.