GCM Engine: Split up reg/unreg UNKNOWN_ERROR to improve metrics

GCM Engine: Split up reg/unreg UNKNOWN_ERROR to improve metrics

When the GCM server handles registration/unregistration requests, it can
return several specific error names that Chrome's GCM Engine was lumping
together as UNKNOWN_ERROR. This patch splits them out into distinct enum
values, so we'll be able to compare their occurrence rates using UMA and
get more precise bug reports from users reading chrome://gcm-internals.

UNKNOWN_ERROR should no longer occur... until GCM next add new error
types anyway.

The patch also removes some code duplication between
GCMUnregistrationRequestHandler and InstanceIDDeleteTokenRequestHandler,
but it stops short of merging RegistrationRequest and
UnregistrationRequest since this patch may need to be merged to m55.

There should be minimal functional changes, except that we will no
longer auto-retry QUOTA_EXCEEDED and TOO_MANY_REGISTRATIONS registration
errors, which were unlikely to succeed when retried anyway.

BUG=623062
Committed: https://crrev.com/5f4601a5426efc4d350963048122d053128dbc0e
Cr-Commit-Position: refs/heads/master@{#429254}

[johnme, 2016-10-20 17:05:29]

johnme@chromium.org changed reviewers:
+ zea@chromium.org

[Nicolas Zea, 2016-10-20 21:05:23]

LGTM with some nits. I also wonder if it's worth mentioning in the histogram
description what version started recording these new values? That way anyone
looking at the  histogram won't be confused by the sudden decrease in
UNKNOWN_ERRORS?

https://codereview.chromium.org/2434243002/diff/1/google_apis/gcm/engine/gcm_...
File google_apis/gcm/engine/gcm_unregistration_request_handler.h (right):

https://codereview.chromium.org/2434243002/diff/1/google_apis/gcm/engine/gcm_...
google_apis/gcm/engine/gcm_unregistration_request_handler.h:23:
UnregistrationRequest::Status ParseResponse(std::string response) override;
make this const ref? (same with other methods now taking a string param)

https://codereview.chromium.org/2434243002/diff/1/google_apis/gcm/engine/regi...
File google_apis/gcm/engine/registration_request.cc (right):

https://codereview.chromium.org/2434243002/diff/1/google_apis/gcm/engine/regi...
google_apis/gcm/engine/registration_request.cc:48: const char
kInternalServerError[] = "InternalServerError";
Is this defined server-side? Seems odd that the format is CamelCase rather than
ALL_CAPS

https://codereview.chromium.org/2434243002/diff/1/google_apis/gcm/engine/unre...
File google_apis/gcm/engine/unregistration_request.cc (right):

https://codereview.chromium.org/2434243002/diff/1/google_apis/gcm/engine/unre...
google_apis/gcm/engine/unregistration_request.cc:43: const char
kPhoneRegistrationError[] = "PHONE_REGISTRATION_ERROR";
nit: Given this maps to DEVICE_REGISTRATION_ERROR, I think it's cleaner to have
the variable name here b kDeviceRegistrationError

[johnme, 2016-11-01 14:15:40]

Addressed review comments - thanks :)

> I also wonder if it's worth mentioning in the histogram
description what version started recording these new values?

Makes sense. I commented this as "M56 (merged mid-cycle to M55)" so that it can
be merged with a clean cherry-pick. If it doesn't get merged then I'll update
the comment on master.

https://codereview.chromium.org/2434243002/diff/1/google_apis/gcm/engine/gcm_...
File google_apis/gcm/engine/gcm_unregistration_request_handler.h (right):

https://codereview.chromium.org/2434243002/diff/1/google_apis/gcm/engine/gcm_...
google_apis/gcm/engine/gcm_unregistration_request_handler.h:23:
UnregistrationRequest::Status ParseResponse(std::string response) override;
On 2016/10/20 21:05:22, Nicolas Zea wrote:
> make this const ref? (same with other methods now taking a string param)

Ahem, must have been a long day. Done.

https://codereview.chromium.org/2434243002/diff/1/google_apis/gcm/engine/regi...
File google_apis/gcm/engine/registration_request.cc (right):

https://codereview.chromium.org/2434243002/diff/1/google_apis/gcm/engine/regi...
google_apis/gcm/engine/registration_request.cc:48: const char
kInternalServerError[] = "InternalServerError";
On 2016/10/20 21:05:23, Nicolas Zea wrote:
> Is this defined server-side? Seems odd that the format is CamelCase rather
than
> ALL_CAPS

Yup, this one just happens to be CamelCase for no apparent reason:
https://goto.google.com/iepyy

https://codereview.chromium.org/2434243002/diff/1/google_apis/gcm/engine/unre...
File google_apis/gcm/engine/unregistration_request.cc (right):

https://codereview.chromium.org/2434243002/diff/1/google_apis/gcm/engine/unre...
google_apis/gcm/engine/unregistration_request.cc:43: const char
kPhoneRegistrationError[] = "PHONE_REGISTRATION_ERROR";
On 2016/10/20 21:05:23, Nicolas Zea wrote:
> nit: Given this maps to DEVICE_REGISTRATION_ERROR, I think it's cleaner to
have
> the variable name here b kDeviceRegistrationError

Done.

[johnme, 2016-11-01 14:16:18]

johnme@chromium.org changed reviewers:
+ isherman@chromium.org

[johnme, 2016-11-01 14:16:19]

isherman: please review histograms - thanks :)

[Ilya Sherman, 2016-11-01 21:26:08]

histograms lgtm.  Typically I'd recommend renaming the histogram for a semantic
change like this; but if it's important to you to preserve the existing name,
then updating the description is ok.

[johnme, 2016-11-02 11:18:27]

The CQ bit was checked by johnme@chromium.org

[johnme, 2016-11-02 11:18:27]

The patchset sent to the CQ was uploaded after l-g-t-m from zea@chromium.org
Link to the patchset: https://codereview.chromium.org/2434243002/#ps20001
(title: "Address review comments")

[commit-bot: I haz the power, 2016-11-02 11:18:32]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[johnme, 2016-11-02 11:18:37]

The CQ bit was unchecked by johnme@chromium.org

[johnme, 2016-11-02 11:20:13]

The CQ bit was checked by johnme@chromium.org

[johnme, 2016-11-02 11:20:14]

The patchset sent to the CQ was uploaded after l-g-t-m from zea@chromium.org,
isherman@chromium.org
Link to the patchset: https://codereview.chromium.org/2434243002/#ps40001
(title: "mid-cycle -> mid-beta")

[commit-bot: I haz the power, 2016-11-02 11:20:23]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-02 12:22:56]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-11-02 12:24:45]

Description was changed from

==========
GCM Engine: Split up reg/unreg UNKNOWN_ERROR to improve metrics

When the GCM server handles registration/unregistration requests, it can
return several specific error names that Chrome's GCM Engine was lumping
together as UNKNOWN_ERROR. This patch splits them out into distinct enum
values, so we'll be able to compare their occurrence rates using UMA and
get more precise bug reports from users reading chrome://gcm-internals.

UNKNOWN_ERROR should no longer occur... until GCM next add new error
types anyway.

The patch also removes some code duplication between
GCMUnregistrationRequestHandler and InstanceIDDeleteTokenRequestHandler,
but it stops short of merging RegistrationRequest and
UnregistrationRequest since this patch may need to be merged to m55.

There should be minimal functional changes, except that we will no
longer auto-retry QUOTA_EXCEEDED and TOO_MANY_REGISTRATIONS registration
errors, which were unlikely to succeed when retried anyway.

BUG=623062
==========

to

==========
GCM Engine: Split up reg/unreg UNKNOWN_ERROR to improve metrics

When the GCM server handles registration/unregistration requests, it can
return several specific error names that Chrome's GCM Engine was lumping
together as UNKNOWN_ERROR. This patch splits them out into distinct enum
values, so we'll be able to compare their occurrence rates using UMA and
get more precise bug reports from users reading chrome://gcm-internals.

UNKNOWN_ERROR should no longer occur... until GCM next add new error
types anyway.

The patch also removes some code duplication between
GCMUnregistrationRequestHandler and InstanceIDDeleteTokenRequestHandler,
but it stops short of merging RegistrationRequest and
UnregistrationRequest since this patch may need to be merged to m55.

There should be minimal functional changes, except that we will no
longer auto-retry QUOTA_EXCEEDED and TOO_MANY_REGISTRATIONS registration
errors, which were unlikely to succeed when retried anyway.

BUG=623062

Committed: https://crrev.com/5f4601a5426efc4d350963048122d053128dbc0e
Cr-Commit-Position: refs/heads/master@{#429254}
==========

[commit-bot: I haz the power, 2016-11-02 12:24:46]

Patchset 3 (id:??) landed as
https://crrev.com/5f4601a5426efc4d350963048122d053128dbc0e
Cr-Commit-Position: refs/heads/master@{#429254}

[johnme, 2016-11-03 18:37:09]

Cherry-picked to M55 in https://codereview.chromium.org/2474153002 as
5f4601a5426efc4d350963048122d053128dbc0e 2883@{#441}