Add activity recording capability to gcm internals page. User can refresh, start/stop recording, an…

google_apis/gcm/gcm_client_impl.cc

Index: google_apis/gcm/gcm_client_impl.cc
diff --git a/google_apis/gcm/gcm_client_impl.cc b/google_apis/gcm/gcm_client_impl.cc
index 4cc0fdd38b0acf28280c6df8b7cef754703a3db2..d97a4d1391bf9c9a7528100a9214add0a86a9596 100644
--- a/google_apis/gcm/gcm_client_impl.cc
+++ b/google_apis/gcm/gcm_client_impl.cc
@@ -11,6 +11,7 @@
 #include "base/message_loop/message_loop.h"
 #include "base/metrics/histogram.h"
 #include "base/sequenced_task_runner.h"
+#include "base/strings/stringprintf.h"
 #include "base/time/default_clock.h"
 #include "google_apis/gcm/base/mcs_message.h"
 #include "google_apis/gcm/base/mcs_util.h"
@@ -18,6 +19,7 @@
 #include "google_apis/gcm/engine/connection_factory_impl.h"
 #include "google_apis/gcm/engine/gcm_store_impl.h"
 #include "google_apis/gcm/engine/mcs_client.h"
+#include "google_apis/gcm/gcm_stats_recorder.h"
 #include "google_apis/gcm/protocol/mcs.pb.h"
 #include "net/http/http_network_session.h"
 #include "net/url_request/url_request_context.h"
@@ -128,12 +130,14 @@ scoped_ptr<MCSClient> GCMInternalsBuilder::BuildMCSClient(
     const std::string& version,
     base::Clock* clock,
     ConnectionFactory* connection_factory,
-    GCMStore* gcm_store) {
+    GCMStore* gcm_store,
+    GCMStatsRecorder* recorder) {
   return make_scoped_ptr<MCSClient>(
       new MCSClient(version,
                     clock,
                     connection_factory,
-                    gcm_store));
+                    gcm_store,
+                    recorder));
 }
 
 scoped_ptr<ConnectionFactory> GCMInternalsBuilder::BuildConnectionFactory(
@@ -237,7 +241,8 @@ void GCMClientImpl::InitializeMCSClient(
       chrome_build_proto_.chrome_version(),
       clock_.get(),
       connection_factory_.get(),
-      gcm_store_.get()).Pass();
+      gcm_store_.get(),
+      &recorder_).Pass();
 
   mcs_client_->Initialize(
       base::Bind(&GCMClientImpl::OnMCSError, weak_ptr_factory_.GetWeakPtr()),
@@ -522,16 +527,37 @@ std::string GCMClientImpl::GetStateString() const {
   }
 }
 
+void GCMClientImpl::SetRecording(bool recording) {
+  recorder_.SetRecording(recording);
+}
+
+void GCMClientImpl::ClearActivityLogs() {
+  recorder_.Clear();
+}
+
 GCMClient::GCMStatistics GCMClientImpl::GetStatistics() const {
   GCMClient::GCMStatistics stats;
-  stats.gcm_client_state = GCMClientImpl::GetStateString();
+  stats.is_recording = recorder_.is_recording();
+  stats.gcm_client_state = GetStateString();
   stats.connection_client_created = mcs_client_.get() != NULL;
   if (mcs_client_.get()) {
     stats.connection_state = mcs_client_->GetStateString();
-    // TODO(juyik): add more statistics such as message metadata list, etc.
+    stats.send_queue_size = mcs_client_->GetSendQueueSize();
+    stats.resend_queue_size = mcs_client_->GetResendQueueSize();
   }
   if (device_checkin_info_.android_id > 0)
     stats.android_id = device_checkin_info_.android_id;
+
+  for (std::deque<GCMStatsRecorder::SendingActivity>::const_iterator it =
+          recorder_.sending_activities().begin();

[fgorski, 2014/03/29 05:09:29]

stats.sending.insert(
    stats.sending.being(),
    recorder_.sending_activities().being(),
    recorder_.sending_activities().end())
and that applies to the registrations below as well.

2. please document that you are doing newest first.

3. Would it make sense for the recorder/stats to do that work? GCMClientImpl
should probably not be responsible for copying monitoring data from one
structure to another. (Unless it has people skills. :) )

[juyik, 2014/03/31 22:19:23]

Done.

+       it != recorder_.sending_activities().end();
+       ++it) {
+    stats.sending.push_back(*it);
+  }
+  for (RegistrationInfoMap::const_iterator it = registrations_.begin();
+       it != registrations_.end(); ++it) {
+    stats.registered_app_ids.push_back(it->first);
+  }
   return stats;
 }
 
@@ -660,6 +686,12 @@ void GCMClientImpl::HandleIncomingSendError(
     send_error_details.additional_data.erase(iter);
   }
 
+  recorder_.RecordIncomingSendError(
+      data_message_stanza.category(),
+      data_message_stanza.to(),
+      data_message_stanza.id(),
+      data_message_stanza.ByteSize(),

[fgorski, 2014/03/29 05:09:29]

I am not getting it. How is byte size of an error message relevant here?
Same goes for TTL.

I think the important part here is the origin of SendError, e.g. server vs. gcm
Client (doing a TTL expiration of the message).

Here is my suggestion:
Extract calls to delegate_->OnMessageSendError to a separate method with
parameters being (app_id, send_error_details)  ==> there is another call you are
missing
Do your best by logging only information from the parameters above.
Make sure to capture the result (it will be either SERVER_ERROR or TTL_EXCEEDED,
which will be a good indication of where the error originated)

Update:
I see that we are getting the local TTL EXCEEDED  as part of send message
status, so maybe there is a way to make the method more generic and add send
error as additional case. 

[juyik, 2014/03/31 22:19:23]

I have removed byte size and ttl. This call site and this recorder method is
exclusively used to record server errors. GCM client side send error is recorded
elsewhere.

+      data_message_stanza.ttl());
   delegate_->OnMessageSendError(data_message_stanza.category(),
                                 send_error_details);
 }