Add checkin activity recording to gcm recorder.

Add checkin activity recording to gcm recorder.

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

[jianli, 2014-05-01 00:22:34]

https://codereview.chromium.org/261573002/diff/1/google_apis/gcm/engine/check...
File google_apis/gcm/engine/checkin_request.cc (right):

https://codereview.chromium.org/261573002/diff/1/google_apis/gcm/engine/check...
google_apis/gcm/engine/checkin_request.cc:171: recorder_->RecordCheckinFailure(
Can this be merged with RecordCheckinStatusToUMA?

https://codereview.chromium.org/261573002/diff/1/google_apis/gcm/monitoring/g...
File google_apis/gcm/monitoring/gcm_stats_recorder.cc (right):

https://codereview.chromium.org/261573002/diff/1/google_apis/gcm/monitoring/g...
google_apis/gcm/monitoring/gcm_stats_recorder.cc:242: std::string().c_str()));
nit: should passing empty string simply work?

[juyik, 2014-05-01 01:03:35]

https://codereview.chromium.org/261573002/diff/1/google_apis/gcm/engine/check...
File google_apis/gcm/engine/checkin_request.cc (right):

https://codereview.chromium.org/261573002/diff/1/google_apis/gcm/engine/check...
google_apis/gcm/engine/checkin_request.cc:171: recorder_->RecordCheckinFailure(
On 2014/05/01 00:22:35, jianli wrote:
> Can this be merged with RecordCheckinStatusToUMA?

Done.

https://codereview.chromium.org/261573002/diff/1/google_apis/gcm/monitoring/g...
File google_apis/gcm/monitoring/gcm_stats_recorder.cc (right):

https://codereview.chromium.org/261573002/diff/1/google_apis/gcm/monitoring/g...
google_apis/gcm/monitoring/gcm_stats_recorder.cc:242: std::string().c_str()));
On 2014/05/01 00:22:35, jianli wrote:
> nit: should passing empty string simply work?

Done.

[jianli, 2014-05-01 21:36:30]

lgtm

https://codereview.chromium.org/261573002/diff/40001/google_apis/gcm/engine/c...
File google_apis/gcm/engine/checkin_request.cc (right):

https://codereview.chromium.org/261573002/diff/40001/google_apis/gcm/engine/c...
google_apis/gcm/engine/checkin_request.cc:68: void
RecordCheckinStatusAndToUMA(CheckinRequestStatus status,
nit: RecordCheckinStatusAndReportUMA

https://codereview.chromium.org/261573002/diff/40001/google_apis/gcm/engine/c...
google_apis/gcm/engine/checkin_request.cc:218:
recorder_->RecordCheckinSuccess();
nit: I think these 2 calls could also be merged into
RecordCheckinStatusAndReportUMA. In the function, you could check status to
decide which record method to call.

[juyik, 2014-05-01 21:54:34]

https://codereview.chromium.org/261573002/diff/40001/google_apis/gcm/engine/c...
File google_apis/gcm/engine/checkin_request.cc (right):

https://codereview.chromium.org/261573002/diff/40001/google_apis/gcm/engine/c...
google_apis/gcm/engine/checkin_request.cc:68: void
RecordCheckinStatusAndToUMA(CheckinRequestStatus status,
On 2014/05/01 21:36:31, jianli wrote:
> nit: RecordCheckinStatusAndReportUMA

Done.

https://codereview.chromium.org/261573002/diff/40001/google_apis/gcm/engine/c...
google_apis/gcm/engine/checkin_request.cc:218:
recorder_->RecordCheckinSuccess();
On 2014/05/01 21:36:31, jianli wrote:
> nit: I think these 2 calls could also be merged into
> RecordCheckinStatusAndReportUMA. In the function, you could check status to
> decide which record method to call.

Done.

[juyik, 2014-05-01 23:02:20]

Hi Nicolas, can you take a look at this patch? Thanks.

[jianli, 2014-05-01 23:02:37]

Still lgtm

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

https://codereview.chromium.org/261573002/diff/60001/google_apis/gcm/engine/c...
google_apis/gcm/engine/checkin_request.cc:67: // Record checkin failure status
to both stats recorder and to UMA.
nit: please update comment

[juyik, 2014-05-02 00:41:54]

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

https://codereview.chromium.org/261573002/diff/60001/google_apis/gcm/engine/c...
google_apis/gcm/engine/checkin_request.cc:67: // Record checkin failure status
to both stats recorder and to UMA.
On 2014/05/01 23:02:37, jianli wrote:
> nit: please update comment

Done.

[Nicolas Zea, 2014-05-02 20:51:36]

lgtm

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

https://codereview.chromium.org/261573002/diff/80001/google_apis/gcm/monitori...
google_apis/gcm/monitoring/gcm_stats_recorder.cc:22: // Insert an itme to the
front of deque while maintaining the size of the deque.
nit: itme -> item

https://codereview.chromium.org/261573002/diff/80001/google_apis/gcm/monitori...
google_apis/gcm/monitoring/gcm_stats_recorder.cc:241: "%s.%s", status.c_str(),
will_retry ? " Will retry." : ""));
nit: perhaps also say "Will not retry" for false case

https://codereview.chromium.org/261573002/diff/80001/google_apis/gcm/monitori...
File google_apis/gcm/monitoring/gcm_stats_recorder_unittest.cc (right):

https://codereview.chromium.org/261573002/diff/80001/google_apis/gcm/monitori...
google_apis/gcm/monitoring/gcm_stats_recorder_unittest.cc:18: static uint64
kAndroidId = static_cast<uint64>(4);
nit: I think you could just use 4U rather than static cast

[juyik, 2014-05-02 21:44:18]

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

https://codereview.chromium.org/261573002/diff/80001/google_apis/gcm/monitori...
google_apis/gcm/monitoring/gcm_stats_recorder.cc:22: // Insert an itme to the
front of deque while maintaining the size of the deque.
On 2014/05/02 20:51:36, Nicolas Zea wrote:
> nit: itme -> item

Done.

https://codereview.chromium.org/261573002/diff/80001/google_apis/gcm/monitori...
google_apis/gcm/monitoring/gcm_stats_recorder.cc:241: "%s.%s", status.c_str(),
will_retry ? " Will retry." : ""));
On 2014/05/02 20:51:36, Nicolas Zea wrote:
> nit: perhaps also say "Will not retry" for false case

Done.

https://codereview.chromium.org/261573002/diff/80001/google_apis/gcm/monitori...
File google_apis/gcm/monitoring/gcm_stats_recorder_unittest.cc (right):

https://codereview.chromium.org/261573002/diff/80001/google_apis/gcm/monitori...
google_apis/gcm/monitoring/gcm_stats_recorder_unittest.cc:18: static uint64
kAndroidId = static_cast<uint64>(4);
On 2014/05/02 20:51:36, Nicolas Zea wrote:
> nit: I think you could just use 4U rather than static cast

Done.

[juyik, 2014-05-02 21:44:27]

The CQ bit was checked by juyik@chromium.org

[commit-bot: I haz the power, 2014-05-02 21:44:49]

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

[commit-bot: I haz the power, 2014-05-02 21:44:55]

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

[commit-bot: I haz the power, 2014-05-02 21:44:56]

Failed to apply patch for google_apis/gcm/engine/checkin_request.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file google_apis/gcm/engine/checkin_request.cc
  Hunk #2 succeeded at 23 (offset -2 lines).
  Hunk #3 succeeded at 39 (offset -2 lines).
  Hunk #4 FAILED at 98.
  Hunk #5 succeeded at 141 with fuzz 1.
  1 out of 10 hunks FAILED -- saving rejects to file
google_apis/gcm/engine/checkin_request.cc.rej

Patch:       google_apis/gcm/engine/checkin_request.cc
Index: google_apis/gcm/engine/checkin_request.cc
diff --git a/google_apis/gcm/engine/checkin_request.cc
b/google_apis/gcm/engine/checkin_request.cc
index
7addbebcad04b8ebe81bf7e6ec0e5ed2274335cb..77974d49fb31e23328c5c1f963c532de7b237083
100644
--- a/google_apis/gcm/engine/checkin_request.cc
+++ b/google_apis/gcm/engine/checkin_request.cc
@@ -7,6 +7,7 @@
 #include "base/bind.h"
 #include "base/message_loop/message_loop.h"
 #include "base/metrics/histogram.h"
+#include "google_apis/gcm/monitoring/gcm_stats_recorder.h"
 #include "google_apis/gcm/protocol/checkin.pb.h"
 #include "net/http/http_status_code.h"
 #include "net/url_request/url_fetcher.h"
@@ -24,7 +25,7 @@ const int kDefaultUserSerialNumber = 0;
 // This enum is also used in an UMA histogram (GCMCheckinRequestStatus
 // enum defined in tools/metrics/histograms/histogram.xml). Hence the entries
 // here shouldn't be deleted or re-ordered and new ones should be added to
-// the end.
+// the end, and update the GetCheckinRequestStatusString(...) below.
 enum CheckinRequestStatus {
   SUCCESS,                    // Checkin completed successfully.
   URL_FETCHING_FAILED,        // URL fetching failed.
@@ -40,8 +41,40 @@ enum CheckinRequestStatus {
   STATUS_COUNT
 };
 
-void RecordCheckinStatusToUMA(CheckinRequestStatus status) {
+// Returns string representation of enum CheckinRequestStatus.
+std::string GetCheckinRequestStatusString(CheckinRequestStatus status) {
+  switch (status) {
+    case SUCCESS:
+      return "SUCCESS";
+    case URL_FETCHING_FAILED:
+      return "URL_FETCHING_FAILED";
+    case HTTP_BAD_REQUEST:
+      return "HTTP_BAD_REQUEST";
+    case HTTP_UNAUTHORIZED:
+      return "HTTP_UNAUTHORIZED";
+    case HTTP_NOT_OK:
+      return "HTTP_NOT_OK";
+    case RESPONSE_PARSING_FAILED:
+      return "RESPONSE_PARSING_FAILED";
+    case ZERO_ID_OR_TOKEN:
+      return "ZERO_ID_OR_TOKEN";
+    default:
+      NOTREACHED();
+      return "UNKNOWN_STATUS";
+  }
+}
+
+// Records checkin status to both stats recorder and reports to UMA.
+void RecordCheckinStatusAndReportUMA(CheckinRequestStatus status,
+                                     GCMStatsRecorder* recorder,
+                                     bool will_retry) {
   UMA_HISTOGRAM_ENUMERATION("GCM.CheckinRequestStatus", status, STATUS_COUNT);
+  if (status == SUCCESS)
+    recorder->RecordCheckinSuccess();
+  else {
+    recorder->RecordCheckinFailure(GetCheckinRequestStatusString(status),
+                                   will_retry);
+  }
 }
 
 }  // namespace
@@ -65,11 +98,13 @@ CheckinRequest::CheckinRequest(
     const RequestInfo& request_info,
     const net::BackoffEntry::Policy& backoff_policy,
     const CheckinRequestCallback& callback,
-    net::URLRequestContextGetter* request_context_getter)
+    net::URLRequestContextGetter* request_context_getter,
+    GCMStatsRecorder* recorder)
     : request_context_getter_(request_context_getter),
       callback_(callback),
       backoff_entry_(&backoff_policy),
       request_info_(request_info),
+      recorder_(recorder),
       weak_ptr_factory_(this) {
 }
 
@@ -108,6 +143,7 @@ void CheckinRequest::Start() {
       net::URLFetcher::Create(GURL(kCheckinURL), net::URLFetcher::POST, this));
   url_fetcher_->SetRequestContext(request_context_getter_);
   url_fetcher_->SetUploadData(kRequestContentType, upload_data);
+  recorder_->RecordCheckinInitiated(request_info_.android_id);
   url_fetcher_->Start();
 }
 
@@ -121,6 +157,8 @@ void CheckinRequest::RetryWithBackoff(bool update_backoff) {
     DVLOG(1) << "Delay GCM checkin for: "
              << backoff_entry_.GetTimeUntilRelease().InMilliseconds()
              << " milliseconds.";
+    recorder_->RecordCheckinDelayedDueToBackoff(
+        backoff_entry_.GetTimeUntilRelease().InMilliseconds());
     base::MessageLoop::current()->PostDelayedTask(
         FROM_HERE,
         base::Bind(&CheckinRequest::RetryWithBackoff,
@@ -138,7 +176,7 @@ void CheckinRequest::OnURLFetchComplete(const
net::URLFetcher* source) {
   checkin_proto::AndroidCheckinResponse response_proto;
   if (!source->GetStatus().is_success()) {
     LOG(ERROR) << "Failed to get checkin response. Fetcher failed. Retrying.";
-    RecordCheckinStatusToUMA(URL_FETCHING_FAILED);
+    RecordCheckinStatusAndReportUMA(URL_FETCHING_FAILED, recorder_, true);
     RetryWithBackoff(true);
     return;
   }
@@ -151,8 +189,9 @@ void CheckinRequest::OnURLFetchComplete(const
net::URLFetcher* source) {
     // UNAUTHORIZED indicates that security token didn't match the android id.
     LOG(ERROR) << "No point retrying the checkin with status: "
                << response_status << ". Checkin failed.";
-    RecordCheckinStatusToUMA(response_status == net::HTTP_BAD_REQUEST ?
-        HTTP_BAD_REQUEST : HTTP_UNAUTHORIZED);
+    CheckinRequestStatus status = response_status == net::HTTP_BAD_REQUEST ?
+        HTTP_BAD_REQUEST : HTTP_UNAUTHORIZED;
+    RecordCheckinStatusAndReportUMA(status, recorder_, false);
     callback_.Run(response_proto);
     return;
   }
@@ -162,8 +201,9 @@ void CheckinRequest::OnURLFetchComplete(const
net::URLFetcher* source) {
       !response_proto.ParseFromString(response_string)) {
     LOG(ERROR) << "Failed to get checkin response. HTTP Status: "
                << response_status << ". Retrying.";
-    RecordCheckinStatusToUMA(response_status != net::HTTP_OK ?
-        HTTP_NOT_OK : RESPONSE_PARSING_FAILED);
+    CheckinRequestStatus status = response_status != net::HTTP_OK ?
+        HTTP_NOT_OK : RESPONSE_PARSING_FAILED;
+    RecordCheckinStatusAndReportUMA(status, recorder_, true);
     RetryWithBackoff(true);
     return;
   }
@@ -173,12 +213,12 @@ void CheckinRequest::OnURLFetchComplete(const
net::URLFetcher* source) {
       response_proto.android_id() == 0 ||
       response_proto.security_token() == 0) {
     LOG(ERROR) << "Android ID or security token is 0. Retrying.";
-    RecordCheckinStatusToUMA(ZERO_ID_OR_TOKEN);
+    RecordCheckinStatusAndReportUMA(ZERO_ID_OR_TOKEN, recorder_, true);
     RetryWithBackoff(true);
     return;
   }
 
-  RecordCheckinStatusToUMA(SUCCESS);
+  RecordCheckinStatusAndReportUMA(SUCCESS, recorder_, false);
   callback_.Run(response_proto);
 }

[juyik, 2014-05-02 22:11:48]

The CQ bit was checked by juyik@chromium.org

[commit-bot: I haz the power, 2014-05-02 22:13:22]

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

[commit-bot: I haz the power, 2014-05-02 23:41:32]

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

[commit-bot: I haz the power, 2014-05-02 23:41:33]

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

[juyik, 2014-05-03 14:51:04]

The CQ bit was checked by juyik@chromium.org

[commit-bot: I haz the power, 2014-05-03 14:51:13]

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

[commit-bot: I haz the power, 2014-05-03 16:26:39]

Change committed as 268070