How to use the `checked_*` methods in `histogram::Snapshot`?
Closed this issue · 5 comments
I am using a previous version of the histogram crate and I've noticed that there are a lot of changes in the latest version (I appreciate the much-improved documentation!). However, I'm struggling to adapt my code to use the new operations of Histogram.
I've been relying on the Histogram::merge of a previous version to combine the histograms produced by different threads. In the new version, the equivalent of this method, which is Histogram::checked_add, has been hidden behind pub(crate), so my only option is to take a snapshot of the histogram and use the Snapshot::checked_merge/Snapshot::checked_add instead. However, these methods require the time range of the snapshots to match, which is not possible in my case, so I'm wondering if it is possible to add the non-checked versions of these methods.
I'm curious as to how these checked_* methods are intended to be used. They require the snapshots' time ranges to be exactly the same. These ranges are assigned from system time, which means you'd have to somehow create 2 histograms at the same time, and also create their snapshots at the same time. Is it true or did I miss something?
Is there a reason to not use the AtomicHistogram type? I typically have a shared histogram that all threads increment and then you can produce periodic snapshots to get percentiles across specific time windows, or you can calculate the percentiles at the end of the run. It looks like you're using this in your grpc benchmark tool. I don't think there's a reason to avoid using the AtomicHistogram it's what we use in rpc-perf.
That said, I don't think there's a reason we couldn't expose Histogram::checked_add publicly. We just went for a more minimal interface with this rewrite.
We deferred decisions around how to handle merging snapshots with non-aligned times. You're correct that this poses an issue for your particular usage pattern. I think we'd have to give more consideration for how to handle snapshots with non-aligned time ranges before making changes to the Snapshot api.
@mihirn - let me know if you have other thoughts on this.
I think making the checked_* functions public makes sense.
To answer the question of how the snapshot merge functions are intended to be used: they are currently designed with the model that one is continuously taking snapshots at some interval and the merge allows you to combine those to get the view over longer intervals.
I'm also hoping to add a sparse histogram representation into the crate which has a merge function which doesn't rely on overlapping times, so that might also work for you depending on exactly what you're doing with the snapshots.
Thank you for the answers. I somehow overlooked the existence of AtomicHistogram. That should solve my problem!
Great. The addition functions are public now in 0.8.1 (released earlier today) if you still wanted to have per-thread histograms.
Thanks for opening this! Marking as closed