Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2342663003:
QUIC - add UMA histogram to track retransmit rate for large uploads. (Closed)
DescriptionQUIC - add UMA histogram to track retransmit rate for large uploads.
R=asvitkine@chromium.org, rch@chromium.org
BUG=
Committed: https://crrev.com/9d93e0c47bf96b411e562c3afbc8b183d56d7423
Cr-Commit-Position: refs/heads/master@{#418719}
Patch Set 1 #
Total comments: 5
Patch Set 2 : add when value is recorded. #
Messages
Total messages: 14 (3 generated)
lgtm https://codereview.chromium.org/2342663003/diff/1/net/quic/chromium/quic_chro... File net/quic/chromium/quic_chromium_client_session.cc (right): https://codereview.chromium.org/2342663003/diff/1/net/quic/chromium/quic_chro... net/quic/chromium/quic_chromium_client_session.cc:376: UMA_HISTOGRAM_COUNTS_1000( I wonder if _100 might be better to avoid quantization artifacts? *shrug*
https://codereview.chromium.org/2342663003/diff/1/net/quic/chromium/quic_chro... File net/quic/chromium/quic_chromium_client_session.cc (right): https://codereview.chromium.org/2342663003/diff/1/net/quic/chromium/quic_chro... net/quic/chromium/quic_chromium_client_session.cc:376: UMA_HISTOGRAM_COUNTS_1000( On 2016/09/14 19:51:20, Ryan Hamilton wrote: > I wonder if _100 might be better to avoid quantization artifacts? *shrug* Wouldn't that make it worse? That's why I chose UMA_HISTOGRAM_COUNTS_1000 instead of UMA_HISTOGRAM_PERCENTAGE (synonymous with _100 I believe). If the expected range of values is say 1-2%, true percentage is too coarse grained, as opposed to 10-20 permille.
https://codereview.chromium.org/2342663003/diff/1/net/quic/chromium/quic_chro... File net/quic/chromium/quic_chromium_client_session.cc (right): https://codereview.chromium.org/2342663003/diff/1/net/quic/chromium/quic_chro... net/quic/chromium/quic_chromium_client_session.cc:376: UMA_HISTOGRAM_COUNTS_1000( On 2016/09/14 19:56:14, Buck wrote: > On 2016/09/14 19:51:20, Ryan Hamilton wrote: > > I wonder if _100 might be better to avoid quantization artifacts? *shrug* > > Wouldn't that make it worse? That's why I chose UMA_HISTOGRAM_COUNTS_1000 > instead of UMA_HISTOGRAM_PERCENTAGE (synonymous with _100 I believe). If the > expected range of values is say 1-2%, true percentage is too coarse grained, as > opposed to 10-20 permille. I was thinking of something different. If, for example, most streams have 100 packets then there are only 100 distinct values for the packet loss rate. If we have 1000 buckets, we'll see data like X, 0, 0, 0, 0, 0, 0, 0, 0, 0, Y, 0, 0, 0, 0, 0, 0, 0, 0, 0, Z. That will end up looking like we have big spikes at 0,10,20, but in reality is just quantization artifacts. You raise a good point that we might care about having higher resolution packet loss rates than 1%. In order to have high fidelity here, I think we need sessions with more than 100 distinct value. My guess is that the number of packets sent per connection follows some sort of exponential slow down but I could be totally wrong.
lgtm https://codereview.chromium.org/2342663003/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2342663003/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:31420: + packets_sent >= 100 are included. Nit: Mention when it's recorded.
https://codereview.chromium.org/2342663003/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2342663003/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:31420: + packets_sent >= 100 are included. On 2016/09/14 21:45:10, Alexei Svitkine (very slow) wrote: > Nit: Mention when it's recorded. Done.
On 2016/09/14 20:41:16, Ryan Hamilton wrote: > https://codereview.chromium.org/2342663003/diff/1/net/quic/chromium/quic_chro... > File net/quic/chromium/quic_chromium_client_session.cc (right): > > https://codereview.chromium.org/2342663003/diff/1/net/quic/chromium/quic_chro... > net/quic/chromium/quic_chromium_client_session.cc:376: > UMA_HISTOGRAM_COUNTS_1000( > On 2016/09/14 19:56:14, Buck wrote: > > On 2016/09/14 19:51:20, Ryan Hamilton wrote: > > > I wonder if _100 might be better to avoid quantization artifacts? *shrug* > > > > Wouldn't that make it worse? That's why I chose UMA_HISTOGRAM_COUNTS_1000 > > instead of UMA_HISTOGRAM_PERCENTAGE (synonymous with _100 I believe). If the > > expected range of values is say 1-2%, true percentage is too coarse grained, > as > > opposed to 10-20 permille. > > I was thinking of something different. If, for example, most streams have 100 > packets then there are only 100 distinct values for the packet loss rate. If we > have 1000 buckets, we'll see data like X, 0, 0, 0, 0, 0, 0, 0, 0, 0, Y, 0, 0, 0, > 0, 0, 0, 0, 0, 0, Z. That will end up looking like we have big spikes at > 0,10,20, but in reality is just quantization artifacts. > > You raise a good point that we might care about having higher resolution packet > loss rates than 1%. In order to have high fidelity here, I think we need > sessions with more than 100 distinct value. My guess is that the number of > packets sent per connection follows some sort of exponential slow down but I > could be totally wrong. I re-used some old dremel I had, and some rough numbers. Roughly 5% of connections send more than 100 packets. Since the threshold is 100, and we expect normal packet loss rates in single digit percentages, I do expect values to spike at the lower 10s (10, 20, 30, ...). OTOH, I suspect the means should behave reasonably well, and I hope the signal will be clear enough for chirp.
The CQ bit was checked by ckrasic@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from asvitkine@chromium.org, rch@chromium.org Link to the patchset: https://codereview.chromium.org/2342663003/#ps20001 (title: "add when value is recorded.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== QUIC - add UMA histogram to track retransmit rate for large uploads. R=asvitkine@chromium.org, rch@chromium.org BUG= ========== to ========== QUIC - add UMA histogram to track retransmit rate for large uploads. R=asvitkine@chromium.org, rch@chromium.org BUG= Committed: https://crrev.com/9d93e0c47bf96b411e562c3afbc8b183d56d7423 Cr-Commit-Position: refs/heads/master@{#418719} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/9d93e0c47bf96b411e562c3afbc8b183d56d7423 Cr-Commit-Position: refs/heads/master@{#418719} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
