Record the net error for failed CheckinRequestStatus cases.
Also modernize and clean up the rest of the file and histogram usage a bit. Bug: 1053439 Change-Id: Id2047169f960986cae5eb08b4a2202e3c84aae3c Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2071264 Commit-Queue: Peter Beverloo <peter@chromium.org> Reviewed-by: Ilya Sherman <isherman@chromium.org> Reviewed-by: Richard Knoll <knollr@chromium.org> Cr-Commit-Position: refs/heads/master@{#753684}
This commit is contained in:

committed by
Commit Bot

parent
657d7447d3
commit
af532090f9
@ -6,7 +6,7 @@
|
||||
|
||||
#include "base/bind.h"
|
||||
#include "base/location.h"
|
||||
#include "base/metrics/histogram_macros.h"
|
||||
#include "base/metrics/histogram_functions.h"
|
||||
#include "google_apis/gcm/monitoring/gcm_stats_recorder.h"
|
||||
#include "google_apis/gcm/protocol/checkin.pb.h"
|
||||
#include "net/base/load_flags.h"
|
||||
@ -23,54 +23,62 @@ const int kRequestVersionValue = 3;
|
||||
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, and update the GetCheckinRequestStatusString(...) below.
|
||||
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.
|
||||
// enum defined in tools/metrics/histograms/enums.xml). Hence the entries here
|
||||
// shouldn't be deleted or re-ordered and new ones should be added to the end,
|
||||
// and update the GetCheckinRequestStatusString(...) below.
|
||||
enum class CheckinRequestStatus {
|
||||
kSuccess = 0, // Checkin completed successfully.
|
||||
// kUrlFetchingFailed = 1,
|
||||
kBadRequest = 2, // The request was malformed.
|
||||
kUnauthorized = 3, // The security token didn't match the AID.
|
||||
kStatusNotOK = 4, // HTTP status was not OK.
|
||||
kResponseParsingFailed = 5, // Check in response parsing failed.
|
||||
kZeroIdOrToken = 6, // Either returned android id or security token
|
||||
// was zero.
|
||||
kFailedNetError = 7, // A network error was returned.
|
||||
kFailedNoResponse = 8, // No or invalid response info was returned.
|
||||
kFailedNoHeaders = 9, // No or invalid headers were returned.
|
||||
|
||||
// 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
|
||||
kMaxValue = kFailedNoHeaders,
|
||||
};
|
||||
|
||||
// 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";
|
||||
case STATUS_COUNT:
|
||||
NOTREACHED();
|
||||
break;
|
||||
case CheckinRequestStatus::kSuccess:
|
||||
return "Success";
|
||||
case CheckinRequestStatus::kBadRequest:
|
||||
return "Failed: HTTP 400 Bad Request";
|
||||
case CheckinRequestStatus::kUnauthorized:
|
||||
return "Failed: HTTP 401 Unauthorized";
|
||||
case CheckinRequestStatus::kStatusNotOK:
|
||||
return "Failed: HTTP not OK";
|
||||
case CheckinRequestStatus::kResponseParsingFailed:
|
||||
return "Failed: Response parsing failed";
|
||||
case CheckinRequestStatus::kZeroIdOrToken:
|
||||
return "Failed: Zero Android ID or security token";
|
||||
case CheckinRequestStatus::kFailedNetError:
|
||||
return "Failed: Network error";
|
||||
case CheckinRequestStatus::kFailedNoResponse:
|
||||
return "Failed: No response";
|
||||
case CheckinRequestStatus::kFailedNoHeaders:
|
||||
return "Failed: No headers";
|
||||
}
|
||||
return "UNKNOWN_STATUS";
|
||||
|
||||
NOTREACHED();
|
||||
return "Failed: Unknown reason";
|
||||
}
|
||||
|
||||
// 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)
|
||||
base::UmaHistogramEnumeration("GCM.CheckinRequestStatus", status);
|
||||
|
||||
if (status == CheckinRequestStatus::kSuccess)
|
||||
recorder->RecordCheckinSuccess();
|
||||
else {
|
||||
recorder->RecordCheckinFailure(GetCheckinRequestStatusString(status),
|
||||
@ -94,7 +102,7 @@ CheckinRequest::RequestInfo::RequestInfo(
|
||||
|
||||
CheckinRequest::RequestInfo::RequestInfo(const RequestInfo& other) = default;
|
||||
|
||||
CheckinRequest::RequestInfo::~RequestInfo() {}
|
||||
CheckinRequest::RequestInfo::~RequestInfo() = default;
|
||||
|
||||
CheckinRequest::CheckinRequest(
|
||||
const GURL& checkin_url,
|
||||
@ -114,7 +122,7 @@ CheckinRequest::CheckinRequest(
|
||||
DCHECK(io_task_runner_);
|
||||
}
|
||||
|
||||
CheckinRequest::~CheckinRequest() {}
|
||||
CheckinRequest::~CheckinRequest() = default;
|
||||
|
||||
void CheckinRequest::Start() {
|
||||
DCHECK(io_task_runner_->RunsTasksInCurrentSequence());
|
||||
@ -217,37 +225,56 @@ void CheckinRequest::RetryWithBackoff() {
|
||||
|
||||
void CheckinRequest::OnURLLoadComplete(const network::SimpleURLLoader* source,
|
||||
std::unique_ptr<std::string> body) {
|
||||
checkin_proto::AndroidCheckinResponse response_proto;
|
||||
if (source->NetError() != net::OK || !source->ResponseInfo() ||
|
||||
!source->ResponseInfo()->headers) {
|
||||
LOG(ERROR) << "Failed to get checkin response. Fetcher failed. Retrying.";
|
||||
RecordCheckinStatusAndReportUMA(URL_FETCHING_FAILED, recorder_, true);
|
||||
if (source->NetError() != net::OK) {
|
||||
RecordCheckinStatusAndReportUMA(CheckinRequestStatus::kFailedNetError,
|
||||
recorder_, /* will_retry= */ true);
|
||||
base::UmaHistogramSparse("GCM.CheckinRequestStatusNetError",
|
||||
std::abs(source->NetError()));
|
||||
|
||||
RetryWithBackoff();
|
||||
return;
|
||||
}
|
||||
|
||||
if (!source->ResponseInfo()) {
|
||||
RecordCheckinStatusAndReportUMA(CheckinRequestStatus::kFailedNoResponse,
|
||||
recorder_, /* will_retry= */ true);
|
||||
RetryWithBackoff();
|
||||
return;
|
||||
}
|
||||
|
||||
if (!source->ResponseInfo()->headers) {
|
||||
RecordCheckinStatusAndReportUMA(CheckinRequestStatus::kFailedNoHeaders,
|
||||
recorder_, /* will_retry= */ true);
|
||||
RetryWithBackoff();
|
||||
return;
|
||||
}
|
||||
|
||||
checkin_proto::AndroidCheckinResponse response_proto;
|
||||
|
||||
net::HttpStatusCode response_status = static_cast<net::HttpStatusCode>(
|
||||
source->ResponseInfo()->headers->response_code());
|
||||
if (response_status == net::HTTP_BAD_REQUEST ||
|
||||
response_status == net::HTTP_UNAUTHORIZED) {
|
||||
// BAD_REQUEST indicates that the request was malformed.
|
||||
// UNAUTHORIZED indicates that security token didn't match the android id.
|
||||
LOG(ERROR) << "No point retrying the checkin with status: "
|
||||
<< response_status << ". Checkin failed.";
|
||||
CheckinRequestStatus status = response_status == net::HTTP_BAD_REQUEST ?
|
||||
HTTP_BAD_REQUEST : HTTP_UNAUTHORIZED;
|
||||
RecordCheckinStatusAndReportUMA(status, recorder_, false);
|
||||
CheckinRequestStatus status = response_status == net::HTTP_BAD_REQUEST
|
||||
? CheckinRequestStatus::kBadRequest
|
||||
: CheckinRequestStatus::kUnauthorized;
|
||||
RecordCheckinStatusAndReportUMA(status, recorder_, /* will_retry= */ false);
|
||||
std::move(callback_).Run(response_status, response_proto);
|
||||
return;
|
||||
}
|
||||
|
||||
if (response_status != net::HTTP_OK || !body ||
|
||||
!response_proto.ParseFromString(*body)) {
|
||||
LOG(ERROR) << "Failed to get checkin response. HTTP Status: "
|
||||
LOG(ERROR) << "Failed to parse checkin response. HTTP Status: "
|
||||
<< response_status << ". Retrying.";
|
||||
CheckinRequestStatus status = response_status != net::HTTP_OK ?
|
||||
HTTP_NOT_OK : RESPONSE_PARSING_FAILED;
|
||||
RecordCheckinStatusAndReportUMA(status, recorder_, true);
|
||||
|
||||
CheckinRequestStatus status =
|
||||
response_status != net::HTTP_OK
|
||||
? CheckinRequestStatus::kStatusNotOK
|
||||
: CheckinRequestStatus::kResponseParsingFailed;
|
||||
RecordCheckinStatusAndReportUMA(status, recorder_, /* will_retry= */ true);
|
||||
RetryWithBackoff();
|
||||
return;
|
||||
}
|
||||
@ -256,13 +283,14 @@ void CheckinRequest::OnURLLoadComplete(const network::SimpleURLLoader* source,
|
||||
!response_proto.has_security_token() ||
|
||||
response_proto.android_id() == 0 ||
|
||||
response_proto.security_token() == 0) {
|
||||
LOG(ERROR) << "Android ID or security token is 0. Retrying.";
|
||||
RecordCheckinStatusAndReportUMA(ZERO_ID_OR_TOKEN, recorder_, true);
|
||||
RecordCheckinStatusAndReportUMA(CheckinRequestStatus::kZeroIdOrToken,
|
||||
recorder_, /* will_retry= */ true);
|
||||
RetryWithBackoff();
|
||||
return;
|
||||
}
|
||||
|
||||
RecordCheckinStatusAndReportUMA(SUCCESS, recorder_, false);
|
||||
RecordCheckinStatusAndReportUMA(CheckinRequestStatus::kSuccess, recorder_,
|
||||
/* will_retry= */ false);
|
||||
std::move(callback_).Run(response_status, response_proto);
|
||||
}
|
||||
|
||||
|
@ -30393,12 +30393,15 @@ Called by update_feature_policy_enum.py.-->
|
||||
|
||||
<enum name="GCMCheckinRequestStatus">
|
||||
<int value="0" label="Success"/>
|
||||
<int value="1" label="URL fetching failed"/>
|
||||
<int value="1" label="URL fetching failed (until M82)"/>
|
||||
<int value="2" label="HTTP bad request"/>
|
||||
<int value="3" label="HTTP unauthorized"/>
|
||||
<int value="4" label="HTTP not OK"/>
|
||||
<int value="5" label="Response parsing failed"/>
|
||||
<int value="6" label="Zero ID or token"/>
|
||||
<int value="7" label="URL fetching failed: network error"/>
|
||||
<int value="8" label="URL fetching failed: no response info"/>
|
||||
<int value="9" label="URL fetching failed: no headers"/>
|
||||
</enum>
|
||||
|
||||
<enum name="GCMClientResult">
|
||||
|
@ -57990,6 +57990,16 @@ reviews. Googlers can read more about this at go/gwsq-gerrit.
|
||||
<summary>Status code of the outcome of a GCM checkin request.</summary>
|
||||
</histogram>
|
||||
|
||||
<histogram name="GCM.CheckinRequestStatusNetError" enum="NetErrorCodes"
|
||||
expires_after="2020-08-30">
|
||||
<owner>peter@chromium.org</owner>
|
||||
<owner>knollr@chromium.org</owner>
|
||||
<summary>
|
||||
Network error code for a GCM checkin request that failed because of a
|
||||
network error.
|
||||
</summary>
|
||||
</histogram>
|
||||
|
||||
<histogram name="GCM.CheckinRetryCount" units="units"
|
||||
expires_after="2018-08-30">
|
||||
<obsolete>
|
||||
|
Reference in New Issue
Block a user