Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
158023007:
Add GCM check in status code metric collection. We also decided to collect registration success sig… (Closed)
|
Created:
6 years, 10 months ago by juyik Modified:
6 years, 10 months ago CC:
chromium-reviews, Ilya Sherman, jar (doing other things), asvitkine+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionAdd GCM check in status code metric collection. We also decided to collect registration success signal now.
BUG=284553
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=250963
Patch Set 1 #
Total comments: 2
Patch Set 2 : . #
Total comments: 7
Patch Set 3 : . #Patch Set 4 : . #
Total comments: 6
Patch Set 5 : . #Patch Set 6 : resolve merge conflict. #Patch Set 7 : sync and rebased. #Patch Set 8 : . #
Messages
Total messages: 28 (0 generated)
https://codereview.chromium.org/158023007/diff/1/google_apis/gcm/engine/check... File google_apis/gcm/engine/checkin_request.cc (right): https://codereview.chromium.org/158023007/diff/1/google_apis/gcm/engine/check... google_apis/gcm/engine/checkin_request.cc:118: UMA_HISTOGRAM_ENUMERATION("GCM.CheckinRequestStatus", Nit: Instead of repeating this histogram macro in many places, can you make a helper function that just takes the CheckinRequestStatus enum value as a parameter and logs this histogram? The macro expands to a lot of code and its better for generated machine code size to not repeat it too much, if possible.
https://codereview.chromium.org/158023007/diff/1/google_apis/gcm/engine/check... File google_apis/gcm/engine/checkin_request.cc (right): https://codereview.chromium.org/158023007/diff/1/google_apis/gcm/engine/check... google_apis/gcm/engine/checkin_request.cc:118: UMA_HISTOGRAM_ENUMERATION("GCM.CheckinRequestStatus", On 2014/02/12 17:49:05, Alexei Svitkine wrote: > Nit: Instead of repeating this histogram macro in many places, can you make a > helper function that just takes the CheckinRequestStatus enum value as a > parameter and logs this histogram? The macro expands to a lot of code and its > better for generated machine code size to not repeat it too much, if possible. Done.
a few comments. https://codereview.chromium.org/158023007/diff/70001/google_apis/gcm/engine/c... File google_apis/gcm/engine/checkin_request.cc (right): https://codereview.chromium.org/158023007/diff/70001/google_apis/gcm/engine/c... google_apis/gcm/engine/checkin_request.cc:23: // This enum is also used in an UMA histogram (GCMRegistrationRequestStatus GCMRegistrationRequestStatus? https://codereview.chromium.org/158023007/diff/70001/google_apis/gcm/engine/c... google_apis/gcm/engine/checkin_request.cc:33: FAILED_PARSING_RESPONSE, // Failed to parse the checkin response. RESPONSE_PARSIN_FAILED will be consistent with URL_FETCHING... https://codereview.chromium.org/158023007/diff/70001/tools/metrics/histograms... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/158023007/diff/70001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:26371: + <int value="5" label="Failed parsing response"/> Response parsing failed
https://codereview.chromium.org/158023007/diff/70001/google_apis/gcm/engine/c... File google_apis/gcm/engine/checkin_request.cc (right): https://codereview.chromium.org/158023007/diff/70001/google_apis/gcm/engine/c... google_apis/gcm/engine/checkin_request.cc:23: // This enum is also used in an UMA histogram (GCMRegistrationRequestStatus On 2014/02/12 18:22:58, Filip Gorski wrote: > GCMRegistrationRequestStatus? Done. https://codereview.chromium.org/158023007/diff/70001/google_apis/gcm/engine/c... google_apis/gcm/engine/checkin_request.cc:33: FAILED_PARSING_RESPONSE, // Failed to parse the checkin response. On 2014/02/12 18:22:58, Filip Gorski wrote: > RESPONSE_PARSIN_FAILED > will be consistent with URL_FETCHING... Done. https://codereview.chromium.org/158023007/diff/70001/google_apis/gcm/engine/c... google_apis/gcm/engine/checkin_request.cc:33: FAILED_PARSING_RESPONSE, // Failed to parse the checkin response. On 2014/02/12 18:22:58, Filip Gorski wrote: > RESPONSE_PARSIN_FAILED > will be consistent with URL_FETCHING... Done. https://codereview.chromium.org/158023007/diff/70001/tools/metrics/histograms... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/158023007/diff/70001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:26371: + <int value="5" label="Failed parsing response"/> On 2014/02/12 18:22:58, Filip Gorski wrote: > Response parsing failed Done.
Could you re-upload the patchset. It's showing up as error: old chunk mismatch .
On 2014/02/12 18:33:24, Alexei Svitkine wrote: > Could you re-upload the patchset. It's showing up as error: old chunk mismatch > . OK, it's fixed now.
https://codereview.chromium.org/158023007/diff/160002/google_apis/gcm/engine/... File google_apis/gcm/engine/checkin_request.cc (right): https://codereview.chromium.org/158023007/diff/160002/google_apis/gcm/engine/... google_apis/gcm/engine/checkin_request.cc:137: } else { nit: bracket is not needed for single line if/else. Even better, you could simplify this block to: RecordCheckinStatusToUMA(response_status == net::HTTP_BAD_REQUEST ? HTTP_BAD_REQUEST : HTTP_UNAUTHORIZED); https://codereview.chromium.org/158023007/diff/160002/google_apis/gcm/engine/... google_apis/gcm/engine/checkin_request.cc:151: } else { ditto https://codereview.chromium.org/158023007/diff/160002/google_apis/gcm/engine/... File google_apis/gcm/engine/registration_request.cc (right): https://codereview.chromium.org/158023007/diff/160002/google_apis/gcm/engine/... google_apis/gcm/engine/registration_request.cc:206: UMA_HISTOGRAM_ENUMERATION("GCM.RegistrationRequestStatus", SUCCESS, nit: define a helper function as you did in CheckinRequest
lgtm please address comments from Jian Li.
https://codereview.chromium.org/158023007/diff/160002/google_apis/gcm/engine/... File google_apis/gcm/engine/checkin_request.cc (right): https://codereview.chromium.org/158023007/diff/160002/google_apis/gcm/engine/... google_apis/gcm/engine/checkin_request.cc:137: } else { On 2014/02/12 18:41:05, jianli wrote: > nit: bracket is not needed for single line if/else. > > Even better, you could simplify this block to: > RecordCheckinStatusToUMA(response_status == net::HTTP_BAD_REQUEST ? > HTTP_BAD_REQUEST : HTTP_UNAUTHORIZED); Done. https://codereview.chromium.org/158023007/diff/160002/google_apis/gcm/engine/... google_apis/gcm/engine/checkin_request.cc:151: } else { On 2014/02/12 18:41:05, jianli wrote: > ditto Done. https://codereview.chromium.org/158023007/diff/160002/google_apis/gcm/engine/... File google_apis/gcm/engine/registration_request.cc (right): https://codereview.chromium.org/158023007/diff/160002/google_apis/gcm/engine/... google_apis/gcm/engine/registration_request.cc:206: UMA_HISTOGRAM_ENUMERATION("GCM.RegistrationRequestStatus", SUCCESS, On 2014/02/12 18:41:05, jianli wrote: > nit: define a helper function as you did in CheckinRequest Done.
lgtm
lgtm
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/158023007/360001
The CQ bit was unchecked by commit-bot@chromium.org
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 FAILED at 19.
Hunk #3 succeeded at 94 (offset -1 lines).
Hunk #4 succeeded at 107 (offset -1 lines).
Hunk #5 succeeded at 118 (offset -1 lines).
Hunk #6 succeeded at 129 (offset -1 lines).
1 out of 6 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
b46415ab96ffb48e073e7e9ff420e5816f445551..3ff0f9df2b6deb0c0c69dfe2ac64747d7d3b06e2
100644
--- a/google_apis/gcm/engine/checkin_request.cc
+++ b/google_apis/gcm/engine/checkin_request.cc
@@ -6,6 +6,7 @@
#include "base/bind.h"
#include "base/message_loop/message_loop.h"
+#include "base/metrics/histogram.h"
#include "google_apis/gcm/protocol/checkin.pb.h"
#include "net/http/http_status_code.h"
#include "net/url_request/url_fetcher.h"
@@ -18,6 +19,30 @@ namespace {
const char kCheckinURL[] = "https://android.clients.google.com/checkin";
const char kRequestContentType[] = "application/x-protobuf";
const int kRequestVersionValue = 2;
+
+// 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.
+enum CheckinRequestStatus {
+ SUCCESS, // Checkin completed successfully.
+ URL_FETCHING_FAILED, // URL fetching failed.
+ HTTP_BAD_REQUEST, // The request was malformed.
+ HTTP_UNAUTHORIZED, // The security token didn't match the android
id.
+ HTTP_NOT_OK, // HTTP status was not OK.
+ RESPONSE_PARSING_FAILED, // Check in response parsing failed.
+ ZERO_ID_OR_TOKEN, // Either returned android id or security token
+ // was zero.
+ // NOTE: always keep this entry at the end. Add new status types only
+ // immediately above this line. Make sure to update the corresponding
+ // histogram enum accordingly.
+ STATUS_COUNT
+};
+
+void RecordCheckinStatusToUMA(CheckinRequestStatus status) {
+ UMA_HISTOGRAM_ENUMERATION("GCM.CheckinRequestStatus", status, STATUS_COUNT);
+}
+
} // namespace
CheckinRequest::CheckinRequest(
@@ -94,6 +119,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);
RetryWithBackoff(true);
return;
}
@@ -106,6 +132,8 @@ 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);
callback_.Run(0,0);
return;
}
@@ -115,6 +143,8 @@ 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);
RetryWithBackoff(true);
return;
}
@@ -124,10 +154,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);
RetryWithBackoff(true);
return;
}
+ RecordCheckinStatusToUMA(SUCCESS);
callback_.Run(response_proto.android_id(), response_proto.security_token());
}
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/158023007/530001
The CQ bit was unchecked by commit-bot@chromium.org
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 FAILED at 19.
Hunk #3 succeeded at 94 (offset -1 lines).
Hunk #4 succeeded at 107 (offset -1 lines).
Hunk #5 succeeded at 118 (offset -1 lines).
Hunk #6 succeeded at 129 (offset -1 lines).
1 out of 6 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
b46415ab96ffb48e073e7e9ff420e5816f445551..c1b95eeac0086542d701d3785bba4f14155bd3c7
100644
--- a/google_apis/gcm/engine/checkin_request.cc
+++ b/google_apis/gcm/engine/checkin_request.cc
@@ -6,6 +6,7 @@
#include "base/bind.h"
#include "base/message_loop/message_loop.h"
+#include "base/metrics/histogram.h"
#include "google_apis/gcm/protocol/checkin.pb.h"
#include "net/http/http_status_code.h"
#include "net/url_request/url_fetcher.h"
@@ -18,6 +19,31 @@ namespace {
const char kCheckinURL[] = "https://android.clients.google.com/checkin";
const char kRequestContentType[] = "application/x-protobuf";
const int kRequestVersionValue = 2;
+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.
+enum CheckinRequestStatus {
+ SUCCESS, // Checkin completed successfully.
+ URL_FETCHING_FAILED, // URL fetching failed.
+ HTTP_BAD_REQUEST, // The request was malformed.
+ HTTP_UNAUTHORIZED, // The security token didn't match the android
id.
+ HTTP_NOT_OK, // HTTP status was not OK.
+ RESPONSE_PARSING_FAILED, // Check in response parsing failed.
+ ZERO_ID_OR_TOKEN, // Either returned android id or security token
+ // was zero.
+ // NOTE: always keep this entry at the end. Add new status types only
+ // immediately above this line. Make sure to update the corresponding
+ // histogram enum accordingly.
+ STATUS_COUNT
+};
+
+void RecordCheckinStatusToUMA(CheckinRequestStatus status) {
+ UMA_HISTOGRAM_ENUMERATION("GCM.CheckinRequestStatus", status, STATUS_COUNT);
+}
+
} // namespace
CheckinRequest::CheckinRequest(
@@ -94,6 +120,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);
RetryWithBackoff(true);
return;
}
@@ -106,6 +133,8 @@ 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);
callback_.Run(0,0);
return;
}
@@ -115,6 +144,8 @@ 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);
RetryWithBackoff(true);
return;
}
@@ -124,10 +155,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);
RetryWithBackoff(true);
return;
}
+ RecordCheckinStatusToUMA(SUCCESS);
callback_.Run(response_proto.android_id(), response_proto.security_token());
}
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/158023007/630001
The CQ bit was unchecked by commit-bot@chromium.org
Retried try job too often on win_rel for step(s) base_unittests, browser_tests, interactive_ui_tests, net_unittests, unit_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
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/158023007/630001
Message was sent while issue was closed.
Change committed as 250963 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
