Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
344613003:
Record GCM received data message intervals in UMA (Closed)
DescriptionRecord GCM received data message intervals in UMA.
BUG=386273
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=283847
Patch Set 1 #
Total comments: 8
Patch Set 2 : Addressed Jian's and Filip's comments #
Total comments: 12
Patch Set 3 : Addressed Filip's comments. #Patch Set 4 : Fixed format issues. #
Total comments: 1
Messages
Total messages: 18 (0 generated)
https://codereview.chromium.org/344613003/diff/1/components/gcm_driver/gcm_st... File components/gcm_driver/gcm_stats_recorder_impl.cc (right): https://codereview.chromium.org/344613003/diff/1/components/gcm_driver/gcm_st... components/gcm_driver/gcm_stats_recorder_impl.cc:369: if (last_data_message_received_timestamp_ > 0) { will this trigger only after all of the messages that arrive during reconnect are consumed, or after the first one? We should let all of them pass, and start recording after a second (?) after connection was made. https://codereview.chromium.org/344613003/diff/1/components/gcm_driver/gcm_st... components/gcm_driver/gcm_stats_recorder_impl.cc:370: UMA_HISTOGRAM_COUNTS("GCM.DataMessageReceivedIntervalSeconds", please update histograms as well.
https://codereview.chromium.org/344613003/diff/1/components/gcm_driver/gcm_st... File components/gcm_driver/gcm_stats_recorder_impl.cc (right): https://codereview.chromium.org/344613003/diff/1/components/gcm_driver/gcm_st... components/gcm_driver/gcm_stats_recorder_impl.cc:368: int64 new_timestamp = clock_->Now().ToTimeT(); Why not calling Time::Now()?
Update on the review. I think if you update the if statement this is exactly what we need. https://codereview.chromium.org/344613003/diff/1/components/gcm_driver/gcm_st... File components/gcm_driver/gcm_stats_recorder_impl.cc (right): https://codereview.chromium.org/344613003/diff/1/components/gcm_driver/gcm_st... components/gcm_driver/gcm_stats_recorder_impl.cc:369: if (last_data_message_received_timestamp_ > 0) { On 2014/06/18 20:06:19, fgorski wrote: > will this trigger only after all of the messages that arrive during reconnect > are consumed, or after the first one? > > We should let all of them pass, and start recording after a second (?) after > connection was made. Ju-Yi, given that the wake-up period during sleep is supposed to be 2 sec., why don't we discard all of the 2 second bursts. We can update the name of the enum to reflect that.
https://codereview.chromium.org/344613003/diff/1/components/gcm_driver/gcm_st... File components/gcm_driver/gcm_stats_recorder_impl.cc (right): https://codereview.chromium.org/344613003/diff/1/components/gcm_driver/gcm_st... components/gcm_driver/gcm_stats_recorder_impl.cc:368: int64 new_timestamp = clock_->Now().ToTimeT(); On 2014/06/19 17:24:06, jianli wrote: > Why not calling Time::Now()? Done. https://codereview.chromium.org/344613003/diff/1/components/gcm_driver/gcm_st... components/gcm_driver/gcm_stats_recorder_impl.cc:369: if (last_data_message_received_timestamp_ > 0) { On 2014/06/18 20:06:19, fgorski wrote: > will this trigger only after all of the messages that arrive during reconnect > are consumed, or after the first one? This is triggered every time a data message is consumed. So when last_data_message_received_timestamp_ is zero, it means it is the first time we've seen received message so there is no interval to think about. > > We should let all of them pass, and start recording after a second (?) after > connection was made. https://codereview.chromium.org/344613003/diff/1/components/gcm_driver/gcm_st... components/gcm_driver/gcm_stats_recorder_impl.cc:369: if (last_data_message_received_timestamp_ > 0) { On 2014/06/20 14:23:45, fgorski wrote: > On 2014/06/18 20:06:19, fgorski wrote: > > will this trigger only after all of the messages that arrive during reconnect > > are consumed, or after the first one? > > > > We should let all of them pass, and start recording after a second (?) after > > connection was made. > > Ju-Yi, given that the wake-up period during sleep is supposed to be 2 sec., why > don't we discard all of the 2 second bursts. We can update the name of the enum > to reflect that. Done. Collapsed all received data message within 2 seconds into 1 count. Changed the UMA name as well. So when reconnection happens, we will receive a whole bunch of data messages within 2 seconds and they count for 1 burst. https://codereview.chromium.org/344613003/diff/1/components/gcm_driver/gcm_st... components/gcm_driver/gcm_stats_recorder_impl.cc:370: UMA_HISTOGRAM_COUNTS("GCM.DataMessageReceivedIntervalSeconds", On 2014/06/18 20:06:19, fgorski wrote: > please update histograms as well. Updated the name.
Logic is OK, but I'd use base::Time and base::TimeDelta to ensure precision. https://codereview.chromium.org/344613003/diff/20001/components/gcm_driver/gc... File components/gcm_driver/gcm_stats_recorder_impl.cc (right): https://codereview.chromium.org/344613003/diff/20001/components/gcm_driver/gc... components/gcm_driver/gcm_stats_recorder_impl.cc:19: const int64 RECEIVED_DATA_MESSAGE_BUSRT_SECONDS = 2; s/BUSRT/BURST/ s/BURST/BURST_LENGTH/ https://codereview.chromium.org/344613003/diff/20001/components/gcm_driver/gc... components/gcm_driver/gcm_stats_recorder_impl.cc:368: int64 new_timestamp = base::Time::Now().ToTimeT(); use base::Time https://codereview.chromium.org/344613003/diff/20001/components/gcm_driver/gc... components/gcm_driver/gcm_stats_recorder_impl.cc:371: } else if ((new_timestamp - last_data_message_received_timestamp_) >= new_timestamp - last_data_message_received_timestamp_ > base::TimeDelta::FromSeconds(RECEIVED_DATA_MESSAGE_BURST_SECONDS) https://codereview.chromium.org/344613003/diff/20001/components/gcm_driver/gc... components/gcm_driver/gcm_stats_recorder_impl.cc:374: new_timestamp - last_data_message_received_timestamp_); (new_timestamp - last_data_message_received_timestamp_).InSeconds() https://codereview.chromium.org/344613003/diff/20001/components/gcm_driver/gc... File components/gcm_driver/gcm_stats_recorder_impl.h (right): https://codereview.chromium.org/344613003/diff/20001/components/gcm_driver/gc... components/gcm_driver/gcm_stats_recorder_impl.h:150: int64 last_data_message_received_timestamp_; use base::Time Consider renaming to: last_data_message_burst_start_time_
Found one more issue. https://codereview.chromium.org/344613003/diff/20001/components/gcm_driver/gc... File components/gcm_driver/gcm_stats_recorder_impl.cc (right): https://codereview.chromium.org/344613003/diff/20001/components/gcm_driver/gc... components/gcm_driver/gcm_stats_recorder_impl.cc:374: new_timestamp - last_data_message_received_timestamp_); One more thing, Remember to set last_data_message_received_timestamp_ = new_timestamp, as you detected new burst.
https://codereview.chromium.org/344613003/diff/20001/components/gcm_driver/gc... File components/gcm_driver/gcm_stats_recorder_impl.cc (right): https://codereview.chromium.org/344613003/diff/20001/components/gcm_driver/gc... components/gcm_driver/gcm_stats_recorder_impl.cc:19: const int64 RECEIVED_DATA_MESSAGE_BUSRT_SECONDS = 2; On 2014/07/16 04:11:11, fgorski wrote: > s/BUSRT/BURST/ > s/BURST/BURST_LENGTH/ > > Done. https://codereview.chromium.org/344613003/diff/20001/components/gcm_driver/gc... components/gcm_driver/gcm_stats_recorder_impl.cc:368: int64 new_timestamp = base::Time::Now().ToTimeT(); On 2014/07/16 04:11:11, fgorski wrote: > use base::Time Done. https://codereview.chromium.org/344613003/diff/20001/components/gcm_driver/gc... components/gcm_driver/gcm_stats_recorder_impl.cc:371: } else if ((new_timestamp - last_data_message_received_timestamp_) >= On 2014/07/16 04:11:11, fgorski wrote: > new_timestamp - last_data_message_received_timestamp_ > > base::TimeDelta::FromSeconds(RECEIVED_DATA_MESSAGE_BURST_SECONDS) Done. https://codereview.chromium.org/344613003/diff/20001/components/gcm_driver/gc... components/gcm_driver/gcm_stats_recorder_impl.cc:374: new_timestamp - last_data_message_received_timestamp_); On 2014/07/16 04:32:24, fgorski wrote: > One more thing, > Remember to set last_data_message_received_timestamp_ = new_timestamp, as you > detected new burst. Done. https://codereview.chromium.org/344613003/diff/20001/components/gcm_driver/gc... components/gcm_driver/gcm_stats_recorder_impl.cc:374: new_timestamp - last_data_message_received_timestamp_); On 2014/07/16 04:11:11, fgorski wrote: > (new_timestamp - last_data_message_received_timestamp_).InSeconds() Done. https://codereview.chromium.org/344613003/diff/20001/components/gcm_driver/gc... File components/gcm_driver/gcm_stats_recorder_impl.h (right): https://codereview.chromium.org/344613003/diff/20001/components/gcm_driver/gc... components/gcm_driver/gcm_stats_recorder_impl.h:150: int64 last_data_message_received_timestamp_; On 2014/07/16 04:11:11, fgorski wrote: > use base::Time > > Consider renaming to: last_data_message_burst_start_time_ Done.
lgtm
The CQ bit was checked by juyik@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/juyik@chromium.org/344613003/60001
https://codereview.chromium.org/344613003/diff/60001/components/gcm_driver/gc... File components/gcm_driver/gcm_stats_recorder_impl.cc (right): https://codereview.chromium.org/344613003/diff/60001/components/gcm_driver/gc... components/gcm_driver/gcm_stats_recorder_impl.cc:384: to_registered_app ? std::string() : This isn't a part of this patch but don't we already know that to_registered_app is false here because of the if statement? We shouldn't need to check it again.
True. I will fix this in another patch. On Wed, Jul 16, 2014 at 4:49 PM, <chirantan@chromium.org> wrote: > > https://codereview.chromium.org/344613003/diff/60001/ > components/gcm_driver/gcm_stats_recorder_impl.cc > File components/gcm_driver/gcm_stats_recorder_impl.cc (right): > > https://codereview.chromium.org/344613003/diff/60001/ > components/gcm_driver/gcm_stats_recorder_impl.cc#newcode384 > components/gcm_driver/gcm_stats_recorder_impl.cc:384: to_registered_app > ? std::string() : > This isn't a part of this patch but don't we already know that > to_registered_app is false here because of the if statement? We > shouldn't need to check it again. > > https://codereview.chromium.org/344613003/ > -- Thanks, Ju-Yi To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2014/07/16 23:58:31, chromium-reviews wrote: > True. I will fix this in another patch. > > > On Wed, Jul 16, 2014 at 4:49 PM, <mailto:chirantan@chromium.org> wrote: > > > > > https://codereview.chromium.org/344613003/diff/60001/ > > components/gcm_driver/gcm_stats_recorder_impl.cc > > File components/gcm_driver/gcm_stats_recorder_impl.cc (right): > > > > https://codereview.chromium.org/344613003/diff/60001/ > > components/gcm_driver/gcm_stats_recorder_impl.cc#newcode384 > > components/gcm_driver/gcm_stats_recorder_impl.cc:384: to_registered_app > > ? std::string() : > > This isn't a part of this patch but don't we already know that > > to_registered_app is false here because of the if statement? We > > shouldn't need to check it again. > > > > https://codereview.chromium.org/344613003/ > > > > > > -- > Thanks, > > Ju-Yi > > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org. Hey, Ju-Yi, Looks like the patch does not include histograms.
The CQ bit was unchecked by juyik@chromium.org
The CQ bit was checked by juyik@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/juyik@chromium.org/344613003/60001
Message was sent while issue was closed.
Change committed as 283847 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
