Record connection, registration, and receiving activities.

Implement connection, registration, and receiving activities recording on the google apis side, for gcm-internals page.

arv: please do owner review for chrome/browser/ui/webui/*

BUG=341256
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=267008

[jianli, 2014-04-23 17:22:34]

How about unregistration?

https://codereview.chromium.org/248213004/diff/20001/google_apis/gcm/engine/c...
File google_apis/gcm/engine/connection_factory_impl.cc (right):

https://codereview.chromium.org/248213004/diff/20001/google_apis/gcm/engine/c...
google_apis/gcm/engine/connection_factory_impl.cc:242: GURL cur_endpoint =
GetCurrentEndpoint();
nit: avoid using abbreviation if possible. so name it as current_endpoint

https://codereview.chromium.org/248213004/diff/20001/google_apis/gcm/gcm_clie...
File google_apis/gcm/gcm_client_impl.cc (right):

https://codereview.chromium.org/248213004/diff/20001/google_apis/gcm/gcm_clie...
google_apis/gcm/gcm_client_impl.cc:715: recorder_.RecordDataRecieved(app_id,
data_message_stanza.from(),
You can combine both recording into one by introducing a bool flag to track if
the message should be dropped, doing the recording, and then checking for the
dropping.

https://codereview.chromium.org/248213004/diff/20001/google_apis/gcm/monitori...
File google_apis/gcm/monitoring/gcm_stats_recorder.cc (right):

https://codereview.chromium.org/248213004/diff/20001/google_apis/gcm/monitori...
google_apis/gcm/monitoring/gcm_stats_recorder.cc:131:
GCMStatsRecorder::ReceivingActivity::ReceivingActivity() :
nit: ":" should be placed at the next line.

https://codereview.chromium.org/248213004/diff/20001/google_apis/gcm/monitori...
google_apis/gcm/monitoring/gcm_stats_recorder.cc:169: if (is_recording_) {
nit: brackets not needed

https://codereview.chromium.org/248213004/diff/20001/google_apis/gcm/monitori...
File google_apis/gcm/monitoring/gcm_stats_recorder.h (right):

https://codereview.chromium.org/248213004/diff/20001/google_apis/gcm/monitori...
google_apis/gcm/monitoring/gcm_stats_recorder.h:41: 
nit: remove empty line

https://codereview.chromium.org/248213004/diff/20001/google_apis/gcm/monitori...
google_apis/gcm/monitoring/gcm_stats_recorder.h:49: std::string android_id;
Is this needed? Android id is one per profile.

https://codereview.chromium.org/248213004/diff/20001/google_apis/gcm/monitori...
google_apis/gcm/monitoring/gcm_stats_recorder.h:103: void
RecordRegistrationSent(const std::string& android_id,
No need to record android_id.

https://codereview.chromium.org/248213004/diff/20001/google_apis/gcm/monitori...
google_apis/gcm/monitoring/gcm_stats_recorder.h:108: void
RecordRegistrationResponse(const std::string& android_id,
ditto for android_id.

https://codereview.chromium.org/248213004/diff/20001/google_apis/gcm/monitori...
google_apis/gcm/monitoring/gcm_stats_recorder.h:115: const std::string&
android_id,
ditto for android_id.

https://codereview.chromium.org/248213004/diff/20001/google_apis/gcm/monitori...
google_apis/gcm/monitoring/gcm_stats_recorder.h:121: void
RecordDataRecieved(const std::string& app_id,
RecordDataMessageReceived

https://codereview.chromium.org/248213004/diff/20001/google_apis/gcm/monitori...
google_apis/gcm/monitoring/gcm_stats_recorder.h:124: bool to_registered_app);
dropped?

https://codereview.chromium.org/248213004/diff/20001/google_apis/gcm/monitori...
google_apis/gcm/monitoring/gcm_stats_recorder.h:127: void
RecordDataDeletedMessage(const std::string& app_id,
RecordMessagesDeletedReceived

[jianli, 2014-04-23 17:55:58]

For unregistration, it is fine to tackle it in a separate patch.


On Wed, Apr 23, 2014 at 10:22 AM, <jianli@chromium.org> wrote:

> How about unregistration?
>
>
> https://codereview.chromium.org/248213004/diff/20001/
> google_apis/gcm/engine/connection_factory_impl.cc
> File google_apis/gcm/engine/connection_factory_impl.cc (right):
>
> https://codereview.chromium.org/248213004/diff/20001/
> google_apis/gcm/engine/connection_factory_impl.cc#newcode242
> google_apis/gcm/engine/connection_factory_impl.cc:242: GURL cur_endpoint
> = GetCurrentEndpoint();
> nit: avoid using abbreviation if possible. so name it as
> current_endpoint
>
> https://codereview.chromium.org/248213004/diff/20001/
> google_apis/gcm/gcm_client_impl.cc
> File google_apis/gcm/gcm_client_impl.cc (right):
>
> https://codereview.chromium.org/248213004/diff/20001/
> google_apis/gcm/gcm_client_impl.cc#newcode715
> google_apis/gcm/gcm_client_impl.cc:715:
> recorder_.RecordDataRecieved(app_id, data_message_stanza.from(),
> You can combine both recording into one by introducing a bool flag to
> track if the message should be dropped, doing the recording, and then
> checking for the dropping.
>
> https://codereview.chromium.org/248213004/diff/20001/
> google_apis/gcm/monitoring/gcm_stats_recorder.cc
> File google_apis/gcm/monitoring/gcm_stats_recorder.cc (right):
>
> https://codereview.chromium.org/248213004/diff/20001/
> google_apis/gcm/monitoring/gcm_stats_recorder.cc#newcode131
> google_apis/gcm/monitoring/gcm_stats_recorder.cc:131:
> GCMStatsRecorder::ReceivingActivity::ReceivingActivity() :
> nit: ":" should be placed at the next line.
>
> https://codereview.chromium.org/248213004/diff/20001/
> google_apis/gcm/monitoring/gcm_stats_recorder.cc#newcode169
> google_apis/gcm/monitoring/gcm_stats_recorder.cc:169: if (is_recording_)
> {
> nit: brackets not needed
>
> https://codereview.chromium.org/248213004/diff/20001/
> google_apis/gcm/monitoring/gcm_stats_recorder.h
> File google_apis/gcm/monitoring/gcm_stats_recorder.h (right):
>
> https://codereview.chromium.org/248213004/diff/20001/
> google_apis/gcm/monitoring/gcm_stats_recorder.h#newcode41
> google_apis/gcm/monitoring/gcm_stats_recorder.h:41:
> nit: remove empty line
>
> https://codereview.chromium.org/248213004/diff/20001/
> google_apis/gcm/monitoring/gcm_stats_recorder.h#newcode49
> google_apis/gcm/monitoring/gcm_stats_recorder.h:49: std::string
> android_id;
> Is this needed? Android id is one per profile.
>
> https://codereview.chromium.org/248213004/diff/20001/
> google_apis/gcm/monitoring/gcm_stats_recorder.h#newcode103
> google_apis/gcm/monitoring/gcm_stats_recorder.h:103: void
> RecordRegistrationSent(const std::string& android_id,
> No need to record android_id.
>
> https://codereview.chromium.org/248213004/diff/20001/
> google_apis/gcm/monitoring/gcm_stats_recorder.h#newcode108
> google_apis/gcm/monitoring/gcm_stats_recorder.h:108: void
> RecordRegistrationResponse(const std::string& android_id,
> ditto for android_id.
>
> https://codereview.chromium.org/248213004/diff/20001/
> google_apis/gcm/monitoring/gcm_stats_recorder.h#newcode115
> google_apis/gcm/monitoring/gcm_stats_recorder.h:115: const std::string&
> android_id,
> ditto for android_id.
>
> https://codereview.chromium.org/248213004/diff/20001/
> google_apis/gcm/monitoring/gcm_stats_recorder.h#newcode121
> google_apis/gcm/monitoring/gcm_stats_recorder.h:121: void
> RecordDataRecieved(const std::string& app_id,
> RecordDataMessageReceived
>
> https://codereview.chromium.org/248213004/diff/20001/
> google_apis/gcm/monitoring/gcm_stats_recorder.h#newcode124
> google_apis/gcm/monitoring/gcm_stats_recorder.h:124: bool
> to_registered_app);
> dropped?
>
> https://codereview.chromium.org/248213004/diff/20001/
> google_apis/gcm/monitoring/gcm_stats_recorder.h#newcode127
> google_apis/gcm/monitoring/gcm_stats_recorder.h:127: void
> RecordDataDeletedMessage(const std::string& app_id,
> RecordMessagesDeletedReceived
>
> https://codereview.chromium.org/248213004/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[juyik, 2014-04-23 21:36:55]

Added recording for unregistration as well.

https://codereview.chromium.org/248213004/diff/20001/google_apis/gcm/engine/c...
File google_apis/gcm/engine/connection_factory_impl.cc (right):

https://codereview.chromium.org/248213004/diff/20001/google_apis/gcm/engine/c...
google_apis/gcm/engine/connection_factory_impl.cc:242: GURL cur_endpoint =
GetCurrentEndpoint();
On 2014/04/23 17:22:35, jianli wrote:
> nit: avoid using abbreviation if possible. so name it as current_endpoint

Done.

https://codereview.chromium.org/248213004/diff/20001/google_apis/gcm/gcm_clie...
File google_apis/gcm/gcm_client_impl.cc (right):

https://codereview.chromium.org/248213004/diff/20001/google_apis/gcm/gcm_clie...
google_apis/gcm/gcm_client_impl.cc:715: recorder_.RecordDataRecieved(app_id,
data_message_stanza.from(),
On 2014/04/23 17:22:35, jianli wrote:
> You can combine both recording into one by introducing a bool flag to track if
> the message should be dropped, doing the recording, and then checking for the
> dropping.

Done.

https://codereview.chromium.org/248213004/diff/20001/google_apis/gcm/monitori...
File google_apis/gcm/monitoring/gcm_stats_recorder.cc (right):

https://codereview.chromium.org/248213004/diff/20001/google_apis/gcm/monitori...
google_apis/gcm/monitoring/gcm_stats_recorder.cc:131:
GCMStatsRecorder::ReceivingActivity::ReceivingActivity() :
On 2014/04/23 17:22:35, jianli wrote:
> nit: ":" should be placed at the next line.

Done.

https://codereview.chromium.org/248213004/diff/20001/google_apis/gcm/monitori...
google_apis/gcm/monitoring/gcm_stats_recorder.cc:169: if (is_recording_) {
On 2014/04/23 17:22:35, jianli wrote:
> nit: brackets not needed

Done.

https://codereview.chromium.org/248213004/diff/20001/google_apis/gcm/monitori...
File google_apis/gcm/monitoring/gcm_stats_recorder.h (right):

https://codereview.chromium.org/248213004/diff/20001/google_apis/gcm/monitori...
google_apis/gcm/monitoring/gcm_stats_recorder.h:41: 
On 2014/04/23 17:22:35, jianli wrote:
> nit: remove empty line

Done.

https://codereview.chromium.org/248213004/diff/20001/google_apis/gcm/monitori...
google_apis/gcm/monitoring/gcm_stats_recorder.h:49: std::string android_id;
On 2014/04/23 17:22:35, jianli wrote:
> Is this needed? Android id is one per profile.

Done.

https://codereview.chromium.org/248213004/diff/20001/google_apis/gcm/monitori...
google_apis/gcm/monitoring/gcm_stats_recorder.h:103: void
RecordRegistrationSent(const std::string& android_id,
On 2014/04/23 17:22:35, jianli wrote:
> No need to record android_id.

Done.

https://codereview.chromium.org/248213004/diff/20001/google_apis/gcm/monitori...
google_apis/gcm/monitoring/gcm_stats_recorder.h:108: void
RecordRegistrationResponse(const std::string& android_id,
On 2014/04/23 17:22:35, jianli wrote:
> ditto for android_id.

Done.

https://codereview.chromium.org/248213004/diff/20001/google_apis/gcm/monitori...
google_apis/gcm/monitoring/gcm_stats_recorder.h:115: const std::string&
android_id,
On 2014/04/23 17:22:35, jianli wrote:
> ditto for android_id.

Done.

https://codereview.chromium.org/248213004/diff/20001/google_apis/gcm/monitori...
google_apis/gcm/monitoring/gcm_stats_recorder.h:121: void
RecordDataRecieved(const std::string& app_id,
On 2014/04/23 17:22:35, jianli wrote:
> RecordDataMessageReceived

Done.

https://codereview.chromium.org/248213004/diff/20001/google_apis/gcm/monitori...
google_apis/gcm/monitoring/gcm_stats_recorder.h:124: bool to_registered_app);
On 2014/04/23 17:22:35, jianli wrote:
> dropped?
Not really. This means the received message is meant for an app that is not in
the registration cache, which is an error and I record it here.

https://codereview.chromium.org/248213004/diff/20001/google_apis/gcm/monitori...
google_apis/gcm/monitoring/gcm_stats_recorder.h:127: void
RecordDataDeletedMessage(const std::string& app_id,
On 2014/04/23 17:22:35, jianli wrote:
> RecordMessagesDeletedReceived

N/A anymore because I combined merged the 2 methods according to your comments
in gcm_client_impl.cc.

[jianli, 2014-04-23 23:16:38]

lgtm

https://codereview.chromium.org/248213004/diff/60001/google_apis/gcm/gcm_clie...
File google_apis/gcm/gcm_client_impl.cc (right):

https://codereview.chromium.org/248213004/diff/60001/google_apis/gcm/gcm_clie...
google_apis/gcm/gcm_client_impl.cc:708:
recorder_.RecordDataMessageRecieved(app_id, data_message_stanza.from(),
You can combine this recording with the following recording in one, like:
  bool to_registered_apps = iter != registrations_.end() && ...;
  recorder_.RecordDataMessageRecieved(app_id, data_message_stanza.from(),
      data_message_stanza.ByteSize(), to_registered_apps, false);

  if (!to_registered_apps)
    return;

[Nicolas Zea, 2014-04-23 23:37:04]

https://codereview.chromium.org/248213004/diff/60001/google_apis/gcm/engine/c...
File google_apis/gcm/engine/connection_factory_impl.cc (right):

https://codereview.chromium.org/248213004/diff/60001/google_apis/gcm/engine/c...
google_apis/gcm/engine/connection_factory_impl.cc:297: LOG(ERROR) << "Failed to
connect to MCS endpoint with error " << result;
Should we also record connection failure?

https://codereview.chromium.org/248213004/diff/60001/google_apis/gcm/gcm_clie...
File google_apis/gcm/gcm_client.h (right):

https://codereview.chromium.org/248213004/diff/60001/google_apis/gcm/gcm_clie...
google_apis/gcm/gcm_client.h:108:
std::vector<GCMStatsRecorder::ConnectionActivity> connection_activities;
I wonder if all of these should be encapsulated in a single struct?

https://codereview.chromium.org/248213004/diff/60001/google_apis/gcm/monitori...
File google_apis/gcm/monitoring/gcm_stats_recorder.cc (right):

https://codereview.chromium.org/248213004/diff/60001/google_apis/gcm/monitori...
google_apis/gcm/monitoring/gcm_stats_recorder.cc:200: if (is_recording_)
I think it's clearer to just do if (!is_recording) return for these methods, to
make it clear there's nothing we do when we're not recording

[juyik, 2014-04-24 00:53:45]

https://codereview.chromium.org/248213004/diff/60001/google_apis/gcm/engine/c...
File google_apis/gcm/engine/connection_factory_impl.cc (right):

https://codereview.chromium.org/248213004/diff/60001/google_apis/gcm/engine/c...
google_apis/gcm/engine/connection_factory_impl.cc:297: LOG(ERROR) << "Failed to
connect to MCS endpoint with error " << result;
On 2014/04/23 23:37:04, Nicolas Zea wrote:
> Should we also record connection failure?

Done.

https://codereview.chromium.org/248213004/diff/60001/google_apis/gcm/gcm_clie...
File google_apis/gcm/gcm_client.h (right):

https://codereview.chromium.org/248213004/diff/60001/google_apis/gcm/gcm_clie...
google_apis/gcm/gcm_client.h:108:
std::vector<GCMStatsRecorder::ConnectionActivity> connection_activities;
On 2014/04/23 23:37:04, Nicolas Zea wrote:
> I wonder if all of these should be encapsulated in a single struct?

Done.

https://codereview.chromium.org/248213004/diff/60001/google_apis/gcm/gcm_clie...
File google_apis/gcm/gcm_client_impl.cc (right):

https://codereview.chromium.org/248213004/diff/60001/google_apis/gcm/gcm_clie...
google_apis/gcm/gcm_client_impl.cc:708:
recorder_.RecordDataMessageRecieved(app_id, data_message_stanza.from(),
On 2014/04/23 23:16:39, jianli wrote:
> You can combine this recording with the following recording in one, like:
>   bool to_registered_apps = iter != registrations_.end() && ...;
>   recorder_.RecordDataMessageRecieved(app_id, data_message_stanza.from(),
>       data_message_stanza.ByteSize(), to_registered_apps, false);
> 
>   if (!to_registered_apps)
>     return;

Done.

https://codereview.chromium.org/248213004/diff/60001/google_apis/gcm/monitori...
File google_apis/gcm/monitoring/gcm_stats_recorder.cc (right):

https://codereview.chromium.org/248213004/diff/60001/google_apis/gcm/monitori...
google_apis/gcm/monitoring/gcm_stats_recorder.cc:200: if (is_recording_)
On 2014/04/23 23:37:04, Nicolas Zea wrote:
> I think it's clearer to just do if (!is_recording) return for these methods,
to
> make it clear there's nothing we do when we're not recording

Done.

[Nicolas Zea, 2014-04-24 21:37:42]

LGTM with a nit

https://codereview.chromium.org/248213004/diff/90001/google_apis/gcm/monitori...
File google_apis/gcm/monitoring/gcm_stats_recorder.h (right):

https://codereview.chromium.org/248213004/diff/90001/google_apis/gcm/monitori...
google_apis/gcm/monitoring/gcm_stats_recorder.h:154: bool to_registered_app,
nit: style guide prefers using an enum rather than a bool for these kinds of
parameters.

[juyik, 2014-04-25 20:37:38]

Hi arv, can you do an owner review of 1 file? Thanks.

https://codereview.chromium.org/248213004/diff/90001/google_apis/gcm/monitori...
File google_apis/gcm/monitoring/gcm_stats_recorder.h (right):

https://codereview.chromium.org/248213004/diff/90001/google_apis/gcm/monitori...
google_apis/gcm/monitoring/gcm_stats_recorder.h:154: bool to_registered_app,
On 2014/04/24 21:37:43, Nicolas Zea wrote:
> nit: style guide prefers using an enum rather than a bool for these kinds of
> parameters.

Done. Added enum of received message type.

[arv (Not doing code reviews), 2014-04-28 16:55:45]

Rubber stamp LGTM

[juyik, 2014-04-28 20:25:42]

The CQ bit was checked by juyik@chromium.org

[commit-bot: I haz the power, 2014-04-28 20:26:14]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/juyik@chromium.org/248213004/130001

[juyik, 2014-04-28 22:36:37]

The CQ bit was unchecked by juyik@chromium.org

[juyik, 2014-04-28 23:21:10]

The CQ bit was checked by juyik@chromium.org

[commit-bot: I haz the power, 2014-04-28 23:21:49]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/juyik@chromium.org/248213004/150001

[commit-bot: I haz the power, 2014-04-29 00:40:06]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-04-29 00:40:07]

Try jobs failed on following builders:
  tryserver.chromium on win_chromium_rel

[juyik, 2014-04-29 17:58:51]

The CQ bit was checked by juyik@chromium.org

[commit-bot: I haz the power, 2014-04-29 17:59:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/juyik@chromium.org/248213004/170001

[commit-bot: I haz the power, 2014-04-30 00:39:15]

Change committed as 267008