Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1181743005:
Report video and network stats averaged over 1s, and create corresponding UMA metrics. (Closed)
DescriptionReport video and network stats averaged over 1s, and create corresponding UMA metrics.
BUG=502061
Committed: https://crrev.com/b877083e221c58a67a9a00e2024050f069948c6d
Cr-Commit-Position: refs/heads/master@{#339136}
Patch Set 1 #Patch Set 2 : Update stats for software-renderer as well. #
Total comments: 6
Patch Set 3 : Create more fine-grained stats. Also capture fps including empty frames. #Patch Set 4 : Remove unnecessary RunningAverage stats. #
Total comments: 27
Patch Set 5 : Clarify metric names. Better document UMA min-max and boundary settings. #
Total comments: 21
Patch Set 6 : Create separate timers for updating stats for display and for UMA. #Patch Set 7 : Set time-window for display stats rate-counters to 3. #
Total comments: 8
Patch Set 8 : Declare time-window constants as static. #
Total comments: 34
Patch Set 9 : Collapse UMA and display stats into 1 set of metrics. Remove uploading of latency metrics to UMA because with the current implementation they will be inaccurate. #
Total comments: 6
Patch Set 10 : Add Chromoting UMA metrics to histograms.xml. Reduce # of buckets for bandwidth to 100. #
Total comments: 10
Patch Set 11 : Note frequency at which UMA stats are being updated/logged. #
Total comments: 56
Patch Set 12 : Fix bandwidth and frame-rate being incorrectly updated in software video-renderer. Add sergeyu@ to … #
Total comments: 3
Patch Set 13 : Set bandwidth histogram minimum to 0. #
Messages
Total messages: 54 (15 generated)
Patchset #2 (id:20001) has been deleted
anandc@chromium.org changed reviewers: + sergeyu@chromium.org
Sergey, PTAL. If the metric names are OK, I'll update histograms.xml (https://code.google.com/p/chromium/codesearch#chromium/src/tools/metrics/hist...) to include the newly added metrics in a separate CL/
The UMA reporting calls are being added in client-side code, which runs in the - how will the resulting stats get routed through Chrome to be reported? On 18 June 2015 at 16:14, <anandc@chromium.org> wrote: > Reviewers: Sergey Ulanov, > > Message: > Sergey, PTAL. > > If the metric names are OK, I'll update histograms.xml > ( > https://code.google.com/p/chromium/codesearch#chromium/src/tools/metrics/hist... > ) > to include the newly added metrics in a separate CL/ > > Description: > Improve accuracy of FPS and video-bandwidth stats. > Add UMA Stats for FPS, bandwidth, capture, encode and round-trip latencies. > > BUG=502061 > > Please review this at https://codereview.chromium.org/1181743005/ > > Base URL: https://chromium.googlesource.com/chromium/src.git@master > > Affected files (+36, -6 lines): > M remoting/client/chromoting_stats.cc > M remoting/client/plugin/chromoting_instance.cc > M remoting/client/plugin/pepper_video_renderer_3d.cc > M remoting/client/software_video_renderer.cc > > > Index: remoting/client/chromoting_stats.cc > diff --git a/remoting/client/chromoting_stats.cc > b/remoting/client/chromoting_stats.cc > index > 71b7199ff5adca1f9222636bb5cc5fe79efedea8..4e0ba49a050a5ce82634da988121e5694e316223 > 100644 > --- a/remoting/client/chromoting_stats.cc > +++ b/remoting/client/chromoting_stats.cc > @@ -7,7 +7,9 @@ > namespace { > > // The default window of bandwidth and frame rate in seconds. > -const int kTimeWindow = 3; > +// Set to 60s, so that we average at 60s intervals, same as the rate at > which we > +// log stats. > +const int kTimeWindow = 60; > > // We take the last 10 latency numbers and report the average. > const int kLatencyWindow = 10; > Index: remoting/client/plugin/chromoting_instance.cc > diff --git a/remoting/client/plugin/chromoting_instance.cc > b/remoting/client/plugin/chromoting_instance.cc > index > c32d31ded27bcd4d333f047d9af82c1536d5442a..df2f4f0a4022446ac4c6ecadb9cc80fe28a3b378 > 100644 > --- a/remoting/client/plugin/chromoting_instance.cc > +++ b/remoting/client/plugin/chromoting_instance.cc > @@ -19,6 +19,7 @@ > #include "base/json/json_writer.h" > #include "base/lazy_instance.h" > #include "base/logging.h" > +#include "base/metrics/histogram_macros.h" > #include "base/strings/string_split.h" > #include "base/strings/stringprintf.h" > #include "base/synchronization/lock.h" > @@ -1064,6 +1065,11 @@ void ChromotingInstance::SendPerfStats() { > data->SetDouble("renderLatency", stats->video_paint_ms()->Average()); > data->SetDouble("roundtripLatency", stats->round_trip_ms()->Average()); > PostLegacyJsonMessage("onPerfStats", data.Pass()); > + // Also update UMA histograms for video frame-rate and bandwidth. > + UMA_HISTOGRAM_COUNTS("Chromoting.Video.FrameRate", > + stats->video_frame_rate()->Rate()); > + UMA_HISTOGRAM_COUNTS("Chromoting.Video.Bandwidth", > + stats->video_bandwidth()->Rate()); > } > > // static > Index: remoting/client/plugin/pepper_video_renderer_3d.cc > diff --git a/remoting/client/plugin/pepper_video_renderer_3d.cc > b/remoting/client/plugin/pepper_video_renderer_3d.cc > index > 711c2104b702cf083f78a4b4b337286fb20eb91e..3207c4d85c117f7d3b606c37f1a63c0448b332a4 > 100644 > --- a/remoting/client/plugin/pepper_video_renderer_3d.cc > +++ b/remoting/client/plugin/pepper_video_renderer_3d.cc > @@ -7,6 +7,7 @@ > #include <math.h> > > #include "base/callback_helpers.h" > +#include "base/metrics/histogram_macros.h" > #include "base/stl_util.h" > #include "ppapi/c/pp_codecs.h" > #include "ppapi/c/ppb_opengles2.h" > @@ -194,13 +195,21 @@ void > PepperVideoRenderer3D::ProcessVideoPacket(scoped_ptr<VideoPacket> packet, > if (!packet->data().size()) > return; > > - // Update statistics. > + // Update statistics and UMA histograms. > stats_.video_frame_rate()->Record(1); > stats_.video_bandwidth()->Record(packet->data().size()); > - if (packet->has_capture_time_ms()) > + if (packet->has_capture_time_ms()) { > stats_.video_capture_ms()->Record(packet->capture_time_ms()); > - if (packet->has_encode_time_ms()) > + const base::TimeDelta capture_time = > + base::TimeDelta::FromInternalValue(packet->capture_time_ms()); > + UMA_HISTOGRAM_TIMES("Chromoting.Video.CaptureLatency", capture_time); > + } > + if (packet->has_encode_time_ms()) { > stats_.video_encode_ms()->Record(packet->encode_time_ms()); > + const base::TimeDelta encode_time = > + base::TimeDelta::FromInternalValue(packet->encode_time_ms()); > + UMA_HISTOGRAM_TIMES("Chromoting.Video.EncodeLatency", encode_time); > + } > if (packet->has_latest_event_timestamp() && > packet->latest_event_timestamp() > latest_input_event_timestamp_) { > latest_input_event_timestamp_ = packet->latest_event_timestamp(); > @@ -208,6 +217,8 @@ void > PepperVideoRenderer3D::ProcessVideoPacket(scoped_ptr<VideoPacket> packet, > base::Time::Now() - > base::Time::FromInternalValue(packet->latest_event_timestamp()); > stats_.round_trip_ms()->Record(round_trip_latency.InMilliseconds()); > + UMA_HISTOGRAM_TIMES("Chromoting.Video.RoundTripLatency", > + round_trip_latency); > } > > bool resolution_changed = false; > Index: remoting/client/software_video_renderer.cc > diff --git a/remoting/client/software_video_renderer.cc > b/remoting/client/software_video_renderer.cc > index > 51782e05f476f4942a30408a7e52d0214cc0a80f..8c8855ddddec660ecc837068bc2e01ce1b904ed7 > 100644 > --- a/remoting/client/software_video_renderer.cc > +++ b/remoting/client/software_video_renderer.cc > @@ -11,6 +11,7 @@ > #include "base/callback_helpers.h" > #include "base/location.h" > #include "base/logging.h" > +#include "base/metrics/histogram_macros.h" > #include "base/single_thread_task_runner.h" > #include "remoting/base/util.h" > #include "remoting/client/frame_consumer.h" > @@ -362,10 +363,18 @@ void > SoftwareVideoRenderer::ProcessVideoPacket(scoped_ptr<VideoPacket> packet, > > // Record other statistics received from host. > stats_.video_bandwidth()->Record(packet->data().size()); > - if (packet->has_capture_time_ms()) > + if (packet->has_capture_time_ms()) { > stats_.video_capture_ms()->Record(packet->capture_time_ms()); > - if (packet->has_encode_time_ms()) > + const base::TimeDelta capture_time = > + base::TimeDelta::FromInternalValue(packet->capture_time_ms()); > + UMA_HISTOGRAM_TIMES("Chromoting.Video.CaptureLatency", capture_time); > + } > + if (packet->has_encode_time_ms()) { > stats_.video_encode_ms()->Record(packet->encode_time_ms()); > + const base::TimeDelta encode_time = > + base::TimeDelta::FromInternalValue(packet->encode_time_ms()); > + UMA_HISTOGRAM_TIMES("Chromoting.Video.EncodeLatency", encode_time); > + } > if (packet->has_latest_event_timestamp() && > packet->latest_event_timestamp() > latest_event_timestamp_) { > latest_event_timestamp_ = packet->latest_event_timestamp(); > @@ -373,6 +382,8 @@ void > SoftwareVideoRenderer::ProcessVideoPacket(scoped_ptr<VideoPacket> packet, > base::Time::Now() - > base::Time::FromInternalValue(packet->latest_event_timestamp()); > stats_.round_trip_ms()->Record(round_trip_latency.InMilliseconds()); > + UMA_HISTOGRAM_TIMES("Chromoting.Video.RoundTripLatency", > + round_trip_latency); > } > > // Measure the latency between the last packet being received and > presented. > > > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2015/06/19 16:25:48, Wez wrote: > The UMA reporting calls are being added in client-side code, which runs in > the - how will the resulting stats get routed through Chrome to be > reported? > > On 18 June 2015 at 16:14, <mailto:anandc@chromium.org> wrote: > > > Reviewers: Sergey Ulanov, > > > > Message: > > Sergey, PTAL. > > > > If the metric names are OK, I'll update histograms.xml > > ( > > > https://code.google.com/p/chromium/codesearch#chromium/src/tools/metrics/hist... > > ) > > to include the newly added metrics in a separate CL/ > > > > Description: > > Improve accuracy of FPS and video-bandwidth stats. > > Add UMA Stats for FPS, bandwidth, capture, encode and round-trip latencies. > > > > BUG=502061 > > > > Please review this at https://codereview.chromium.org/1181743005/ > > > > Base URL: https://chromium.googlesource.com/chromium/src.git@master > > > > Affected files (+36, -6 lines): > > M remoting/client/chromoting_stats.cc > > M remoting/client/plugin/chromoting_instance.cc > > M remoting/client/plugin/pepper_video_renderer_3d.cc > > M remoting/client/software_video_renderer.cc > > > > > > Index: remoting/client/chromoting_stats.cc > > diff --git a/remoting/client/chromoting_stats.cc > > b/remoting/client/chromoting_stats.cc > > index > > > 71b7199ff5adca1f9222636bb5cc5fe79efedea8..4e0ba49a050a5ce82634da988121e5694e316223 > > 100644 > > --- a/remoting/client/chromoting_stats.cc > > +++ b/remoting/client/chromoting_stats.cc > > @@ -7,7 +7,9 @@ > > namespace { > > > > // The default window of bandwidth and frame rate in seconds. > > -const int kTimeWindow = 3; > > +// Set to 60s, so that we average at 60s intervals, same as the rate at > > which we > > +// log stats. > > +const int kTimeWindow = 60; > > > > // We take the last 10 latency numbers and report the average. > > const int kLatencyWindow = 10; > > Index: remoting/client/plugin/chromoting_instance.cc > > diff --git a/remoting/client/plugin/chromoting_instance.cc > > b/remoting/client/plugin/chromoting_instance.cc > > index > > > c32d31ded27bcd4d333f047d9af82c1536d5442a..df2f4f0a4022446ac4c6ecadb9cc80fe28a3b378 > > 100644 > > --- a/remoting/client/plugin/chromoting_instance.cc > > +++ b/remoting/client/plugin/chromoting_instance.cc > > @@ -19,6 +19,7 @@ > > #include "base/json/json_writer.h" > > #include "base/lazy_instance.h" > > #include "base/logging.h" > > +#include "base/metrics/histogram_macros.h" > > #include "base/strings/string_split.h" > > #include "base/strings/stringprintf.h" > > #include "base/synchronization/lock.h" > > @@ -1064,6 +1065,11 @@ void ChromotingInstance::SendPerfStats() { > > data->SetDouble("renderLatency", stats->video_paint_ms()->Average()); > > data->SetDouble("roundtripLatency", stats->round_trip_ms()->Average()); > > PostLegacyJsonMessage("onPerfStats", data.Pass()); > > + // Also update UMA histograms for video frame-rate and bandwidth. > > + UMA_HISTOGRAM_COUNTS("Chromoting.Video.FrameRate", > > + stats->video_frame_rate()->Rate()); > > + UMA_HISTOGRAM_COUNTS("Chromoting.Video.Bandwidth", > > + stats->video_bandwidth()->Rate()); > > } > > > > // static > > Index: remoting/client/plugin/pepper_video_renderer_3d.cc > > diff --git a/remoting/client/plugin/pepper_video_renderer_3d.cc > > b/remoting/client/plugin/pepper_video_renderer_3d.cc > > index > > > 711c2104b702cf083f78a4b4b337286fb20eb91e..3207c4d85c117f7d3b606c37f1a63c0448b332a4 > > 100644 > > --- a/remoting/client/plugin/pepper_video_renderer_3d.cc > > +++ b/remoting/client/plugin/pepper_video_renderer_3d.cc > > @@ -7,6 +7,7 @@ > > #include <math.h> > > > > #include "base/callback_helpers.h" > > +#include "base/metrics/histogram_macros.h" > > #include "base/stl_util.h" > > #include "ppapi/c/pp_codecs.h" > > #include "ppapi/c/ppb_opengles2.h" > > @@ -194,13 +195,21 @@ void > > PepperVideoRenderer3D::ProcessVideoPacket(scoped_ptr<VideoPacket> packet, > > if (!packet->data().size()) > > return; > > > > - // Update statistics. > > + // Update statistics and UMA histograms. > > stats_.video_frame_rate()->Record(1); > > stats_.video_bandwidth()->Record(packet->data().size()); > > - if (packet->has_capture_time_ms()) > > + if (packet->has_capture_time_ms()) { > > stats_.video_capture_ms()->Record(packet->capture_time_ms()); > > - if (packet->has_encode_time_ms()) > > + const base::TimeDelta capture_time = > > + base::TimeDelta::FromInternalValue(packet->capture_time_ms()); > > + UMA_HISTOGRAM_TIMES("Chromoting.Video.CaptureLatency", capture_time); > > + } > > + if (packet->has_encode_time_ms()) { > > stats_.video_encode_ms()->Record(packet->encode_time_ms()); > > + const base::TimeDelta encode_time = > > + base::TimeDelta::FromInternalValue(packet->encode_time_ms()); > > + UMA_HISTOGRAM_TIMES("Chromoting.Video.EncodeLatency", encode_time); > > + } > > if (packet->has_latest_event_timestamp() && > > packet->latest_event_timestamp() > latest_input_event_timestamp_) { > > latest_input_event_timestamp_ = packet->latest_event_timestamp(); > > @@ -208,6 +217,8 @@ void > > PepperVideoRenderer3D::ProcessVideoPacket(scoped_ptr<VideoPacket> packet, > > base::Time::Now() - > > base::Time::FromInternalValue(packet->latest_event_timestamp()); > > stats_.round_trip_ms()->Record(round_trip_latency.InMilliseconds()); > > + UMA_HISTOGRAM_TIMES("Chromoting.Video.RoundTripLatency", > > + round_trip_latency); > > } > > > > bool resolution_changed = false; > > Index: remoting/client/software_video_renderer.cc > > diff --git a/remoting/client/software_video_renderer.cc > > b/remoting/client/software_video_renderer.cc > > index > > > 51782e05f476f4942a30408a7e52d0214cc0a80f..8c8855ddddec660ecc837068bc2e01ce1b904ed7 > > 100644 > > --- a/remoting/client/software_video_renderer.cc > > +++ b/remoting/client/software_video_renderer.cc > > @@ -11,6 +11,7 @@ > > #include "base/callback_helpers.h" > > #include "base/location.h" > > #include "base/logging.h" > > +#include "base/metrics/histogram_macros.h" > > #include "base/single_thread_task_runner.h" > > #include "remoting/base/util.h" > > #include "remoting/client/frame_consumer.h" > > @@ -362,10 +363,18 @@ void > > SoftwareVideoRenderer::ProcessVideoPacket(scoped_ptr<VideoPacket> packet, > > > > // Record other statistics received from host. > > stats_.video_bandwidth()->Record(packet->data().size()); > > - if (packet->has_capture_time_ms()) > > + if (packet->has_capture_time_ms()) { > > stats_.video_capture_ms()->Record(packet->capture_time_ms()); > > - if (packet->has_encode_time_ms()) > > + const base::TimeDelta capture_time = > > + base::TimeDelta::FromInternalValue(packet->capture_time_ms()); > > + UMA_HISTOGRAM_TIMES("Chromoting.Video.CaptureLatency", capture_time); > > + } > > + if (packet->has_encode_time_ms()) { > > stats_.video_encode_ms()->Record(packet->encode_time_ms()); > > + const base::TimeDelta encode_time = > > + base::TimeDelta::FromInternalValue(packet->encode_time_ms()); > > + UMA_HISTOGRAM_TIMES("Chromoting.Video.EncodeLatency", encode_time); > > + } > > if (packet->has_latest_event_timestamp() && > > packet->latest_event_timestamp() > latest_event_timestamp_) { > > latest_event_timestamp_ = packet->latest_event_timestamp(); > > @@ -373,6 +382,8 @@ void > > SoftwareVideoRenderer::ProcessVideoPacket(scoped_ptr<VideoPacket> packet, > > base::Time::Now() - > > base::Time::FromInternalValue(packet->latest_event_timestamp()); > > stats_.round_trip_ms()->Record(round_trip_latency.InMilliseconds()); > > + UMA_HISTOGRAM_TIMES("Chromoting.Video.RoundTripLatency", > > + round_trip_latency); > > } > > > > // Measure the latency between the last packet being received and > > presented. > > > > > > > > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org. Thanks for the comment. I'll look at moving the UMA gathering work to the web-app, and modify this CL to just involve making the frame-rate and bandwidth metrics more accurate for a 60s window.
I'm not saying that moving the UMA stats gathering to the web-app is required; IIRC there is a (private) PPAPI API for that (see https://code.google.com/p/chromium/codesearch#chromium/src/ppapi/api/private/...) - can/will the UMA macros hook up to that, for example? On 19 June 2015 at 10:33, <anandc@chromium.org> wrote: > On 2015/06/19 16:25:48, Wez wrote: > >> The UMA reporting calls are being added in client-side code, which runs in >> the - how will the resulting stats get routed through Chrome to be >> reported? >> > > On 18 June 2015 at 16:14, <mailto:anandc@chromium.org> wrote: >> > > > Reviewers: Sergey Ulanov, >> > >> > Message: >> > Sergey, PTAL. >> > >> > If the metric names are OK, I'll update histograms.xml >> > ( >> > >> > > > https://code.google.com/p/chromium/codesearch#chromium/src/tools/metrics/hist... > >> > ) >> > to include the newly added metrics in a separate CL/ >> > >> > Description: >> > Improve accuracy of FPS and video-bandwidth stats. >> > Add UMA Stats for FPS, bandwidth, capture, encode and round-trip >> latencies. >> > >> > BUG=502061 >> > >> > Please review this at https://codereview.chromium.org/1181743005/ >> > >> > Base URL: https://chromium.googlesource.com/chromium/src.git@master >> > >> > Affected files (+36, -6 lines): >> > M remoting/client/chromoting_stats.cc >> > M remoting/client/plugin/chromoting_instance.cc >> > M remoting/client/plugin/pepper_video_renderer_3d.cc >> > M remoting/client/software_video_renderer.cc >> > >> > >> > Index: remoting/client/chromoting_stats.cc >> > diff --git a/remoting/client/chromoting_stats.cc >> > b/remoting/client/chromoting_stats.cc >> > index >> > >> > > > 71b7199ff5adca1f9222636bb5cc5fe79efedea8..4e0ba49a050a5ce82634da988121e5694e316223 > >> > 100644 >> > --- a/remoting/client/chromoting_stats.cc >> > +++ b/remoting/client/chromoting_stats.cc >> > @@ -7,7 +7,9 @@ >> > namespace { >> > >> > // The default window of bandwidth and frame rate in seconds. >> > -const int kTimeWindow = 3; >> > +// Set to 60s, so that we average at 60s intervals, same as the rate at >> > which we >> > +// log stats. >> > +const int kTimeWindow = 60; >> > >> > // We take the last 10 latency numbers and report the average. >> > const int kLatencyWindow = 10; >> > Index: remoting/client/plugin/chromoting_instance.cc >> > diff --git a/remoting/client/plugin/chromoting_instance.cc >> > b/remoting/client/plugin/chromoting_instance.cc >> > index >> > >> > > > c32d31ded27bcd4d333f047d9af82c1536d5442a..df2f4f0a4022446ac4c6ecadb9cc80fe28a3b378 > >> > 100644 >> > --- a/remoting/client/plugin/chromoting_instance.cc >> > +++ b/remoting/client/plugin/chromoting_instance.cc >> > @@ -19,6 +19,7 @@ >> > #include "base/json/json_writer.h" >> > #include "base/lazy_instance.h" >> > #include "base/logging.h" >> > +#include "base/metrics/histogram_macros.h" >> > #include "base/strings/string_split.h" >> > #include "base/strings/stringprintf.h" >> > #include "base/synchronization/lock.h" >> > @@ -1064,6 +1065,11 @@ void ChromotingInstance::SendPerfStats() { >> > data->SetDouble("renderLatency", stats->video_paint_ms()->Average()); >> > data->SetDouble("roundtripLatency", >> stats->round_trip_ms()->Average()); >> > PostLegacyJsonMessage("onPerfStats", data.Pass()); >> > + // Also update UMA histograms for video frame-rate and bandwidth. >> > + UMA_HISTOGRAM_COUNTS("Chromoting.Video.FrameRate", >> > + stats->video_frame_rate()->Rate()); >> > + UMA_HISTOGRAM_COUNTS("Chromoting.Video.Bandwidth", >> > + stats->video_bandwidth()->Rate()); >> > } >> > >> > // static >> > Index: remoting/client/plugin/pepper_video_renderer_3d.cc >> > diff --git a/remoting/client/plugin/pepper_video_renderer_3d.cc >> > b/remoting/client/plugin/pepper_video_renderer_3d.cc >> > index >> > >> > > > 711c2104b702cf083f78a4b4b337286fb20eb91e..3207c4d85c117f7d3b606c37f1a63c0448b332a4 > >> > 100644 >> > --- a/remoting/client/plugin/pepper_video_renderer_3d.cc >> > +++ b/remoting/client/plugin/pepper_video_renderer_3d.cc >> > @@ -7,6 +7,7 @@ >> > #include <math.h> >> > >> > #include "base/callback_helpers.h" >> > +#include "base/metrics/histogram_macros.h" >> > #include "base/stl_util.h" >> > #include "ppapi/c/pp_codecs.h" >> > #include "ppapi/c/ppb_opengles2.h" >> > @@ -194,13 +195,21 @@ void >> > PepperVideoRenderer3D::ProcessVideoPacket(scoped_ptr<VideoPacket> >> packet, >> > if (!packet->data().size()) >> > return; >> > >> > - // Update statistics. >> > + // Update statistics and UMA histograms. >> > stats_.video_frame_rate()->Record(1); >> > stats_.video_bandwidth()->Record(packet->data().size()); >> > - if (packet->has_capture_time_ms()) >> > + if (packet->has_capture_time_ms()) { >> > stats_.video_capture_ms()->Record(packet->capture_time_ms()); >> > - if (packet->has_encode_time_ms()) >> > + const base::TimeDelta capture_time = >> > + base::TimeDelta::FromInternalValue(packet->capture_time_ms()); >> > + UMA_HISTOGRAM_TIMES("Chromoting.Video.CaptureLatency", >> capture_time); >> > + } >> > + if (packet->has_encode_time_ms()) { >> > stats_.video_encode_ms()->Record(packet->encode_time_ms()); >> > + const base::TimeDelta encode_time = >> > + base::TimeDelta::FromInternalValue(packet->encode_time_ms()); >> > + UMA_HISTOGRAM_TIMES("Chromoting.Video.EncodeLatency", encode_time); >> > + } >> > if (packet->has_latest_event_timestamp() && >> > packet->latest_event_timestamp() > >> latest_input_event_timestamp_) { >> > latest_input_event_timestamp_ = packet->latest_event_timestamp(); >> > @@ -208,6 +217,8 @@ void >> > PepperVideoRenderer3D::ProcessVideoPacket(scoped_ptr<VideoPacket> >> packet, >> > base::Time::Now() - >> > >> base::Time::FromInternalValue(packet->latest_event_timestamp()); >> > >> stats_.round_trip_ms()->Record(round_trip_latency.InMilliseconds()); >> > + UMA_HISTOGRAM_TIMES("Chromoting.Video.RoundTripLatency", >> > + round_trip_latency); >> > } >> > >> > bool resolution_changed = false; >> > Index: remoting/client/software_video_renderer.cc >> > diff --git a/remoting/client/software_video_renderer.cc >> > b/remoting/client/software_video_renderer.cc >> > index >> > >> > > > 51782e05f476f4942a30408a7e52d0214cc0a80f..8c8855ddddec660ecc837068bc2e01ce1b904ed7 > >> > 100644 >> > --- a/remoting/client/software_video_renderer.cc >> > +++ b/remoting/client/software_video_renderer.cc >> > @@ -11,6 +11,7 @@ >> > #include "base/callback_helpers.h" >> > #include "base/location.h" >> > #include "base/logging.h" >> > +#include "base/metrics/histogram_macros.h" >> > #include "base/single_thread_task_runner.h" >> > #include "remoting/base/util.h" >> > #include "remoting/client/frame_consumer.h" >> > @@ -362,10 +363,18 @@ void >> > SoftwareVideoRenderer::ProcessVideoPacket(scoped_ptr<VideoPacket> >> packet, >> > >> > // Record other statistics received from host. >> > stats_.video_bandwidth()->Record(packet->data().size()); >> > - if (packet->has_capture_time_ms()) >> > + if (packet->has_capture_time_ms()) { >> > stats_.video_capture_ms()->Record(packet->capture_time_ms()); >> > - if (packet->has_encode_time_ms()) >> > + const base::TimeDelta capture_time = >> > + base::TimeDelta::FromInternalValue(packet->capture_time_ms()); >> > + UMA_HISTOGRAM_TIMES("Chromoting.Video.CaptureLatency", >> capture_time); >> > + } >> > + if (packet->has_encode_time_ms()) { >> > stats_.video_encode_ms()->Record(packet->encode_time_ms()); >> > + const base::TimeDelta encode_time = >> > + base::TimeDelta::FromInternalValue(packet->encode_time_ms()); >> > + UMA_HISTOGRAM_TIMES("Chromoting.Video.EncodeLatency", encode_time); >> > + } >> > if (packet->has_latest_event_timestamp() && >> > packet->latest_event_timestamp() > latest_event_timestamp_) { >> > latest_event_timestamp_ = packet->latest_event_timestamp(); >> > @@ -373,6 +382,8 @@ void >> > SoftwareVideoRenderer::ProcessVideoPacket(scoped_ptr<VideoPacket> >> packet, >> > base::Time::Now() - >> > >> base::Time::FromInternalValue(packet->latest_event_timestamp()); >> > >> stats_.round_trip_ms()->Record(round_trip_latency.InMilliseconds()); >> > + UMA_HISTOGRAM_TIMES("Chromoting.Video.RoundTripLatency", >> > + round_trip_latency); >> > } >> > >> > // Measure the latency between the last packet being received and >> > presented. >> > >> > >> > >> > > To unsubscribe from this group and stop receiving emails from it, send an >> > email > >> to mailto:chromium-reviews+unsubscribe@chromium.org. >> > > > Thanks for the comment. > I'll look at moving the UMA gathering work to the web-app, and modify this > CL to > just involve making the frame-rate and bandwidth metrics more accurate for > a 60s > window. > > https://codereview.chromium.org/1181743005/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
https://codereview.chromium.org/1181743005/diff/40001/remoting/client/chromot... File remoting/client/chromoting_stats.cc (right): https://codereview.chromium.org/1181743005/diff/40001/remoting/client/chromot... remoting/client/chromoting_stats.cc:10: // Set to 60s, so that we average at 60s intervals, same as the rate at which we Actually ChromotingInstance::SendPerfStats() is called once per second. The webapp logs the values only every 60 seconds, but UI is updated once per second. UMA stats are aggregated on the client so we can report them more often than once a minute. I think you can just add another counter in ChromotingStats for frame rate with 1 second averages and keep existing counter at 3 second window. https://codereview.chromium.org/1181743005/diff/40001/remoting/client/chromot... remoting/client/chromoting_stats.cc:12: const int kTimeWindow = 60; I don't think we want to changed that - it's useful to have more fine-grained value in the client UI. https://codereview.chromium.org/1181743005/diff/40001/remoting/client/plugin/... File remoting/client/plugin/pepper_video_renderer_3d.cc (right): https://codereview.chromium.org/1181743005/diff/40001/remoting/client/plugin/... remoting/client/plugin/pepper_video_renderer_3d.cc:195: if (!packet->data().size()) When a frame is empty it's currently not counted for FPS (host still tries to send 30 FPS, while some of the frames are empty). This makes sense for the values we want to show in the UI, but for protocol comparison it's useful to account for empty frames as well. Essentially we want to know how often FPS drops below 30 FPS. So I think we need another FPS counter that's incremented above this line. And then ideally we should report both values with and without empty frames.
On 2015/06/19 18:46:16, Wez wrote: > I'm not saying that moving the UMA stats gathering to the web-app is > required; IIRC there is a (private) PPAPI API for that (see > https://code.google.com/p/chromium/codesearch#chromium/src/ppapi/api/private/...) > - can/will the UMA macros hook up to that, for example? There are also C++ bindings for the API, so it should be easy to use it: ppapi/cpp/private/uma_private.h
Patchset #3 (id:60001) has been deleted
Patchset #3 (id:80001) has been deleted
Patchset #3 (id:100001) has been deleted
Thanks Sergey. PTAL. https://codereview.chromium.org/1181743005/diff/40001/remoting/client/chromot... File remoting/client/chromoting_stats.cc (right): https://codereview.chromium.org/1181743005/diff/40001/remoting/client/chromot... remoting/client/chromoting_stats.cc:10: // Set to 60s, so that we average at 60s intervals, same as the rate at which we On 2015/06/19 23:08:42, Sergey Ulanov wrote: > Actually ChromotingInstance::SendPerfStats() is called once per second. The > webapp logs the values only every 60 seconds, but UI is updated once per second. > UMA stats are aggregated on the client so we can report them more often than > once a minute. I think you can just add another counter in ChromotingStats for > frame rate with 1 second averages and keep existing counter at 3 second window. OK. Created rate counters for fps and bytes/sec, and running-average counters for encode, capture and round-drip latency. It seems cleaner to create separate structs containing metrics calculated for different time-windows. Maybe do that in a follow-up CL? https://codereview.chromium.org/1181743005/diff/40001/remoting/client/chromot... remoting/client/chromoting_stats.cc:12: const int kTimeWindow = 60; On 2015/06/19 23:08:42, Sergey Ulanov wrote: > I don't think we want to changed that - it's useful to have more fine-grained > value in the client UI. Done. https://codereview.chromium.org/1181743005/diff/40001/remoting/client/plugin/... File remoting/client/plugin/pepper_video_renderer_3d.cc (right): https://codereview.chromium.org/1181743005/diff/40001/remoting/client/plugin/... remoting/client/plugin/pepper_video_renderer_3d.cc:195: if (!packet->data().size()) On 2015/06/19 23:08:42, Sergey Ulanov wrote: > When a frame is empty it's currently not counted for FPS (host still tries to > send 30 FPS, while some of the frames are empty). This makes sense for the > values we want to show in the UI, but for protocol comparison it's useful to > account for empty frames as well. Essentially we want to know how often FPS > drops below 30 FPS. So I think we need another FPS counter that's incremented > above this line. And then ideally we should report both values with and without > empty frames. Done.
Sergey/Wez, PTAL at patch-set #4. I figured out that there is no reason to have additional RunningAverage counters for the latency-stats. We can just average over the last 10 values every second when updating UMA stats. Also in PS#4: adding the FPS counters including empty packets that I forgot to include earlier.
dahlke@google.com changed reviewers: + dahlke@google.com
https://codereview.chromium.org/1181743005/diff/140001/remoting/client/plugin... File remoting/client/plugin/chromoting_instance.cc (right): https://codereview.chromium.org/1181743005/diff/140001/remoting/client/plugin... remoting/client/plugin/chromoting_instance.cc:1082: // For, the latency metrics use a max value of 1000ms. The max latency for the past 7 days is 11,381ms. If we must have a max value, we should give ourselves some buffer (20,000) and understand what the effect is if we go over that max value.
https://codereview.chromium.org/1181743005/diff/140001/remoting/client/chromo... File remoting/client/chromoting_stats.cc (right): https://codereview.chromium.org/1181743005/diff/140001/remoting/client/chromo... remoting/client/chromoting_stats.cc:10: const int kTimeWindow = 3; nit: Rename this to clarify why it differs from the other windows below. https://codereview.chromium.org/1181743005/diff/140001/remoting/client/chromo... remoting/client/chromoting_stats.cc:13: const int kOneSecWindow = 1; Don't name this after the actual amount it represents - then you may as well just use the literal 1 in the code. If you instead name it after the concept it represents, e.g. kFineGrainedStatsWindowSecs then you no longer need to ranem it if the value changes, and you've expressed its meaning/purpose. https://codereview.chromium.org/1181743005/diff/140001/remoting/client/chromo... remoting/client/chromoting_stats.cc:16: const int kLatencyWindow = 10; nit: Rename this to include the units in the name? https://codereview.chromium.org/1181743005/diff/140001/remoting/client/chromo... File remoting/client/chromoting_stats.h (right): https://codereview.chromium.org/1181743005/diff/140001/remoting/client/chromo... remoting/client/chromoting_stats.h:21: RateCounter* video_bandwidth() { return &video_bandwidth_; } While you're here, can we add aunits suffix to this member, please? https://codereview.chromium.org/1181743005/diff/140001/remoting/client/chromo... remoting/client/chromoting_stats.h:23: RateCounter* video_fps() { return &video_fps_; } What is the difference between this and video_frame_rate(), above...? https://codereview.chromium.org/1181743005/diff/140001/remoting/client/chromo... remoting/client/chromoting_stats.h:24: RateCounter* video_bytes_per_s() { return &video_bytes_per_s_; } Similarly, consider clarifying how this differs from "bandwidth", above? https://codereview.chromium.org/1181743005/diff/140001/remoting/client/chromo... remoting/client/chromoting_stats.h:25: RateCounter* video_fps_with_empty_frames() { There is a 1:1 correspondence between video frames and video packets (aka VideoPacket in our protocol) - it may be clearer to use video_packets_per_s() here. You may wish to add comments to each of these fields to explain their purpose/limitations, etc (e.g. the fact that video_fps() will be zero if there are no actual on-screen changes to deliver. https://codereview.chromium.org/1181743005/diff/140001/remoting/client/plugin... File remoting/client/plugin/chromoting_instance.cc (right): https://codereview.chromium.org/1181743005/diff/140001/remoting/client/plugin... remoting/client/plugin/chromoting_instance.cc:1058: scoped_ptr<base::DictionaryValue> data(new base::DictionaryValue()); Suggest a comment here: "Post performance stats averaged over the past few seconds, to JS, to log, and optionally to display to the user." or similar. https://codereview.chromium.org/1181743005/diff/140001/remoting/client/plugin... remoting/client/plugin/chromoting_instance.cc:1069: // Also update UMA histograms for video frame-rate, bandwidth and latencies. nit: "Also" implies that there is a comment earlier on to explain what we were previously doing. Suggest just "Update UMA with stats averaged over the last second." https://codereview.chromium.org/1181743005/diff/140001/remoting/client/plugin... remoting/client/plugin/chromoting_instance.cc:1071: // We don't expect a frame-rate greater than 40fps, so set boundary to 40. This comment should appear next to a constant defining the value, used in the calls below. https://codereview.chromium.org/1181743005/diff/140001/remoting/client/plugin... remoting/client/plugin/chromoting_instance.cc:1078: // For bandwidth, set the max value to 1MB/s, spread across 1000 buckets. Ditto. What is the rationale for 1000 buckets? https://codereview.chromium.org/1181743005/diff/140001/remoting/client/plugin... remoting/client/plugin/chromoting_instance.cc:1082: // For, the latency metrics use a max value of 1000ms. Ditto re constants as above, and rationale for the 1000 buckets, and the choice of max latency. https://codereview.chromium.org/1181743005/diff/140001/remoting/client/plugin... File remoting/client/plugin/pepper_video_renderer_3d.cc (right): https://codereview.chromium.org/1181743005/diff/140001/remoting/client/plugin... remoting/client/plugin/pepper_video_renderer_3d.cc:203: stats_.video_bytes_per_s()->Record(packet->data().size()); This ends up being pretty ugly. Could we instead switch all the plugin-side sampling to per-second and build the longer-term averaging into the app, for example? Would need some care to make sure the difference in calculations didn't lead to output incomparable with older stats.
Patchset #5 (id:160001) has been deleted
Thanks Wez. PTAL at patch-set #5. https://codereview.chromium.org/1181743005/diff/140001/remoting/client/chromo... File remoting/client/chromoting_stats.cc (right): https://codereview.chromium.org/1181743005/diff/140001/remoting/client/chromo... remoting/client/chromoting_stats.cc:10: const int kTimeWindow = 3; On 2015/06/29 14:56:13, Wez wrote: > nit: Rename this to clarify why it differs from the other windows below. Done. https://codereview.chromium.org/1181743005/diff/140001/remoting/client/chromo... remoting/client/chromoting_stats.cc:13: const int kOneSecWindow = 1; On 2015/06/29 14:56:13, Wez wrote: > Don't name this after the actual amount it represents - then you may as well > just use the literal 1 in the code. If you instead name it after the concept it > represents, e.g. kFineGrainedStatsWindowSecs then you no longer need to ranem it > if the value changes, and you've expressed its meaning/purpose. Done. Thanks. :-) https://codereview.chromium.org/1181743005/diff/140001/remoting/client/chromo... remoting/client/chromoting_stats.cc:16: const int kLatencyWindow = 10; On 2015/06/29 14:56:13, Wez wrote: > nit: Rename this to include the units in the name? Done. https://codereview.chromium.org/1181743005/diff/140001/remoting/client/chromo... File remoting/client/chromoting_stats.h (right): https://codereview.chromium.org/1181743005/diff/140001/remoting/client/chromo... remoting/client/chromoting_stats.h:21: RateCounter* video_bandwidth() { return &video_bandwidth_; } On 2015/06/29 14:56:13, Wez wrote: > While you're here, can we add aunits suffix to this member, please? I've used Bps, to indicate Bytes/sec. https://codereview.chromium.org/1181743005/diff/140001/remoting/client/chromo... remoting/client/chromoting_stats.h:23: RateCounter* video_fps() { return &video_fps_; } On 2015/06/29 14:56:13, Wez wrote: > What is the difference between this and video_frame_rate(), above...? Video frame-rate has a time-window of 3s. This one has a time-window of 1s. I've added a suffix to indicate the way these metrics are used. https://codereview.chromium.org/1181743005/diff/140001/remoting/client/chromo... remoting/client/chromoting_stats.h:24: RateCounter* video_bytes_per_s() { return &video_bytes_per_s_; } On 2015/06/29 14:56:13, Wez wrote: > Similarly, consider clarifying how this differs from "bandwidth", above? Done. https://codereview.chromium.org/1181743005/diff/140001/remoting/client/chromo... remoting/client/chromoting_stats.h:25: RateCounter* video_fps_with_empty_frames() { On 2015/06/29 14:56:13, Wez wrote: > There is a 1:1 correspondence between video frames and video packets (aka > VideoPacket in our protocol) - it may be clearer to use video_packets_per_s() > here. > > You may wish to add comments to each of these fields to explain their > purpose/limitations, etc (e.g. the fact that video_fps() will be zero if there > are no actual on-screen changes to deliver. Done. https://codereview.chromium.org/1181743005/diff/140001/remoting/client/plugin... File remoting/client/plugin/chromoting_instance.cc (right): https://codereview.chromium.org/1181743005/diff/140001/remoting/client/plugin... remoting/client/plugin/chromoting_instance.cc:1058: scoped_ptr<base::DictionaryValue> data(new base::DictionaryValue()); On 2015/06/29 14:56:13, Wez wrote: > Suggest a comment here: > > "Post performance stats averaged over the past few seconds, to JS, to log, and > optionally to display to the user." > > or similar. Done. https://codereview.chromium.org/1181743005/diff/140001/remoting/client/plugin... remoting/client/plugin/chromoting_instance.cc:1069: // Also update UMA histograms for video frame-rate, bandwidth and latencies. On 2015/06/29 14:56:13, Wez wrote: > nit: "Also" implies that there is a comment earlier on to explain what we were > previously doing. > > Suggest just "Update UMA with stats averaged over the last second." Done. https://codereview.chromium.org/1181743005/diff/140001/remoting/client/plugin... remoting/client/plugin/chromoting_instance.cc:1071: // We don't expect a frame-rate greater than 40fps, so set boundary to 40. On 2015/06/29 14:56:13, Wez wrote: > This comment should appear next to a constant defining the value, used in the > calls below. Thanks. Done. Also updated to 41, based on explanation here: https://code.google.com/p/chromium/codesearch#chromium/src/ppapi/c/private/pp... https://codereview.chromium.org/1181743005/diff/140001/remoting/client/plugin... remoting/client/plugin/chromoting_instance.cc:1078: // For bandwidth, set the max value to 1MB/s, spread across 1000 buckets. On 2015/06/29 14:56:13, Wez wrote: > Ditto. > > What is the rationale for 1000 buckets? Done. If, after we go live with these settings, we find that the histograms we get are not fine-grained enough, we can tweak them. If that does happen, we should rename the metrics, rather than change the settings of existing metrics, because that messes up historical comparisons. So, while the current settings look OK to me, please let me know if you have better recommendations for these values. Once we have OK from our team for the current histogram values and metric names, I plan on getting them reviewed by a UMA team member as well. https://codereview.chromium.org/1181743005/diff/140001/remoting/client/plugin... remoting/client/plugin/chromoting_instance.cc:1082: // For, the latency metrics use a max value of 1000ms. On 2015/06/29 14:56:13, Wez wrote: > Ditto re constants as above, and rationale for the 1000 buckets, and the choice > of max latency. Done. https://codereview.chromium.org/1181743005/diff/140001/remoting/client/plugin... File remoting/client/plugin/pepper_video_renderer_3d.cc (right): https://codereview.chromium.org/1181743005/diff/140001/remoting/client/plugin... remoting/client/plugin/pepper_video_renderer_3d.cc:203: stats_.video_bytes_per_s()->Record(packet->data().size()); On 2015/06/29 14:56:13, Wez wrote: > This ends up being pretty ugly. > > Could we instead switch all the plugin-side sampling to per-second and build the > longer-term averaging into the app, for example? Would need some care to make > sure the difference in calculations didn't lead to output incomparable with > older stats. Agreed. If we use only the per-second sampling, the stats shown in the web-app will only report FPS, etc for the last second every 60s. This might be acceptable, but might also make sudden ups/downs more prominent, depending on timing. Other than that, I don't think there are other places where the default 3s window metrics are used. sergeyu@ might know more about any other implications of removing the 3s window metrics. In the meantime, I've updated the metrics names to hopefully make their different intents more obvious and make this a little less ugly. https://codereview.chromium.org/1181743005/diff/180001/remoting/client/softwa... File remoting/client/software_video_renderer.cc (right): https://codereview.chromium.org/1181743005/diff/180001/remoting/client/softwa... remoting/client/software_video_renderer.cc:355: stats_.video_packets_per_s_UMA()->Record(1); There is some duplication here with https://code.google.com/p/chromium/codesearch#chromium/src/remoting/client/pl.... Is it feasible/useful to de-dupe this work into a common function, or are they getting merged into entirely different libraries?
https://codereview.chromium.org/1181743005/diff/180001/remoting/client/chromo... File remoting/client/chromoting_stats.cc (right): https://codereview.chromium.org/1181743005/diff/180001/remoting/client/chromo... remoting/client/chromoting_stats.cc:12: const int kWebAppStatsTimeWindow = 3; I suggest calling it kDisplayStatsTimeWindowSeconds to avoid dependency on "webapp" and to make it clear which units we use here. https://codereview.chromium.org/1181743005/diff/180001/remoting/client/chromo... remoting/client/chromoting_stats.cc:15: const int kFineGrainedStatsWindow = 1; kUmaStatsWindowSeconds https://codereview.chromium.org/1181743005/diff/180001/remoting/client/chromo... remoting/client/chromoting_stats.cc:15: const int kFineGrainedStatsWindow = 1; This value is essentially duplicated in as kPerfStatsIntervalMs in remoting/client/plugin/chromoting_instance.cc. I suggest making it a public const in ChromotingStats and using in chromoting_instance.cc. to make sure that UMA stats are reported at the correct rate. And then it might be better to have 2 separate timers: 1 to report UMA stats and another one to update UI. https://codereview.chromium.org/1181743005/diff/180001/remoting/client/chromo... File remoting/client/chromoting_stats.h (right): https://codereview.chromium.org/1181743005/diff/180001/remoting/client/chromo... remoting/client/chromoting_stats.h:21: RateCounter* video_Bps_webapp() { return &video_Bps_webapp_; } style guide doesn't allow mixed case names like these. Also abbreviations like "bps" and "fps" are discouraged. Another problem is that _webapp suffix doesn't make sense for android and iOS builds. I suggest the followingnames: video_bandwidth_display() video_frame_rate_display() video_bandwidth_uma() video_frame_rate_uma() https://codereview.chromium.org/1181743005/diff/180001/remoting/client/chromo... remoting/client/chromoting_stats.h:25: RateCounter* video_packets_per_s_UMA() { Maybe call it video_packet_rate_uma()? https://codereview.chromium.org/1181743005/diff/180001/remoting/client/chromo... remoting/client/chromoting_stats.h:35: // The following 2 metrics are used in the stats captured every 60s for IMO it would be better to move put these comments next to the public accessors above https://codereview.chromium.org/1181743005/diff/180001/remoting/client/chromo... remoting/client/chromoting_stats.h:36: // display within the web-app. "web-app" doesn't make sense in context of android app. https://codereview.chromium.org/1181743005/diff/180001/remoting/client/chromo... remoting/client/chromoting_stats.h:53: // Time to encode a captured video-packet, measured in ms. "measured in ms" is redundant because there is ms_ suffix. https://codereview.chromium.org/1181743005/diff/180001/remoting/client/plugin... File remoting/client/plugin/pepper_video_renderer_3d.cc (right): https://codereview.chromium.org/1181743005/diff/180001/remoting/client/plugin... remoting/client/plugin/pepper_video_renderer_3d.cc:195: if (!packet->data().size()) { please don't add {} for consistency with other single-line if statements in this file. https://codereview.chromium.org/1181743005/diff/180001/remoting/client/plugin... remoting/client/plugin/pepper_video_renderer_3d.cc:199: // Update statistics. This code is duplicated in SoftwareVideoRenderer. I think ideally it should be moved to ChromotingStats. Please add TODO here.
Patchset #6 (id:200001) has been deleted
Patchset #6 (id:220001) has been deleted
Thanks Sergey. PTAL at patch-set #6. https://codereview.chromium.org/1181743005/diff/180001/remoting/client/chromo... File remoting/client/chromoting_stats.cc (right): https://codereview.chromium.org/1181743005/diff/180001/remoting/client/chromo... remoting/client/chromoting_stats.cc:12: const int kWebAppStatsTimeWindow = 3; On 2015/07/06 21:23:12, Sergey Ulanov wrote: > I suggest calling it kDisplayStatsTimeWindowSeconds to avoid dependency on > "webapp" and to make it clear which units we use here. Done. https://codereview.chromium.org/1181743005/diff/180001/remoting/client/chromo... remoting/client/chromoting_stats.cc:15: const int kFineGrainedStatsWindow = 1; On 2015/07/06 21:23:12, Sergey Ulanov wrote: > kUmaStatsWindowSeconds Done. https://codereview.chromium.org/1181743005/diff/180001/remoting/client/chromo... remoting/client/chromoting_stats.cc:15: const int kFineGrainedStatsWindow = 1; On 2015/07/06 21:23:12, Sergey Ulanov wrote: > This value is essentially duplicated in as kPerfStatsIntervalMs in > remoting/client/plugin/chromoting_instance.cc. I suggest making it a public > const in ChromotingStats and using in chromoting_instance.cc. to make sure that > UMA stats are reported at the correct rate. And then it might be better to have > 2 separate timers: 1 to report UMA stats and another one to update UI. Done. https://codereview.chromium.org/1181743005/diff/180001/remoting/client/chromo... File remoting/client/chromoting_stats.h (right): https://codereview.chromium.org/1181743005/diff/180001/remoting/client/chromo... remoting/client/chromoting_stats.h:21: RateCounter* video_Bps_webapp() { return &video_Bps_webapp_; } On 2015/07/06 21:23:12, Sergey Ulanov wrote: > style guide doesn't allow mixed case names like these. Also abbreviations like > "bps" and "fps" are discouraged. Another problem is that _webapp suffix doesn't > make sense for android and iOS builds. > I suggest the followingnames: > video_bandwidth_display() > video_frame_rate_display() > video_bandwidth_uma() > video_frame_rate_uma() Done. https://codereview.chromium.org/1181743005/diff/180001/remoting/client/chromo... remoting/client/chromoting_stats.h:25: RateCounter* video_packets_per_s_UMA() { On 2015/07/06 21:23:12, Sergey Ulanov wrote: > Maybe call it video_packet_rate_uma()? Done. https://codereview.chromium.org/1181743005/diff/180001/remoting/client/chromo... remoting/client/chromoting_stats.h:35: // The following 2 metrics are used in the stats captured every 60s for On 2015/07/06 21:23:12, Sergey Ulanov wrote: > IMO it would be better to move put these comments next to the public accessors > above Done. https://codereview.chromium.org/1181743005/diff/180001/remoting/client/chromo... remoting/client/chromoting_stats.h:36: // display within the web-app. On 2015/07/06 21:23:12, Sergey Ulanov wrote: > "web-app" doesn't make sense in context of android app. Done. https://codereview.chromium.org/1181743005/diff/180001/remoting/client/chromo... remoting/client/chromoting_stats.h:53: // Time to encode a captured video-packet, measured in ms. On 2015/07/06 21:23:12, Sergey Ulanov wrote: > "measured in ms" is redundant because there is ms_ suffix. Done. https://codereview.chromium.org/1181743005/diff/180001/remoting/client/plugin... File remoting/client/plugin/pepper_video_renderer_3d.cc (right): https://codereview.chromium.org/1181743005/diff/180001/remoting/client/plugin... remoting/client/plugin/pepper_video_renderer_3d.cc:195: if (!packet->data().size()) { On 2015/07/06 21:23:12, Sergey Ulanov wrote: > please don't add {} for consistency with other single-line if statements in this > file. Yes, didn't clean this up from previous patch-set where we had multiple lines here. https://codereview.chromium.org/1181743005/diff/180001/remoting/client/plugin... remoting/client/plugin/pepper_video_renderer_3d.cc:199: // Update statistics. On 2015/07/06 21:23:12, Sergey Ulanov wrote: > This code is duplicated in SoftwareVideoRenderer. I think ideally it should be > moved to ChromotingStats. Please add TODO here. Done.
With the display-stats and UMA stats now being updated on different timers, we can set the time-window for display-stats rate-counters separately and cleanly. So changed it back to 3s as before. PTAL at PS#7. Thanks.
On 2015/07/07 02:23:04, anandc wrote: > With the display-stats and UMA stats now being updated on different timers, we > can set the time-window for display-stats rate-counters separately and cleanly. > So changed it back to 3s as before. > > PTAL at PS#7. Thanks. Ping.
LGTM after my comments are addressed https://codereview.chromium.org/1181743005/diff/260001/remoting/client/chromo... File remoting/client/chromoting_stats.h (right): https://codereview.chromium.org/1181743005/diff/260001/remoting/client/chromo... remoting/client/chromoting_stats.h:22: const int kUMAStatsTimeWindowInSeconds = 1; declare these are static const. https://codereview.chromium.org/1181743005/diff/260001/remoting/client/plugin... File remoting/client/plugin/chromoting_instance.cc (right): https://codereview.chromium.org/1181743005/diff/260001/remoting/client/plugin... remoting/client/plugin/chromoting_instance.cc:1076: ChromotingStats* stats = video_renderer_->GetStats(); you can keep this line where it was - the consts in ChromotingStats should be static. https://codereview.chromium.org/1181743005/diff/260001/remoting/client/plugin... remoting/client/plugin/chromoting_instance.cc:1101: ChromotingStats* stats = video_renderer_->GetStats(); move this below, before uma_interface calls. https://codereview.chromium.org/1181743005/diff/260001/remoting/client/plugin... remoting/client/plugin/chromoting_instance.cc:1109: pp::UMAPrivate uma_interface(this); Maybe call this uma?
Addressed comments. https://codereview.chromium.org/1181743005/diff/260001/remoting/client/chromo... File remoting/client/chromoting_stats.h (right): https://codereview.chromium.org/1181743005/diff/260001/remoting/client/chromo... remoting/client/chromoting_stats.h:22: const int kUMAStatsTimeWindowInSeconds = 1; On 2015/07/08 20:06:47, Sergey Ulanov wrote: > declare these are static const. Done. https://codereview.chromium.org/1181743005/diff/260001/remoting/client/plugin... File remoting/client/plugin/chromoting_instance.cc (right): https://codereview.chromium.org/1181743005/diff/260001/remoting/client/plugin... remoting/client/plugin/chromoting_instance.cc:1076: ChromotingStats* stats = video_renderer_->GetStats(); On 2015/07/08 20:06:47, Sergey Ulanov wrote: > you can keep this line where it was - the consts in ChromotingStats should be > static. Done. https://codereview.chromium.org/1181743005/diff/260001/remoting/client/plugin... remoting/client/plugin/chromoting_instance.cc:1101: ChromotingStats* stats = video_renderer_->GetStats(); On 2015/07/08 20:06:47, Sergey Ulanov wrote: > move this below, before uma_interface calls. Done. https://codereview.chromium.org/1181743005/diff/260001/remoting/client/plugin... remoting/client/plugin/chromoting_instance.cc:1109: pp::UMAPrivate uma_interface(this); On 2015/07/08 20:06:47, Sergey Ulanov wrote: > Maybe call this uma? Done.
lgtm
https://codereview.chromium.org/1181743005/diff/280001/remoting/client/chromo... File remoting/client/chromoting_stats.h (right): https://codereview.chromium.org/1181743005/diff/280001/remoting/client/chromo... remoting/client/chromoting_stats.h:22: static const int kUMAStatsTimeWindowInSeconds = 1; Why is this "TimeWindow" and the one below "RateWindow" and the final one "UpdateWindow"? The combination of the names + comments doesn't give me any real insight into how these are used - why the UMA stats are "time" while the displays stats are "rates", and what it is that we update every 60s - the latter is the existing (non-UMA) stats logging, isn't it? Why not name these to match the _display vs _uma naming, below? Also, why are these statics on this class, rather than constants within the implementation? https://codereview.chromium.org/1181743005/diff/280001/remoting/client/chromo... remoting/client/chromoting_stats.h:29: // display to users. Display stats are captured every second for display to users, not every 60s. Do you mean the stats upload? https://codereview.chromium.org/1181743005/diff/280001/remoting/client/chromo... remoting/client/chromoting_stats.h:36: // Bytes/sec for non-empty video-packets, measured over a 1s time-window. You're calling this video_bandwidth_uma but then specifying in the comment the size of the window - either call this video_bandwidth_1s so it's clear it's the video-bandwidth over the past second, or remove the window aspect from the comment, since it is documented via the constants above. https://codereview.chromium.org/1181743005/diff/280001/remoting/client/chromo... remoting/client/chromoting_stats.h:46: // updated every 60s. This isn't true, AFAIK; these stats are updated after every frame, and can be sampled at any time - UMA & display will presumably sample every 1s and non-UMA stats upload every 60s.
https://codereview.chromium.org/1181743005/diff/280001/remoting/client/chromo... File remoting/client/chromoting_stats.h (right): https://codereview.chromium.org/1181743005/diff/280001/remoting/client/chromo... remoting/client/chromoting_stats.h:22: static const int kUMAStatsTimeWindowInSeconds = 1; nit: kUmaStats... https://codereview.chromium.org/1181743005/diff/280001/remoting/client/chromo... remoting/client/chromoting_stats.h:46: // updated every 60s. On 2015/07/08 23:27:55, Wez wrote: > This isn't true, AFAIK; these stats are updated after every frame, and can be > sampled at any time - UMA & display will presumably sample every 1s and non-UMA > stats upload every 60s. It's also misleading since these stats are averaged over the past 10s, in contrast to the display values (averaged over 3s) and the UMA values (averaged over 1s). https://codereview.chromium.org/1181743005/diff/280001/remoting/client/plugin... File remoting/client/plugin/chromoting_instance.cc (right): https://codereview.chromium.org/1181743005/diff/280001/remoting/client/plugin... remoting/client/plugin/chromoting_instance.cc:75: // be greater than 40fps. Why not? Where does the 40fps come from? What happens if a load of frames get stuck in-transit and we receive them in a sudden burst? Also, doesn't this mean things will break if we e.g. upgrade to 60fps hosts? https://codereview.chromium.org/1181743005/diff/280001/remoting/client/plugin... remoting/client/plugin/chromoting_instance.cc:77: const int kMaxFrameRate = 41; nit: Blank line after this. https://codereview.chromium.org/1181743005/diff/280001/remoting/client/plugin... remoting/client/plugin/chromoting_instance.cc:78: // For bandwidth, we'll use a histogram ranging from 0 to 1MB/s, spread across Why is 1MB/s the maximum? https://codereview.chromium.org/1181743005/diff/280001/remoting/client/plugin... remoting/client/plugin/chromoting_instance.cc:89: // over 1000 buckets. Again, explain why these numbers were chosen? https://codereview.chromium.org/1181743005/diff/280001/remoting/client/plugin... remoting/client/plugin/chromoting_instance.cc:754: video_renderer_->GetStats()->kDisplayStatsUpdateWindowInSeconds)); This is a constant, not a member, so you can just write ChromotingStats::kDisplayStatsUpdateWindowInSeconds https://codereview.chromium.org/1181743005/diff/280001/remoting/client/plugin... remoting/client/plugin/chromoting_instance.cc:1084: // the user. This is very vague... why "eventually", and what does "upstreamed" mean? I'd suggest something like "Supply the app with performance stats averaged over several seconds, suitable for display to the user." https://codereview.chromium.org/1181743005/diff/280001/remoting/client/plugin... remoting/client/plugin/chromoting_instance.cc:1109: // These metrics will be averaged over the last second. Will be averaged by whom...? The UMA APIs average them? Aren't we already averaging them before we even reach here? https://codereview.chromium.org/1181743005/diff/280001/remoting/client/plugin... remoting/client/plugin/chromoting_instance.cc:1119: uma.HistogramCustomTimes("Chromoting.Video.CaptureLatency", nit: This isn't a latency, it's the time taken to capture; suggest CaptureMs for the name, and similarly for the other values below. Also, the values you're logging here are already averaged over the past 10s, which means that any averaging that UMA APIs do will mess things up, surely? Don't you need special UMA-friendly 1s-averaged versions of these? Should we instead be logging these stats for every frame, and letting UMA do all the averaging, and then doing separate averaging for the display stats? https://codereview.chromium.org/1181743005/diff/280001/remoting/client/plugin... File remoting/client/plugin/chromoting_instance.h (right): https://codereview.chromium.org/1181743005/diff/280001/remoting/client/plugin... remoting/client/plugin/chromoting_instance.h:224: void SendUMAPerfStats(); nit: SendUmaPerfStats() as per style-guide naming conventions. Also suggest a single-line comment to introduce these two methods e.g. "Helpers to send stats to the web-app for display and UMA logging." https://codereview.chromium.org/1181743005/diff/280001/remoting/client/plugin... File remoting/client/plugin/pepper_video_renderer_3d.cc (right): https://codereview.chromium.org/1181743005/diff/280001/remoting/client/plugin... remoting/client/plugin/pepper_video_renderer_3d.cc:200: // software_video_renderer.cc. Move to ChromotingStats. nit: File a bug for that work, and refer to it in this comment, please. :) https://codereview.chromium.org/1181743005/diff/280001/remoting/client/softwa... File remoting/client/software_video_renderer.cc (right): https://codereview.chromium.org/1181743005/diff/280001/remoting/client/softwa... remoting/client/software_video_renderer.cc:355: stats_.video_packet_rate_uma()->Record(1); nit: This line belongs above the comment.
Patchset #9 (id:300001) has been deleted
Patchset #9 (id:320001) has been deleted
Thanks a lot for the comments Wez. They helped me see a bug introduced in PS #6 with the rate at which perf-stats are updated, and also made clear the inaccuracy of latency metrics if reported to UMA in the current manner. The CL has been cleaned up now to only upload the rate-metrics to UMA, and average them over 1s, instead of 3s. CL description has also been updated. https://codereview.chromium.org/1181743005/diff/280001/remoting/client/chromo... File remoting/client/chromoting_stats.h (right): https://codereview.chromium.org/1181743005/diff/280001/remoting/client/chromo... remoting/client/chromoting_stats.h:22: static const int kUMAStatsTimeWindowInSeconds = 1; On 2015/07/08 23:42:29, Wez wrote: > nit: kUmaStats... Acknowledged. https://codereview.chromium.org/1181743005/diff/280001/remoting/client/chromo... remoting/client/chromoting_stats.h:22: static const int kUMAStatsTimeWindowInSeconds = 1; On 2015/07/08 23:27:54, Wez wrote: > Why is this "TimeWindow" and the one below "RateWindow" and the final one > "UpdateWindow"? > > The combination of the names + comments doesn't give me any real insight into > how these are used - why the UMA stats are "time" while the displays stats are > "rates", and what it is that we update every 60s - the latter is the existing > (non-UMA) stats logging, isn't it? > > Why not name these to match the _display vs _uma naming, below? > > Also, why are these statics on this class, rather than constants within the > implementation? Cleaned these up. Also added comment for why the static is declared here. https://codereview.chromium.org/1181743005/diff/280001/remoting/client/chromo... remoting/client/chromoting_stats.h:29: // display to users. On 2015/07/08 23:27:54, Wez wrote: > Display stats are captured every second for display to users, not every 60s. Do > you mean the stats upload? Sorry, these were quite confusing. PTAL at latest comments. https://codereview.chromium.org/1181743005/diff/280001/remoting/client/chromo... remoting/client/chromoting_stats.h:36: // Bytes/sec for non-empty video-packets, measured over a 1s time-window. On 2015/07/08 23:27:54, Wez wrote: > You're calling this video_bandwidth_uma but then specifying in the comment the > size of the window - either call this video_bandwidth_1s so it's clear it's the > video-bandwidth over the past second, or remove the window aspect from the > comment, since it is documented via the constants above. Done. https://codereview.chromium.org/1181743005/diff/280001/remoting/client/chromo... remoting/client/chromoting_stats.h:46: // updated every 60s. On 2015/07/08 23:27:55, Wez wrote: > This isn't true, AFAIK; these stats are updated after every frame, and can be > sampled at any time - UMA & display will presumably sample every 1s and non-UMA > stats upload every 60s. Acknowledged. https://codereview.chromium.org/1181743005/diff/280001/remoting/client/chromo... remoting/client/chromoting_stats.h:46: // updated every 60s. On 2015/07/08 23:42:29, Wez wrote: > On 2015/07/08 23:27:55, Wez wrote: > > This isn't true, AFAIK; these stats are updated after every frame, and can be > > sampled at any time - UMA & display will presumably sample every 1s and > non-UMA > > stats upload every 60s. > > It's also misleading since these stats are averaged over the past 10s, in > contrast to the display values (averaged over 3s) and the UMA values (averaged > over 1s). Acknowledged. https://codereview.chromium.org/1181743005/diff/280001/remoting/client/plugin... File remoting/client/plugin/chromoting_instance.cc (right): https://codereview.chromium.org/1181743005/diff/280001/remoting/client/plugin... remoting/client/plugin/chromoting_instance.cc:75: // be greater than 40fps. On 2015/07/08 23:42:29, Wez wrote: > Why not? Where does the 40fps come from? What happens if a load of frames get > stuck in-transit and we receive them in a sudden burst? > > Also, doesn't this mean things will break if we e.g. upgrade to 60fps hosts? The 40fps came from our expected happy-path scenario. To handle the scenario you point out, and leave some room for growth, updated this to 60. Values above the max end up in the right-most bucket of the histograms. https://codereview.chromium.org/1181743005/diff/280001/remoting/client/plugin... remoting/client/plugin/chromoting_instance.cc:77: const int kMaxFrameRate = 41; On 2015/07/08 23:42:29, Wez wrote: > nit: Blank line after this. Done. https://codereview.chromium.org/1181743005/diff/280001/remoting/client/plugin... remoting/client/plugin/chromoting_instance.cc:78: // For bandwidth, we'll use a histogram ranging from 0 to 1MB/s, spread across On 2015/07/08 23:42:29, Wez wrote: > Why is 1MB/s the maximum? Added clarification. https://codereview.chromium.org/1181743005/diff/280001/remoting/client/plugin... remoting/client/plugin/chromoting_instance.cc:89: // over 1000 buckets. On 2015/07/08 23:42:29, Wez wrote: > Again, explain why these numbers were chosen? Removed from this CL. https://codereview.chromium.org/1181743005/diff/280001/remoting/client/plugin... remoting/client/plugin/chromoting_instance.cc:754: video_renderer_->GetStats()->kDisplayStatsUpdateWindowInSeconds)); On 2015/07/08 23:42:29, Wez wrote: > This is a constant, not a member, so you can just write > ChromotingStats::kDisplayStatsUpdateWindowInSeconds Done. https://codereview.chromium.org/1181743005/diff/280001/remoting/client/plugin... remoting/client/plugin/chromoting_instance.cc:1084: // the user. On 2015/07/08 23:42:29, Wez wrote: > This is very vague... why "eventually", and what does "upstreamed" mean? > > I'd suggest something like "Supply the app with performance stats averaged over > several seconds, suitable for display to the user." Thanks. PTAL at latest comment. https://codereview.chromium.org/1181743005/diff/280001/remoting/client/plugin... remoting/client/plugin/chromoting_instance.cc:1109: // These metrics will be averaged over the last second. On 2015/07/08 23:42:29, Wez wrote: > Will be averaged by whom...? The UMA APIs average them? Aren't we already > averaging them before we even reach here? Yes, we are averaging them. Clarified. https://codereview.chromium.org/1181743005/diff/280001/remoting/client/plugin... remoting/client/plugin/chromoting_instance.cc:1119: uma.HistogramCustomTimes("Chromoting.Video.CaptureLatency", On 2015/07/08 23:42:29, Wez wrote: > nit: This isn't a latency, it's the time taken to capture; suggest CaptureMs for > the name, and similarly for the other values below. > > Also, the values you're logging here are already averaged over the past 10s, > which means that any averaging that UMA APIs do will mess things up, surely? > Don't you need special UMA-friendly 1s-averaged versions of these? > > Should we instead be logging these stats for every frame, and letting UMA do all > the averaging, and then doing separate averaging for the display stats? You are correct. The latency metrics, if uploaded to UMA in this manner, won't be accurate, as you point out. Going to do the accurate calculation and upload in a separate CL. Tracked by https://code.google.com/p/chromium/issues/detail?id=508602 https://codereview.chromium.org/1181743005/diff/280001/remoting/client/plugin... File remoting/client/plugin/chromoting_instance.h (right): https://codereview.chromium.org/1181743005/diff/280001/remoting/client/plugin... remoting/client/plugin/chromoting_instance.h:224: void SendUMAPerfStats(); On 2015/07/08 23:42:29, Wez wrote: > nit: SendUmaPerfStats() as per style-guide naming conventions. > > Also suggest a single-line comment to introduce these two methods e.g. "Helpers > to send stats to the web-app for display and UMA logging." Reverted to a single function now. https://codereview.chromium.org/1181743005/diff/280001/remoting/client/plugin... File remoting/client/plugin/pepper_video_renderer_3d.cc (right): https://codereview.chromium.org/1181743005/diff/280001/remoting/client/plugin... remoting/client/plugin/pepper_video_renderer_3d.cc:200: // software_video_renderer.cc. Move to ChromotingStats. On 2015/07/08 23:42:30, Wez wrote: > nit: File a bug for that work, and refer to it in this comment, please. :) Done. https://codereview.chromium.org/1181743005/diff/280001/remoting/client/softwa... File remoting/client/software_video_renderer.cc (right): https://codereview.chromium.org/1181743005/diff/280001/remoting/client/softwa... remoting/client/software_video_renderer.cc:355: stats_.video_packet_rate_uma()->Record(1); On 2015/07/08 23:42:30, Wez wrote: > nit: This line belongs above the comment. Done.
rkaplow@chromium.org changed reviewers: + rkaplow@chromium.org
https://codereview.chromium.org/1181743005/diff/340001/remoting/client/plugin... File remoting/client/plugin/chromoting_instance.cc (right): https://codereview.chromium.org/1181743005/diff/340001/remoting/client/plugin... remoting/client/plugin/chromoting_instance.cc:77: // Histograms expect samples to be less than the boundary value, so set to 41. 61 https://codereview.chromium.org/1181743005/diff/340001/remoting/client/plugin... remoting/client/plugin/chromoting_instance.cc:81: // ranging from 0 to 10MB/s, spread across 1000 buckets. I feel 1000 buckets is excessive. I suspect you should be fine with less, such as 100. Note that since this is log scaled (as you mention) you will get more precision for the smaller bucket sizes. https://codereview.chromium.org/1181743005/diff/340001/remoting/client/plugin... remoting/client/plugin/chromoting_instance.cc:1093: stats->video_packet_rate()->Rate(), kMaxFrameRate); is the max packet rate always same as frame rate? Should you have a different max?
Thanks Robert. PTAL at the latest patch-set. Sergey/Wez: ping. https://codereview.chromium.org/1181743005/diff/340001/remoting/client/plugin... File remoting/client/plugin/chromoting_instance.cc (right): https://codereview.chromium.org/1181743005/diff/340001/remoting/client/plugin... remoting/client/plugin/chromoting_instance.cc:77: // Histograms expect samples to be less than the boundary value, so set to 41. On 2015/07/10 17:41:40, rkaplow wrote: > 61 Done. https://codereview.chromium.org/1181743005/diff/340001/remoting/client/plugin... remoting/client/plugin/chromoting_instance.cc:81: // ranging from 0 to 10MB/s, spread across 1000 buckets. On 2015/07/10 17:41:40, rkaplow wrote: > I feel 1000 buckets is excessive. I suspect you should be fine with less, such > as 100. Note that since this is log scaled (as you mention) you will get more > precision for the smaller bucket sizes. Thanks. Done. https://codereview.chromium.org/1181743005/diff/340001/remoting/client/plugin... remoting/client/plugin/chromoting_instance.cc:1093: stats->video_packet_rate()->Rate(), kMaxFrameRate); On 2015/07/10 17:41:40, rkaplow wrote: > is the max packet rate always same as frame rate? Should you have a different > max? Good point. They may not always be the same: we expect some empty packets in the packet-rate which won't be counted in the frame-rate. Still, I think the same max should be OK. sergeyu@: WDYT?
On 2015/07/10 19:37:14, anandc wrote: > Thanks Robert. PTAL at the latest patch-set. > > Sergey/Wez: ping. > > https://codereview.chromium.org/1181743005/diff/340001/remoting/client/plugin... > File remoting/client/plugin/chromoting_instance.cc (right): > > https://codereview.chromium.org/1181743005/diff/340001/remoting/client/plugin... > remoting/client/plugin/chromoting_instance.cc:77: // Histograms expect samples > to be less than the boundary value, so set to 41. > On 2015/07/10 17:41:40, rkaplow wrote: > > 61 > > Done. > > https://codereview.chromium.org/1181743005/diff/340001/remoting/client/plugin... > remoting/client/plugin/chromoting_instance.cc:81: // ranging from 0 to 10MB/s, > spread across 1000 buckets. > On 2015/07/10 17:41:40, rkaplow wrote: > > I feel 1000 buckets is excessive. I suspect you should be fine with less, such > > as 100. Note that since this is log scaled (as you mention) you will get more > > precision for the smaller bucket sizes. > > Thanks. Done. > > https://codereview.chromium.org/1181743005/diff/340001/remoting/client/plugin... > remoting/client/plugin/chromoting_instance.cc:1093: > stats->video_packet_rate()->Rate(), kMaxFrameRate); > On 2015/07/10 17:41:40, rkaplow wrote: > > is the max packet rate always same as frame rate? Should you have a different > > max? > > Good point. > > They may not always be the same: we expect some empty packets in the packet-rate > which won't be counted in the frame-rate. > Still, I think the same max should be OK. > sergeyu@: WDYT? Ping.
rkaplow@chromium.org changed reviewers: + asvitkine@chromium.org
Generally looks good, but asking asviktine@ for review as well. One thing I am not sure about is using a Enumeration for their fps metric. the main UMA api uses UMA_HISTOGRAM_ENUMERATION for UMA_HISTOGRAM_PERCENTAGE, which is sort of similar, since they want linear buckets from 1 to 60. So I think the fps metric is ok, however other FPS I see within Chrome use regular log numerical metrics.
I think using enumeration macro for FPS is ok here - similar to the percentage precedent that Rob points out. https://codereview.chromium.org/1181743005/diff/360001/remoting/client/plugin... File remoting/client/plugin/chromoting_instance.cc (right): https://codereview.chromium.org/1181743005/diff/360001/remoting/client/plugin... remoting/client/plugin/chromoting_instance.cc:81: // ranging from 0 to 10MB/s, spread across 1000 buckets. Update comment re: # of buckets? https://codereview.chromium.org/1181743005/diff/360001/remoting/client/plugin... remoting/client/plugin/chromoting_instance.cc:1096: kBandwidthHistogramMax, kBandwidthHistogramNumBuckets); Nit: Indent is wrong. Maybe run "git cl format" https://codereview.chromium.org/1181743005/diff/360001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1181743005/diff/360001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:3447: + The bandwidth of non-empty packets in Chromoting remote sessions. For all of these, mention when / with what frequency they are logged.
still LGTM https://codereview.chromium.org/1181743005/diff/360001/remoting/client/plugin... File remoting/client/plugin/chromoting_instance.cc (right): https://codereview.chromium.org/1181743005/diff/360001/remoting/client/plugin... remoting/client/plugin/chromoting_instance.cc:87: const int kBandwidthHistogramMax = 10240000; nit: This should be either 10 MB = 10 * 1000 * 1000 B = 10000000 B or 10 MiB = 10 * 1024 * 1024 B = 10485760 B I'd prefer 10 MB, i.e. s/24/00/
Thanks Alexei. PTAL. https://codereview.chromium.org/1181743005/diff/360001/remoting/client/plugin... File remoting/client/plugin/chromoting_instance.cc (right): https://codereview.chromium.org/1181743005/diff/360001/remoting/client/plugin... remoting/client/plugin/chromoting_instance.cc:81: // ranging from 0 to 10MB/s, spread across 1000 buckets. On 2015/07/13 21:30:06, Alexei Svitkine wrote: > Update comment re: # of buckets? Done. https://codereview.chromium.org/1181743005/diff/360001/remoting/client/plugin... remoting/client/plugin/chromoting_instance.cc:87: const int kBandwidthHistogramMax = 10240000; On 2015/07/13 21:55:28, Sergey Ulanov wrote: > nit: This should be either > 10 MB = 10 * 1000 * 1000 B = 10000000 B > or > 10 MiB = 10 * 1024 * 1024 B = 10485760 B > > I'd prefer 10 MB, i.e. s/24/00/ Done. https://codereview.chromium.org/1181743005/diff/360001/remoting/client/plugin... remoting/client/plugin/chromoting_instance.cc:1096: kBandwidthHistogramMax, kBandwidthHistogramNumBuckets); On 2015/07/13 21:30:06, Alexei Svitkine wrote: > Nit: Indent is wrong. Maybe run "git cl format" Thanks. Done. Ran 'git cl format' and it made a fair number of edits. Going to do all of the style-fixes in a separate CL. https://codereview.chromium.org/1181743005/diff/360001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1181743005/diff/360001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:3447: + The bandwidth of non-empty packets in Chromoting remote sessions. On 2015/07/13 21:30:06, Alexei Svitkine wrote: > For all of these, mention when / with what frequency they are logged. Done.
lgtm https://codereview.chromium.org/1181743005/diff/380001/remoting/client/plugin... File remoting/client/plugin/chromoting_instance.cc (right): https://codereview.chromium.org/1181743005/diff/380001/remoting/client/plugin... remoting/client/plugin/chromoting_instance.cc:750: ChromotingStats::kStatsUpdateFrequencyInSeconds)); Nit: Indent 2 more. https://codereview.chromium.org/1181743005/diff/380001/remoting/client/plugin... remoting/client/plugin/chromoting_instance.cc:1071: ChromotingStats::kStatsUpdateFrequencyInSeconds)); Ditto.
https://codereview.chromium.org/1181743005/diff/360001/remoting/client/chromo... File remoting/client/chromoting_stats.h (right): https://codereview.chromium.org/1181743005/diff/360001/remoting/client/chromo... remoting/client/chromoting_stats.h:21: // plugin for the frequency at which stats should be updated. nit: Please put a blank line between comments and the preceding code, so that the comment is more clearly associated with the code below. https://codereview.chromium.org/1181743005/diff/380001/remoting/client/chromo... File remoting/client/chromoting_stats.h (right): https://codereview.chromium.org/1181743005/diff/380001/remoting/client/chromo... remoting/client/chromoting_stats.h:26: // Bytes/sec for non-empty video-packets. nit: Bytes per second https://codereview.chromium.org/1181743005/diff/380001/remoting/client/chromo... remoting/client/chromoting_stats.h:28: // frames/sec for non-empty video-packets. nit: Frames per second https://codereview.chromium.org/1181743005/diff/380001/remoting/client/chromo... remoting/client/chromoting_stats.h:30: // packets/sec, including empty video-packets. nit: Video packets per second You might also clarify that this is >= frames per second, since frames are each contained in a single packet. https://codereview.chromium.org/1181743005/diff/380001/remoting/client/chromo... remoting/client/chromoting_stats.h:35: // N is defined by kLatencySampleSize. Why not fix these to average over a fixed time, e.g. 1s, rather than a fixed number of samples? https://codereview.chromium.org/1181743005/diff/380001/remoting/client/plugin... File remoting/client/plugin/chromoting_instance.cc (right): https://codereview.chromium.org/1181743005/diff/380001/remoting/client/plugin... remoting/client/plugin/chromoting_instance.cc:78: const int kMaxFrameRate = 61; nit: Be explicit as to units, e.g. kMaxFramesPerSec? https://codereview.chromium.org/1181743005/diff/380001/remoting/client/plugin... remoting/client/plugin/chromoting_instance.cc:84: // Values above the maximum defined here end up in the right-most bucket. What does it mean for a bucket to be "right-most"? There doesn't seem to be any other mention of right or left in these comments, nor code? https://codereview.chromium.org/1181743005/diff/380001/remoting/client/plugin... remoting/client/plugin/chromoting_instance.cc:85: // See $/src/base/metrics/histogram.h for more details. Include the units that these constants are in as a suffix on the name, e.g. kBandwidthHistogramMinBps https://codereview.chromium.org/1181743005/diff/380001/remoting/client/plugin... remoting/client/plugin/chromoting_instance.cc:86: const int kBandwidthHistogramMin = 1; In the comment you state that the minimum is zero, not 1? https://codereview.chromium.org/1181743005/diff/380001/remoting/client/plugin... remoting/client/plugin/chromoting_instance.cc:87: const int kBandwidthHistogramMax = 10000000; Express this as 10 * 1000 * 1000 for clarity. Do you mean to use MegaByte (MB), versus MebiByte (MiB) here? https://codereview.chromium.org/1181743005/diff/380001/remoting/client/plugin... remoting/client/plugin/chromoting_instance.cc:88: const int kBandwidthHistogramNumBuckets = 100; nit: "Num" in the name adds nothing - we know it's a number, 'cos it's an int. :) https://codereview.chromium.org/1181743005/diff/380001/remoting/client/plugin... remoting/client/plugin/chromoting_instance.cc:745: // Start timers that periodically send perf stats. timers -> timer https://codereview.chromium.org/1181743005/diff/380001/remoting/client/plugin... remoting/client/plugin/chromoting_instance.cc:750: ChromotingStats::kStatsUpdateFrequencyInSeconds)); nit: Indentation - have you run git cl format on this CL? https://codereview.chromium.org/1181743005/diff/380001/remoting/client/plugin... remoting/client/plugin/chromoting_instance.cc:1071: ChromotingStats::kStatsUpdateFrequencyInSeconds)); nit: Indentation. https://codereview.chromium.org/1181743005/diff/380001/remoting/client/plugin... remoting/client/plugin/chromoting_instance.cc:1073: // Update performance stats and send them to the client for display to users. nit: This doesn't update the stats, it just fetches them from the VideoRenderer and marshals them to post to the app. https://codereview.chromium.org/1181743005/diff/380001/remoting/client/plugin... remoting/client/plugin/chromoting_instance.cc:1075: // over the 10 most recent samples. This is already documented in ChromotingStats - don't duplicate it here. https://codereview.chromium.org/1181743005/diff/380001/remoting/client/plugin... remoting/client/plugin/chromoting_instance.cc:1088: // averaged over 1s. nit: Record the video frame-rate, ... to UMA. Again, no need to describe the averaging here, since that's described in ChromotingStats. https://codereview.chromium.org/1181743005/diff/380001/remoting/client/plugin... File remoting/client/plugin/pepper_video_renderer_3d.cc (right): https://codereview.chromium.org/1181743005/diff/380001/remoting/client/plugin... remoting/client/plugin/pepper_video_renderer_3d.cc:192: stats_.video_packet_rate()->Record(1); nit: Leave a blank line between this code and the comment on the next block. I'd recommend explicitly commenting that this needs Record()ing _before_ the early-return, since it needs to include empty packets. https://codereview.chromium.org/1181743005/diff/380001/remoting/client/plugin... remoting/client/plugin/pepper_video_renderer_3d.cc:200: // software_video_renderer.cc. Move to ChromotingStats: http://crbug/508602 nit: Suggest reducing to "TODO(...): Move this work into ChromotingStats - see crbug..." https://codereview.chromium.org/1181743005/diff/380001/remoting/client/softwa... File remoting/client/software_video_renderer.cc (right): https://codereview.chromium.org/1181743005/diff/380001/remoting/client/softwa... remoting/client/software_video_renderer.cc:353: stats_.video_packet_rate()->Record(1); See comments in the Pepper 3D renderer re layout & comments. https://codereview.chromium.org/1181743005/diff/380001/remoting/client/softwa... remoting/client/software_video_renderer.cc:357: done.Run(); nit: We should be doing done.Run() here and below with a ScopedTaskRunner... something for the refactoring CL. https://codereview.chromium.org/1181743005/diff/380001/remoting/client/softwa... remoting/client/software_video_renderer.cc:361: // Add one frame to the video frame rate counters. You're only updating a single counter here, though? https://codereview.chromium.org/1181743005/diff/380001/remoting/client/softwa... remoting/client/software_video_renderer.cc:362: stats_.video_bandwidth()->Record(1); Looks like you've mistakenly swapped bandwidth & frame rate? https://codereview.chromium.org/1181743005/diff/380001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1181743005/diff/380001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:3445: + <owner>anandc@chromium.org</owner> What does it mean to "own" a histogram? Should we add e.g. Joe here as well?
Thanks Alexei and Wez. PTAL at PS#12. https://codereview.chromium.org/1181743005/diff/360001/remoting/client/chromo... File remoting/client/chromoting_stats.h (right): https://codereview.chromium.org/1181743005/diff/360001/remoting/client/chromo... remoting/client/chromoting_stats.h:21: // plugin for the frequency at which stats should be updated. On 2015/07/14 20:24:15, Wez wrote: > nit: Please put a blank line between comments and the preceding code, so that > the comment is more clearly associated with the code below. Done. https://codereview.chromium.org/1181743005/diff/380001/remoting/client/chromo... File remoting/client/chromoting_stats.h (right): https://codereview.chromium.org/1181743005/diff/380001/remoting/client/chromo... remoting/client/chromoting_stats.h:26: // Bytes/sec for non-empty video-packets. On 2015/07/14 20:24:16, Wez wrote: > nit: Bytes per second Done. https://codereview.chromium.org/1181743005/diff/380001/remoting/client/chromo... remoting/client/chromoting_stats.h:28: // frames/sec for non-empty video-packets. On 2015/07/14 20:24:15, Wez wrote: > nit: Frames per second Done. https://codereview.chromium.org/1181743005/diff/380001/remoting/client/chromo... remoting/client/chromoting_stats.h:30: // packets/sec, including empty video-packets. On 2015/07/14 20:24:16, Wez wrote: > nit: Video packets per second > > You might also clarify that this is >= frames per second, since frames are each > contained in a single packet. Done. https://codereview.chromium.org/1181743005/diff/380001/remoting/client/chromo... remoting/client/chromoting_stats.h:35: // N is defined by kLatencySampleSize. On 2015/07/14 20:24:15, Wez wrote: > Why not fix these to average over a fixed time, e.g. 1s, rather than a fixed > number of samples? Good suggestion. Here are 2 ways to do that: change these to be rate counters, in which case the RateCounter class should be updated to include a way to return an average for the number of samples recorded over its specific time-window. Or, keep this is a running-average, with a large enough sample size, say 60 or 75, so that we deal with cases where there is a burst of packets. Option #1 sounds better. Would you be OK with doing that in the follow-up CL planned for the refactoring, https://code.google.com/p/chromium/issues/detail?id=508602? https://codereview.chromium.org/1181743005/diff/380001/remoting/client/plugin... File remoting/client/plugin/chromoting_instance.cc (right): https://codereview.chromium.org/1181743005/diff/380001/remoting/client/plugin... remoting/client/plugin/chromoting_instance.cc:78: const int kMaxFrameRate = 61; On 2015/07/14 20:24:17, Wez wrote: > nit: Be explicit as to units, e.g. kMaxFramesPerSec? Done. https://codereview.chromium.org/1181743005/diff/380001/remoting/client/plugin... remoting/client/plugin/chromoting_instance.cc:84: // Values above the maximum defined here end up in the right-most bucket. On 2015/07/14 20:24:16, Wez wrote: > What does it mean for a bucket to be "right-most"? There doesn't seem to be any > other mention of right or left in these comments, nor code? Yes, that is unclear. The bucket with max values could be right-most or bottom-most depending on how it is graphed. Corrected to say max-bucket. https://codereview.chromium.org/1181743005/diff/380001/remoting/client/plugin... remoting/client/plugin/chromoting_instance.cc:85: // See $/src/base/metrics/histogram.h for more details. On 2015/07/14 20:24:16, Wez wrote: > Include the units that these constants are in as a suffix on the name, e.g. > kBandwidthHistogramMinBps Done. https://codereview.chromium.org/1181743005/diff/380001/remoting/client/plugin... remoting/client/plugin/chromoting_instance.cc:86: const int kBandwidthHistogramMin = 1; On 2015/07/14 20:24:17, Wez wrote: > In the comment you state that the minimum is zero, not 1? UMA normalizes a histogram's minimum to 1 if it is < 1. Added a clarification. https://codereview.chromium.org/1181743005/diff/380001/remoting/client/plugin... remoting/client/plugin/chromoting_instance.cc:87: const int kBandwidthHistogramMax = 10000000; On 2015/07/14 20:24:17, Wez wrote: > Express this as 10 * 1000 * 1000 for clarity. > > Do you mean to use MegaByte (MB), versus MebiByte (MiB) here? MegaByte. https://codereview.chromium.org/1181743005/diff/380001/remoting/client/plugin... remoting/client/plugin/chromoting_instance.cc:88: const int kBandwidthHistogramNumBuckets = 100; On 2015/07/14 20:24:16, Wez wrote: > nit: "Num" in the name adds nothing - we know it's a number, 'cos it's an int. > :) Done. https://codereview.chromium.org/1181743005/diff/380001/remoting/client/plugin... remoting/client/plugin/chromoting_instance.cc:745: // Start timers that periodically send perf stats. On 2015/07/14 20:24:17, Wez wrote: > timers -> timer Done. https://codereview.chromium.org/1181743005/diff/380001/remoting/client/plugin... remoting/client/plugin/chromoting_instance.cc:750: ChromotingStats::kStatsUpdateFrequencyInSeconds)); On 2015/07/14 20:24:17, Wez wrote: > nit: Indentation - have you run git cl format on this CL? Sorry, I was using clang-format and it was raising a lot more style-changes than just git cl format. Ran git cl format now, and fixed. Thanks. https://codereview.chromium.org/1181743005/diff/380001/remoting/client/plugin... remoting/client/plugin/chromoting_instance.cc:750: ChromotingStats::kStatsUpdateFrequencyInSeconds)); On 2015/07/14 20:16:33, Alexei Svitkine wrote: > Nit: Indent 2 more. Acknowledged. https://codereview.chromium.org/1181743005/diff/380001/remoting/client/plugin... remoting/client/plugin/chromoting_instance.cc:1071: ChromotingStats::kStatsUpdateFrequencyInSeconds)); On 2015/07/14 20:24:17, Wez wrote: > nit: Indentation. Done. https://codereview.chromium.org/1181743005/diff/380001/remoting/client/plugin... remoting/client/plugin/chromoting_instance.cc:1071: ChromotingStats::kStatsUpdateFrequencyInSeconds)); On 2015/07/14 20:16:33, Alexei Svitkine wrote: > Ditto. Acknowledged. https://codereview.chromium.org/1181743005/diff/380001/remoting/client/plugin... remoting/client/plugin/chromoting_instance.cc:1073: // Update performance stats and send them to the client for display to users. On 2015/07/14 20:24:17, Wez wrote: > nit: This doesn't update the stats, it just fetches them from the VideoRenderer > and marshals them to post to the app. Done. https://codereview.chromium.org/1181743005/diff/380001/remoting/client/plugin... remoting/client/plugin/chromoting_instance.cc:1075: // over the 10 most recent samples. On 2015/07/14 20:24:17, Wez wrote: > This is already documented in ChromotingStats - don't duplicate it here. Done. https://codereview.chromium.org/1181743005/diff/380001/remoting/client/plugin... remoting/client/plugin/chromoting_instance.cc:1088: // averaged over 1s. On 2015/07/14 20:24:16, Wez wrote: > nit: Record the video frame-rate, ... to UMA. > > Again, no need to describe the averaging here, since that's described in > ChromotingStats. Done. https://codereview.chromium.org/1181743005/diff/380001/remoting/client/plugin... File remoting/client/plugin/pepper_video_renderer_3d.cc (right): https://codereview.chromium.org/1181743005/diff/380001/remoting/client/plugin... remoting/client/plugin/pepper_video_renderer_3d.cc:192: stats_.video_packet_rate()->Record(1); On 2015/07/14 20:24:17, Wez wrote: > nit: Leave a blank line between this code and the comment on the next block. > > I'd recommend explicitly commenting that this needs Record()ing _before_ the > early-return, since it needs to include empty packets. Done. https://codereview.chromium.org/1181743005/diff/380001/remoting/client/plugin... remoting/client/plugin/pepper_video_renderer_3d.cc:200: // software_video_renderer.cc. Move to ChromotingStats: http://crbug/508602 On 2015/07/14 20:24:17, Wez wrote: > nit: Suggest reducing to "TODO(...): Move this work into ChromotingStats - see > crbug..." Done. https://codereview.chromium.org/1181743005/diff/380001/remoting/client/softwa... File remoting/client/software_video_renderer.cc (right): https://codereview.chromium.org/1181743005/diff/380001/remoting/client/softwa... remoting/client/software_video_renderer.cc:353: stats_.video_packet_rate()->Record(1); On 2015/07/14 20:24:17, Wez wrote: > See comments in the Pepper 3D renderer re layout & comments. Done. https://codereview.chromium.org/1181743005/diff/380001/remoting/client/softwa... remoting/client/software_video_renderer.cc:357: done.Run(); On 2015/07/14 20:24:17, Wez wrote: > nit: We should be doing done.Run() here and below with a ScopedTaskRunner... > something for the refactoring CL. Added note to tracking bug. https://codereview.chromium.org/1181743005/diff/380001/remoting/client/softwa... remoting/client/software_video_renderer.cc:361: // Add one frame to the video frame rate counters. On 2015/07/14 20:24:17, Wez wrote: > You're only updating a single counter here, though? Done. Holdover from a previous patch-set. Thanks for catching. https://codereview.chromium.org/1181743005/diff/380001/remoting/client/softwa... remoting/client/software_video_renderer.cc:362: stats_.video_bandwidth()->Record(1); On 2015/07/14 20:24:17, Wez wrote: > Looks like you've mistakenly swapped bandwidth & frame rate? Aargh, and didn't catch it even with a diff. Thanks a lot. Done. https://codereview.chromium.org/1181743005/diff/380001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1181743005/diff/380001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:3445: + <owner>anandc@chromium.org</owner> On 2015/07/14 20:24:17, Wez wrote: > What does it mean to "own" a histogram? Should we add e.g. Joe here as well? I'm not entirely sure. asvitkine@/rkaplow@, could you explain? I've added sergeyu@ for now as owner.
Ping.
lgtm https://codereview.chromium.org/1181743005/diff/380001/remoting/client/chromo... File remoting/client/chromoting_stats.h (right): https://codereview.chromium.org/1181743005/diff/380001/remoting/client/chromo... remoting/client/chromoting_stats.h:35: // N is defined by kLatencySampleSize. On 2015/07/15 03:59:54, anandc wrote: > On 2015/07/14 20:24:15, Wez wrote: > > Why not fix these to average over a fixed time, e.g. 1s, rather than a fixed > > number of samples? > > Good suggestion. Here are 2 ways to do that: > change these to be rate counters, in which case the RateCounter class should be > updated to include a way to return an average for the number of samples recorded > over its specific time-window. > Or, keep this is a running-average, with a large enough sample size, say 60 or > 75, so that we deal with cases where there is a burst of packets. > > Option #1 sounds better. > Would you be OK with doing that in the follow-up CL planned for the refactoring, > https://code.google.com/p/chromium/issues/detail?id=508602? Doing it in the follow-up SGTM. I'd prefer that we just update the RunningAverage class to average over a time window rather than over a fixed number of samples. Note that the RunningAverage class could be made to average over the entire period between samples, rather than averaging over a fixed window, so that each call to sample it is implicitly looking at the time period since the preceding sample. https://codereview.chromium.org/1181743005/diff/380001/remoting/client/plugin... File remoting/client/plugin/chromoting_instance.cc (right): https://codereview.chromium.org/1181743005/diff/380001/remoting/client/plugin... remoting/client/plugin/chromoting_instance.cc:84: // Values above the maximum defined here end up in the right-most bucket. On 2015/07/15 03:59:55, anandc wrote: > On 2015/07/14 20:24:16, Wez wrote: > > What does it mean for a bucket to be "right-most"? There doesn't seem to be > any > > other mention of right or left in these comments, nor code? > > Yes, that is unclear. The bucket with max values could be right-most or > bottom-most depending on how it is graphed. > Corrected to say max-bucket. Acknowledged. https://codereview.chromium.org/1181743005/diff/380001/remoting/client/plugin... remoting/client/plugin/chromoting_instance.cc:750: ChromotingStats::kStatsUpdateFrequencyInSeconds)); On 2015/07/15 03:59:54, anandc wrote: > On 2015/07/14 20:24:17, Wez wrote: > > nit: Indentation - have you run git cl format on this CL? > > > Sorry, I was using clang-format and it was raising a lot more style-changes than > just git cl format. > Ran git cl format now, and fixed. Thanks. Hmmm, if clang-format has more complaints then can you email me a sample of what it's upset about - perhaps we can clean that up as a follow-up. https://codereview.chromium.org/1181743005/diff/400001/remoting/client/plugin... File remoting/client/plugin/chromoting_instance.cc (right): https://codereview.chromium.org/1181743005/diff/400001/remoting/client/plugin... remoting/client/plugin/chromoting_instance.cc:87: // normalized to 1. So we'll set the minimum to 1. Why not set it to zero and let UMA "implicitly normalize" it? Why is it doing that implicitly anyway? 0 Bps is a perfectly valid real-world value...
https://codereview.chromium.org/1181743005/diff/380001/remoting/client/chromo... File remoting/client/chromoting_stats.h (right): https://codereview.chromium.org/1181743005/diff/380001/remoting/client/chromo... remoting/client/chromoting_stats.h:35: // N is defined by kLatencySampleSize. On 2015/07/16 19:03:35, Wez wrote: > On 2015/07/15 03:59:54, anandc wrote: > > On 2015/07/14 20:24:15, Wez wrote: > > > Why not fix these to average over a fixed time, e.g. 1s, rather than a fixed > > > number of samples? > > > > Good suggestion. Here are 2 ways to do that: > > change these to be rate counters, in which case the RateCounter class should > be > > updated to include a way to return an average for the number of samples > recorded > > over its specific time-window. > > Or, keep this is a running-average, with a large enough sample size, say 60 or > > 75, so that we deal with cases where there is a burst of packets. > > > > Option #1 sounds better. > > Would you be OK with doing that in the follow-up CL planned for the > refactoring, > > https://code.google.com/p/chromium/issues/detail?id=508602? > > Doing it in the follow-up SGTM. > > I'd prefer that we just update the RunningAverage class to average over a time > window rather than over a fixed number of samples. > > Note that the RunningAverage class could be made to average over the entire > period between samples, rather than averaging over a fixed window, so that each > call to sample it is implicitly looking at the time period since the preceding > sample. Yes, that would work. Thanks. https://codereview.chromium.org/1181743005/diff/380001/remoting/client/plugin... File remoting/client/plugin/chromoting_instance.cc (right): https://codereview.chromium.org/1181743005/diff/380001/remoting/client/plugin... remoting/client/plugin/chromoting_instance.cc:750: ChromotingStats::kStatsUpdateFrequencyInSeconds)); On 2015/07/16 19:03:36, Wez wrote: > On 2015/07/15 03:59:54, anandc wrote: > > On 2015/07/14 20:24:17, Wez wrote: > > > nit: Indentation - have you run git cl format on this CL? > > > > > > Sorry, I was using clang-format and it was raising a lot more style-changes > than > > just git cl format. > > Ran git cl format now, and fixed. Thanks. > > Hmmm, if clang-format has more complaints then can you email me a sample of what > it's upset about - perhaps we can clean that up as a follow-up. Yes, I'm planning to do that in a separate CL (already spoke to sergeyu@ about it). https://codereview.chromium.org/1181743005/diff/400001/remoting/client/plugin... File remoting/client/plugin/chromoting_instance.cc (right): https://codereview.chromium.org/1181743005/diff/400001/remoting/client/plugin... remoting/client/plugin/chromoting_instance.cc:87: // normalized to 1. So we'll set the minimum to 1. On 2015/07/16 19:03:36, Wez wrote: > Why not set it to zero and let UMA "implicitly normalize" it? Why is it doing > that implicitly anyway? 0 Bps is a perfectly valid real-world value... Done. asvitkine@: do you know why UMA normalizes histogram minimums < 1 to 1?
The CQ bit was checked by anandc@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sergeyu@chromium.org, asvitkine@chromium.org, wez@chromium.org Link to the patchset: https://codereview.chromium.org/1181743005/#ps420001 (title: "Set bandwidth histogram minimum to 0.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1181743005/420001
https://codereview.chromium.org/1181743005/diff/400001/remoting/client/plugin... File remoting/client/plugin/chromoting_instance.cc (right): https://codereview.chromium.org/1181743005/diff/400001/remoting/client/plugin... remoting/client/plugin/chromoting_instance.cc:87: // normalized to 1. So we'll set the minimum to 1. On 2015/07/16 20:18:48, anandc wrote: > On 2015/07/16 19:03:36, Wez wrote: > > Why not set it to zero and let UMA "implicitly normalize" it? Why is it doing > > that implicitly anyway? 0 Bps is a perfectly valid real-world value... > > Done. > asvitkine@: do you know why UMA normalizes histogram minimums < 1 to 1? I think the comment is a bit confusing. It's not that it normalizes it to 1. It's that the way it works, is there's always an underflow bucket for values below min_value, which starts at 0. So if you set min to 1, then you'll have a 0-1 bucket already provided. You can kind of think of it as a quirk about the histograms API.
https://codereview.chromium.org/1181743005/diff/380001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1181743005/diff/380001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:3445: + <owner>anandc@chromium.org</owner> On 2015/07/15 03:59:55, anandc wrote: > On 2015/07/14 20:24:17, Wez wrote: > > What does it mean to "own" a histogram? Should we add e.g. Joe here as well? > > I'm not entirely sure. asvitkine@/rkaplow@, could you explain? > > I've added sergeyu@ for now as owner. Sorry, missed this one. It basically means that if someone has a question about this histogram - that you're the right person to ask. (And establishes that this histogram is something that is being used - e.g. as opposed to some legacy histograms we have that don't have an owner.)
Message was sent while issue was closed.
Committed patchset #13 (id:420001)
Message was sent while issue was closed.
Patchset 13 (id:??) landed as https://crrev.com/b877083e221c58a67a9a00e2024050f069948c6d Cr-Commit-Position: refs/heads/master@{#339136} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
