Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2222113003:
Add UMA to net::BidirectionalStream (Closed)
DescriptionAdd UMA to net::BidirectionalStream
Add UMA logging to track the following six metrics in net::BidirectionalStream:
(1) time to reading first byte is allowed
(2) time to reading last byte
(3) time to sending first byte is allowed
(4) time to sending last byte
(5) number of bytes received
(6) number of bytes sent
BUG=635548
Committed: https://crrev.com/d58621f529528a45a3f1f4309f9bc37a1e2005b8
Cr-Commit-Position: refs/heads/master@{#411713}
Patch Set 1 #Patch Set 2 : self review #Patch Set 3 : self review #
Total comments: 10
Patch Set 4 : address comments #
Total comments: 9
Patch Set 5 : Use milliseconds and address other comments #
Total comments: 3
Messages
Total messages: 25 (12 generated)
xunjieli@chromium.org changed reviewers: + isherman@chromium.org, mef@chromium.org
mef@: PTAL * isherman@: PTAL histograms.xml. Thanks!
The CQ bit was checked by xunjieli@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by xunjieli@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2222113003/diff/40001/net/http/bidirectional_... File net/http/bidirectional_stream.cc (right): https://codereview.chromium.org/2222113003/diff/40001/net/http/bidirectional_... net/http/bidirectional_stream.cc:402: UMA_HISTOGRAM_TIMES("BidirectionalStream.HTTP2.TimeToResponseStart", Would it be appropriate to prefix these histogram names with "Net."? https://codereview.chromium.org/2222113003/diff/40001/net/http/bidirectional_... net/http/bidirectional_stream.cc:406: UMA_HISTOGRAM_COUNTS("BidirectionalStream.HTTP2.ReceivedBytes", Are the boundaries set by UMA_HISTOGRAM_COUNTS reasonable for your needs? https://codereview.chromium.org/2222113003/diff/40001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2222113003/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:3627: +<histogram name="BidirectionalStream.TimeToResponseEnd" units="microseconds"> Are you sure that the units are microseconds and not milliseconds? Ditto below. https://codereview.chromium.org/2222113003/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:3639: + response. nit: s/of response/of the response
Description was changed from ========== Add UMA to net::BidirectionalStream Add UMA logging to track the following four metrics in net::BidirectionalStream: (1) time to receiving first byte (2) time to receiving last byte (3) number of bytes received (4) number of bytes sent BUG=635548 ========== to ========== Add UMA to net::BidirectionalStream Add UMA logging to track the following six metrics in net::BidirectionalStream: (1) time to reading first byte is allowed (2) time to reading last byte (3) time to sending first byte is allowed (4) time to sending last byte (3) number of bytes received (4) number of bytes sent BUG=635548 ==========
Thanks for the review! Talked to mef@ offline, and we added two more metrics to track. PTAL. https://codereview.chromium.org/2222113003/diff/40001/net/http/bidirectional_... File net/http/bidirectional_stream.cc (right): https://codereview.chromium.org/2222113003/diff/40001/net/http/bidirectional_... net/http/bidirectional_stream.cc:402: UMA_HISTOGRAM_TIMES("BidirectionalStream.HTTP2.TimeToResponseStart", On 2016/08/08 22:26:37, Ilya Sherman wrote: > Would it be appropriate to prefix these histogram names with "Net."? Done. https://codereview.chromium.org/2222113003/diff/40001/net/http/bidirectional_... net/http/bidirectional_stream.cc:406: UMA_HISTOGRAM_COUNTS("BidirectionalStream.HTTP2.ReceivedBytes", On 2016/08/08 22:26:37, Ilya Sherman wrote: > Are the boundaries set by UMA_HISTOGRAM_COUNTS reasonable for your needs? Yes, elsewhere in net/, the UMA for bytes is logged using UMA_HISTOGRAM_COUNTS (e.g. Net.SpdySendBytes and SpdyReceivedBytes), so I think the boundaries here should be fine. Thanks. https://codereview.chromium.org/2222113003/diff/40001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2222113003/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:3627: +<histogram name="BidirectionalStream.TimeToResponseEnd" units="microseconds"> On 2016/08/08 22:26:37, Ilya Sherman wrote: > Are you sure that the units are microseconds and not milliseconds? Ditto below. Yes, these are microseconds, because base::TimeDelta is in microseconds. See: https://cs.chromium.org/chromium/src/base/time/time.h?rcl=0&l=13
Looks pretty goood. Please also update description with proper ordinals. https://codereview.chromium.org/2222113003/diff/60001/net/http/bidirectional_... File net/http/bidirectional_stream.cc (right): https://codereview.chromium.org/2222113003/diff/60001/net/http/bidirectional_... net/http/bidirectional_stream.cc:153: read_end_time_ = tick_clock_->NowTicks(); Can we use TimeTicks::Now() here and elsewhere? They use monotonic clock, which seems like a right thing to use in this case. https://codereview.chromium.org/2222113003/diff/60001/net/http/bidirectional_... net/http/bidirectional_stream.cc:407: UMA_HISTOGRAM_TIMES("Net.BidirectionalStream.HTTP2.TimeToReadStart", Should these names match those in histograms.xml? https://codereview.chromium.org/2222113003/diff/60001/net/http/bidirectional_... File net/http/bidirectional_stream.h (right): https://codereview.chromium.org/2222113003/diff/60001/net/http/bidirectional_... net/http/bidirectional_stream.h:24: } // namespace base Add class TimeTicks?
https://codereview.chromium.org/2222113003/diff/40001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2222113003/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:3627: +<histogram name="BidirectionalStream.TimeToResponseEnd" units="microseconds"> On 2016/08/09 20:31:28, xunjieli wrote: > On 2016/08/08 22:26:37, Ilya Sherman wrote: > > Are you sure that the units are microseconds and not milliseconds? Ditto > below. > > Yes, these are microseconds, because base::TimeDelta is in microseconds. > See: https://cs.chromium.org/chromium/src/base/time/time.h?rcl=0&l=13 I agree that base::TimeDelta internally uses microseconds as the most granular representation. However, UMA_HISTOGRAM_TIMES calls UMA_HISTOGRAM_CUSTOM_TIMES, which calls FactoryTimeGet(), which records the time value in milliseconds, rather than microseconds. Apologies if I'm still misunderstanding something, perhaps not noticing a slight variation in the macro you're using.
Description was changed from ========== Add UMA to net::BidirectionalStream Add UMA logging to track the following six metrics in net::BidirectionalStream: (1) time to reading first byte is allowed (2) time to reading last byte (3) time to sending first byte is allowed (4) time to sending last byte (3) number of bytes received (4) number of bytes sent BUG=635548 ========== to ========== Add UMA to net::BidirectionalStream Add UMA logging to track the following six metrics in net::BidirectionalStream: (1) time to reading first byte is allowed (2) time to reading last byte (3) time to sending first byte is allowed (4) time to sending last byte (5) number of bytes received (6) number of bytes sent BUG=635548 ==========
Thanks for the review! For some reason, presubmit still gives me the following warnings. Any idea on how I can address that? I thought the suffix part should take care of it, but it looks like it doesn't. PTAL. Thanks! ** Presubmit Warnings ** Some UMA_HISTOGRAM lines have been modified and the associated histogram name has no match in either tools/metrics/histograms/histograms.xml or the modifications of it: [net/http/bidirectional_stream.cc:394] Net.BidirectionalStream.HTTP2.TimeToReadStart \ [net/http/bidirectional_stream.cc:396] Net.BidirectionalStream.HTTP2.TimeToReadEnd \ [net/http/bidirectional_stream.cc:398] Net.BidirectionalStream.HTTP2.TimeToSendStart \ [net/http/bidirectional_stream.cc:400] Net.BidirectionalStream.HTTP2.TimeToSendEnd \ [net/http/bidirectional_stream.cc:402] Net.BidirectionalStream.HTTP2.ReceivedBytes \ [net/http/bidirectional_stream.cc:404] Net.BidirectionalStream.HTTP2.SentBytes \ [net/http/bidirectional_stream.cc:407] Net.BidirectionalStream.QUIC.TimeToReadStart \ [net/http/bidirectional_stream.cc:409] Net.BidirectionalStream.QUIC.TimeToReadEnd \ [net/http/bidirectional_stream.cc:411] Net.BidirectionalStream.QUIC.TimeToSendStart \ [net/http/bidirectional_stream.cc:413] Net.BidirectionalStream.QUIC.TimeToSendEnd \ [net/http/bidirectional_stream.cc:415] Net.BidirectionalStream.QUIC.ReceivedBytes \ [net/http/bidirectional_stream.cc:417] Net.BidirectionalStream.QUIC.SentBytes https://codereview.chromium.org/2222113003/diff/40001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2222113003/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:3627: +<histogram name="BidirectionalStream.TimeToResponseEnd" units="microseconds"> On 2016/08/10 05:49:34, Ilya Sherman wrote: > On 2016/08/09 20:31:28, xunjieli wrote: > > On 2016/08/08 22:26:37, Ilya Sherman wrote: > > > Are you sure that the units are microseconds and not milliseconds? Ditto > > below. > > > > Yes, these are microseconds, because base::TimeDelta is in microseconds. > > See: https://cs.chromium.org/chromium/src/base/time/time.h?rcl=0&l=13 > > I agree that base::TimeDelta internally uses microseconds as the most granular > representation. However, UMA_HISTOGRAM_TIMES calls UMA_HISTOGRAM_CUSTOM_TIMES, > which calls FactoryTimeGet(), which records the time value in milliseconds, > rather than microseconds. Apologies if I'm still misunderstanding something, > perhaps not noticing a slight variation in the macro you're using. Done. Ah, I see. Thanks for explaining that! It is indeed milliseconds as you pointed out. https://codereview.chromium.org/2222113003/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:3639: + response. On 2016/08/08 22:26:37, Ilya Sherman wrote: > nit: s/of response/of the response Done. https://codereview.chromium.org/2222113003/diff/60001/net/http/bidirectional_... File net/http/bidirectional_stream.cc (right): https://codereview.chromium.org/2222113003/diff/60001/net/http/bidirectional_... net/http/bidirectional_stream.cc:153: read_end_time_ = tick_clock_->NowTicks(); On 2016/08/10 01:05:20, mef wrote: > Can we use TimeTicks::Now() here and elsewhere? They use monotonic clock, which > seems like a right thing to use in this case. Done. https://codereview.chromium.org/2222113003/diff/60001/net/http/bidirectional_... net/http/bidirectional_stream.cc:407: UMA_HISTOGRAM_TIMES("Net.BidirectionalStream.HTTP2.TimeToReadStart", On 2016/08/10 01:05:20, mef wrote: > Should these names match those in histograms.xml? I don't understand. They do match right? I added the HTTP2 and QUIC2 parts as a suffix (which is declared in histograms.xml as well). https://codereview.chromium.org/2222113003/diff/60001/net/http/bidirectional_... File net/http/bidirectional_stream.h (right): https://codereview.chromium.org/2222113003/diff/60001/net/http/bidirectional_... net/http/bidirectional_stream.h:24: } // namespace base On 2016/08/10 01:05:20, mef wrote: > Add class TimeTicks? Done. I included the header file instead since that isn't in a unique_ptr.
https://codereview.chromium.org/2222113003/diff/60001/net/http/bidirectional_... File net/http/bidirectional_stream.cc (right): https://codereview.chromium.org/2222113003/diff/60001/net/http/bidirectional_... net/http/bidirectional_stream.cc:407: UMA_HISTOGRAM_TIMES("Net.BidirectionalStream.HTTP2.TimeToReadStart", On 2016/08/11 12:50:40, xunjieli wrote: > On 2016/08/10 01:05:20, mef wrote: > > Should these names match those in histograms.xml? > > I don't understand. They do match right? I added the HTTP2 and QUIC2 parts as a > suffix (which is declared in histograms.xml as well). I would've thought that suffix goes at the end of the name, not in the middle, but I'm definitely not an expert in histograms. https://codereview.chromium.org/2222113003/diff/80001/net/http/bidirectional_... File net/http/bidirectional_stream.h (right): https://codereview.chromium.org/2222113003/diff/80001/net/http/bidirectional_... net/http/bidirectional_stream.h:249: base::TimeTicks start_time_; Would it make sense to replace individual timeticks with net::LoadTimingInfo here? Some of the fields will be unused, but it could make it easier to use for Metrics API implementation.
Metrics lgtm, thanks. https://codereview.chromium.org/2222113003/diff/60001/net/http/bidirectional_... File net/http/bidirectional_stream.cc (right): https://codereview.chromium.org/2222113003/diff/60001/net/http/bidirectional_... net/http/bidirectional_stream.cc:407: UMA_HISTOGRAM_TIMES("Net.BidirectionalStream.HTTP2.TimeToReadStart", On 2016/08/11 15:42:19, mef wrote: > On 2016/08/11 12:50:40, xunjieli wrote: > > On 2016/08/10 01:05:20, mef wrote: > > > Should these names match those in histograms.xml? > > > > I don't understand. They do match right? I added the HTTP2 and QUIC2 parts as > a > > suffix (which is declared in histograms.xml as well). > > I would've thought that suffix goes at the end of the name, not in the middle, > but I'm definitely not an expert in histograms. ordering=prefix should indeed put the "suffix" in the middle, I think exactly as desired. I'm not familiar with the presubmit script that's failing -- it's possible that the script is not familiar with ordering=prefix.
Thanks for the review! https://codereview.chromium.org/2222113003/diff/60001/net/http/bidirectional_... File net/http/bidirectional_stream.cc (right): https://codereview.chromium.org/2222113003/diff/60001/net/http/bidirectional_... net/http/bidirectional_stream.cc:407: UMA_HISTOGRAM_TIMES("Net.BidirectionalStream.HTTP2.TimeToReadStart", On 2016/08/11 18:48:46, Ilya Sherman (Away Aug 11-12) wrote: > On 2016/08/11 15:42:19, mef wrote: > > On 2016/08/11 12:50:40, xunjieli wrote: > > > On 2016/08/10 01:05:20, mef wrote: > > > > Should these names match those in histograms.xml? > > > > > > I don't understand. They do match right? I added the HTTP2 and QUIC2 parts > as > > a > > > suffix (which is declared in histograms.xml as well). > > > > I would've thought that suffix goes at the end of the name, not in the middle, > > but I'm definitely not an expert in histograms. > > ordering=prefix should indeed put the "suffix" in the middle, I think exactly as > desired. I'm not familiar with the presubmit script that's failing -- it's > possible that the script is not familiar with ordering=prefix. I talked to csharrison@ who added a similar histogram, and he said the presubmit script doesn't understand prefix and suffix. I think we are good here. Thanks for the confirmation! https://codereview.chromium.org/2222113003/diff/80001/net/http/bidirectional_... File net/http/bidirectional_stream.h (right): https://codereview.chromium.org/2222113003/diff/80001/net/http/bidirectional_... net/http/bidirectional_stream.h:249: base::TimeTicks start_time_; On 2016/08/11 15:42:19, mef wrote: > Would it make sense to replace individual timeticks with net::LoadTimingInfo > here? Some of the fields will be unused, but it could make it easier to use for > Metrics API implementation. That's a great idea! How about doing this as a follow-up? I'd like to add support for LoadTimingInfo in QUIC as well. I can refactor this once that is ready. What do you think? LoadTimingInfo doesn't have a counter part for |read_end_time_|, so that's something we need to investigate as well for the metrics API.
lgtm https://codereview.chromium.org/2222113003/diff/80001/net/http/bidirectional_... File net/http/bidirectional_stream.h (right): https://codereview.chromium.org/2222113003/diff/80001/net/http/bidirectional_... net/http/bidirectional_stream.h:249: base::TimeTicks start_time_; On 2016/08/11 19:37:09, xunjieli wrote: > On 2016/08/11 15:42:19, mef wrote: > > Would it make sense to replace individual timeticks with net::LoadTimingInfo > > here? Some of the fields will be unused, but it could make it easier to use > for > > Metrics API implementation. > > That's a great idea! How about doing this as a follow-up? I'd like to add > support for LoadTimingInfo in QUIC as well. I can refactor this once that is > ready. What do you think? > > LoadTimingInfo doesn't have a counter part for |read_end_time_|, so that's > something we need to investigate as well for the metrics API. sgtm. Interesting observation about missing read_end_time_, I didn't realize that.
The CQ bit was checked by xunjieli@chromium.org
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.
Description was changed from ========== Add UMA to net::BidirectionalStream Add UMA logging to track the following six metrics in net::BidirectionalStream: (1) time to reading first byte is allowed (2) time to reading last byte (3) time to sending first byte is allowed (4) time to sending last byte (5) number of bytes received (6) number of bytes sent BUG=635548 ========== to ========== Add UMA to net::BidirectionalStream Add UMA logging to track the following six metrics in net::BidirectionalStream: (1) time to reading first byte is allowed (2) time to reading last byte (3) time to sending first byte is allowed (4) time to sending last byte (5) number of bytes received (6) number of bytes sent BUG=635548 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Add UMA to net::BidirectionalStream Add UMA logging to track the following six metrics in net::BidirectionalStream: (1) time to reading first byte is allowed (2) time to reading last byte (3) time to sending first byte is allowed (4) time to sending last byte (5) number of bytes received (6) number of bytes sent BUG=635548 ========== to ========== Add UMA to net::BidirectionalStream Add UMA logging to track the following six metrics in net::BidirectionalStream: (1) time to reading first byte is allowed (2) time to reading last byte (3) time to sending first byte is allowed (4) time to sending last byte (5) number of bytes received (6) number of bytes sent BUG=635548 Committed: https://crrev.com/d58621f529528a45a3f1f4309f9bc37a1e2005b8 Cr-Commit-Position: refs/heads/master@{#411713} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/d58621f529528a45a3f1f4309f9bc37a1e2005b8 Cr-Commit-Position: refs/heads/master@{#411713} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
