[GCM] Add more UMA to GCM

[GCM] Add more UMA to GCM

Also fix a bug that the default TTL value is set to 0.

BUG=361374, 371149
TEST=new test added for default TTL bug

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

[jianli, 2014-05-08 05:50:06]

Adding asvitkine for UMA.

[fgorski, 2014-05-08 18:08:36]

https://codereview.chromium.org/270783002/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/api/gcm/gcm_api.cc (right):

https://codereview.chromium.org/270783002/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/gcm/gcm_api.cc:135:
UMA_HISTOGRAM_COUNTS("GCM.UnregisterAPICall", 1);
Consider using something like
GCM.APICallXXX
GCM.APIEventXXX

And similar, so that it is easy to filter them out.

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

https://codereview.chromium.org/270783002/diff/40001/google_apis/gcm/engine/c...
google_apis/gcm/engine/checkin_request.cc:223:
UMA_HISTOGRAM_COUNTS("GCM.CheckinRetryNumber",
Do we report try count if the checkin fails or do we attempt to check in
infinitely?

https://codereview.chromium.org/270783002/diff/40001/google_apis/gcm/engine/r...
File google_apis/gcm/engine/registration_request.cc (right):

https://codereview.chromium.org/270783002/diff/40001/google_apis/gcm/engine/r...
google_apis/gcm/engine/registration_request.cc:95:
UMA_HISTOGRAM_COUNTS("GCM.RegistrationSenderNumber", sender_ids.size());
nit: wouldn't that be more suitable in start?

https://codereview.chromium.org/270783002/diff/40001/google_apis/gcm/engine/r...
google_apis/gcm/engine/registration_request.cc:258:
UMA_HISTOGRAM_COUNTS("GCM.RegistrationRetryNumber",
Do we not care about the retry number if we don't end with success?

https://codereview.chromium.org/270783002/diff/40001/google_apis/gcm/engine/r...
google_apis/gcm/engine/registration_request.cc:259:
backoff_entry_.failure_count() + 1);
nit: Technically number of retries is = failure count
number of total tries = failure count + 1

https://codereview.chromium.org/270783002/diff/40001/google_apis/gcm/engine/u...
File google_apis/gcm/engine/unregistration_request.cc (right):

https://codereview.chromium.org/270783002/diff/40001/google_apis/gcm/engine/u...
google_apis/gcm/engine/unregistration_request.cc:130: void
UnregistrationRequest::Start() {
Add UMA about Unregistration attempt.

https://codereview.chromium.org/270783002/diff/40001/google_apis/gcm/engine/u...
google_apis/gcm/engine/unregistration_request.cc:215:
UMA_HISTOGRAM_COUNTS("GCM.UnregistrationRetryNumber",
Same comments as in Registration

https://codereview.chromium.org/270783002/diff/40001/google_apis/gcm/gcm_clie...
File google_apis/gcm/gcm_client.h (right):

https://codereview.chromium.org/270783002/diff/40001/google_apis/gcm/gcm_clie...
google_apis/gcm/gcm_client.h:71: static const int kMaximumTTL = 60 * 60 * 24 * 7
* 4;  // 4 weeks.
nit: we are using "big endian" everywhere, making it 4* 7* 24 * 60 * 60

https://codereview.chromium.org/270783002/diff/40001/google_apis/gcm/gcm_clie...
File google_apis/gcm/gcm_client_impl.cc (right):

https://codereview.chromium.org/270783002/diff/40001/google_apis/gcm/gcm_clie...
google_apis/gcm/gcm_client_impl.cc:73: ZERO_TTL,
TTL_ZERO
TTL_IS_ZERO

and TTL_MAX / TTL_MAXIMUM

to make all of the values consistent?

https://codereview.chromium.org/270783002/diff/40001/google_apis/gcm/gcm_clie...
google_apis/gcm/gcm_client_impl.cc:145: else if (message.time_to_live <= 60 * 60
* 24 * 7)
nit: if you decide to reverse the order in my last comment. I think this can
still stay.

[jianli, 2014-05-08 18:40:07]

https://codereview.chromium.org/270783002/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/api/gcm/gcm_api.cc (right):

https://codereview.chromium.org/270783002/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/gcm/gcm_api.cc:135:
UMA_HISTOGRAM_COUNTS("GCM.UnregisterAPICall", 1);
On 2014/05/08 18:08:36, fgorski wrote:
> Consider using something like
> GCM.APICallXXX
> GCM.APIEventXXX
> 
> And similar, so that it is easy to filter them out.

Done.

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

https://codereview.chromium.org/270783002/diff/40001/google_apis/gcm/engine/c...
google_apis/gcm/engine/checkin_request.cc:223:
UMA_HISTOGRAM_COUNTS("GCM.CheckinRetryNumber",
On 2014/05/08 18:08:36, fgorski wrote:
> Do we report try count if the checkin fails or do we attempt to check in
> infinitely?

What we care about is the number of retries to do the checkin successfully. If
the checkin failed due to HTTP_BAD_REQUEST or HTTP_UNAUTHORIZED, it will be
reported by GCM.CheckinRequestStatus. Then we don't care about the number of
retries since no matter how many the checkin is retried, it should always fail.

https://codereview.chromium.org/270783002/diff/40001/google_apis/gcm/engine/r...
File google_apis/gcm/engine/registration_request.cc (right):

https://codereview.chromium.org/270783002/diff/40001/google_apis/gcm/engine/r...
google_apis/gcm/engine/registration_request.cc:95:
UMA_HISTOGRAM_COUNTS("GCM.RegistrationSenderNumber", sender_ids.size());
On 2014/05/08 18:08:36, fgorski wrote:
> nit: wouldn't that be more suitable in start?

Done.

https://codereview.chromium.org/270783002/diff/40001/google_apis/gcm/engine/r...
google_apis/gcm/engine/registration_request.cc:258:
UMA_HISTOGRAM_COUNTS("GCM.RegistrationRetryNumber",
On 2014/05/08 18:08:36, fgorski wrote:
> Do we not care about the retry number if we don't end with success?

Yes. See my comment above.

https://codereview.chromium.org/270783002/diff/40001/google_apis/gcm/engine/r...
google_apis/gcm/engine/registration_request.cc:259:
backoff_entry_.failure_count() + 1);
On 2014/05/08 18:08:36, fgorski wrote:
> nit: Technically number of retries is = failure count
> number of total tries = failure count + 1

Original I mean the UMA was for total number of requests being asked. But based
on the UMA name, I think we really mean for the total number of retries. If the
checkin succeeds at 1st try, the UMA count should be reported as 0. I updated
the code to report fail_count.

https://codereview.chromium.org/270783002/diff/40001/google_apis/gcm/engine/u...
File google_apis/gcm/engine/unregistration_request.cc (right):

https://codereview.chromium.org/270783002/diff/40001/google_apis/gcm/engine/u...
google_apis/gcm/engine/unregistration_request.cc:130: void
UnregistrationRequest::Start() {
On 2014/05/08 18:08:36, fgorski wrote:
> Add UMA about Unregistration attempt.

What do you mean?

https://codereview.chromium.org/270783002/diff/40001/google_apis/gcm/engine/u...
google_apis/gcm/engine/unregistration_request.cc:215:
UMA_HISTOGRAM_COUNTS("GCM.UnregistrationRetryNumber",
On 2014/05/08 18:08:36, fgorski wrote:
> Same comments as in Registration

ditto.

https://codereview.chromium.org/270783002/diff/40001/google_apis/gcm/gcm_clie...
File google_apis/gcm/gcm_client.h (right):

https://codereview.chromium.org/270783002/diff/40001/google_apis/gcm/gcm_clie...
google_apis/gcm/gcm_client.h:71: static const int kMaximumTTL = 60 * 60 * 24 * 7
* 4;  // 4 weeks.
On 2014/05/08 18:08:36, fgorski wrote:
> nit: we are using "big endian" everywhere, making it 4* 7* 24 * 60 * 60

Done.

https://codereview.chromium.org/270783002/diff/40001/google_apis/gcm/gcm_clie...
File google_apis/gcm/gcm_client_impl.cc (right):

https://codereview.chromium.org/270783002/diff/40001/google_apis/gcm/gcm_clie...
google_apis/gcm/gcm_client_impl.cc:73: ZERO_TTL,
On 2014/05/08 18:08:36, fgorski wrote:
> TTL_ZERO
> TTL_IS_ZERO
> 
> and TTL_MAX / TTL_MAXIMUM
> 
> to make all of the values consistent?

Done.

https://codereview.chromium.org/270783002/diff/40001/google_apis/gcm/gcm_clie...
google_apis/gcm/gcm_client_impl.cc:145: else if (message.time_to_live <= 60 * 60
* 24 * 7)
On 2014/05/08 18:08:36, fgorski wrote:
> nit: if you decide to reverse the order in my last comment. I think this can
> still stay.

Done.

[fgorski, 2014-05-08 18:46:29]

lgtm

https://codereview.chromium.org/270783002/diff/40001/google_apis/gcm/engine/u...
File google_apis/gcm/engine/unregistration_request.cc (right):

https://codereview.chromium.org/270783002/diff/40001/google_apis/gcm/engine/u...
google_apis/gcm/engine/unregistration_request.cc:130: void
UnregistrationRequest::Start() {
On 2014/05/08 18:40:07, jianli wrote:
> On 2014/05/08 18:08:36, fgorski wrote:
> > Add UMA about Unregistration attempt.
> 
> What do you mean?

My mistake, I was going for being symmetric with Registration request, but you
are logging the number of senders there, so my comment does not make sense.

[Alexei Svitkine (slow), 2014-05-08 21:21:57]

https://codereview.chromium.org/270783002/diff/60001/chrome/browser/extension...
File chrome/browser/extensions/api/gcm/gcm_api.cc (right):

https://codereview.chromium.org/270783002/diff/60001/chrome/browser/extension...
chrome/browser/extensions/api/gcm/gcm_api.cc:135:
UMA_HISTOGRAM_COUNTS("GCM.APICallUnregister", 1);
Use an UMA_HISTOGRAM_BOOLEAN() instead. The resulting histogram will be the
same, but it will just allocate 2 buckets instead of 50, so a memory saving.

https://codereview.chromium.org/270783002/diff/60001/tools/metrics/histograms...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/270783002/diff/60001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:7177: +  <summary>Number of senders
specified in registration request.</summary>
Nit: "in a GCM registration request"

[jianli, 2014-05-08 21:26:07]

https://codereview.chromium.org/270783002/diff/60001/chrome/browser/extension...
File chrome/browser/extensions/api/gcm/gcm_api.cc (right):

https://codereview.chromium.org/270783002/diff/60001/chrome/browser/extension...
chrome/browser/extensions/api/gcm/gcm_api.cc:135:
UMA_HISTOGRAM_COUNTS("GCM.APICallUnregister", 1);
On 2014/05/08 21:21:57, Alexei Svitkine wrote:
> Use an UMA_HISTOGRAM_BOOLEAN() instead. The resulting histogram will be the
> same, but it will just allocate 2 buckets instead of 50, so a memory saving.

Done.

https://codereview.chromium.org/270783002/diff/60001/tools/metrics/histograms...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/270783002/diff/60001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:7177: +  <summary>Number of senders
specified in registration request.</summary>
On 2014/05/08 21:21:57, Alexei Svitkine wrote:
> Nit: "in a GCM registration request"

Done.

[Nicolas Zea, 2014-05-08 21:28:19]

LGTM with some naming nits

https://codereview.chromium.org/270783002/diff/60001/google_apis/gcm/engine/r...
File google_apis/gcm/engine/registration_request.cc (right):

https://codereview.chromium.org/270783002/diff/60001/google_apis/gcm/engine/r...
google_apis/gcm/engine/registration_request.cc:154:
UMA_HISTOGRAM_COUNTS("GCM.RegistrationSenderNumber",
I think RegistrationSenderIdCount is more accurate/readable than SenderNumber.

https://codereview.chromium.org/270783002/diff/60001/google_apis/gcm/engine/r...
google_apis/gcm/engine/registration_request.cc:259:
UMA_HISTOGRAM_COUNTS("GCM.RegistrationRetryNumber",
Number -> Count here too

https://codereview.chromium.org/270783002/diff/60001/google_apis/gcm/engine/u...
File google_apis/gcm/engine/unregistration_request.cc (right):

https://codereview.chromium.org/270783002/diff/60001/google_apis/gcm/engine/u...
google_apis/gcm/engine/unregistration_request.cc:215:
UMA_HISTOGRAM_COUNTS("GCM.UnregistrationRetryNumber",
Number -> Count

https://codereview.chromium.org/270783002/diff/60001/google_apis/gcm/gcm_clie...
File google_apis/gcm/gcm_client_impl.cc (right):

https://codereview.chromium.org/270783002/diff/60001/google_apis/gcm/gcm_clie...
google_apis/gcm/gcm_client_impl.cc:597: RecordROutgoingMessageToUMA(message);
RecordROutgoingMessageToUma -> remove the extra R

[Alexei Svitkine (slow), 2014-05-08 21:36:24]

lgtm % zea's comments

When renaming the histograms per his comments, please make sure to update both
the xml file and code. Thanks!

[jianli, 2014-05-08 21:40:46]

All done. Will put the revised patch in CQ.

https://codereview.chromium.org/270783002/diff/60001/google_apis/gcm/engine/r...
File google_apis/gcm/engine/registration_request.cc (right):

https://codereview.chromium.org/270783002/diff/60001/google_apis/gcm/engine/r...
google_apis/gcm/engine/registration_request.cc:154:
UMA_HISTOGRAM_COUNTS("GCM.RegistrationSenderNumber",
On 2014/05/08 21:28:19, Nicolas Zea wrote:
> I think RegistrationSenderIdCount is more accurate/readable than SenderNumber.

Done.

https://codereview.chromium.org/270783002/diff/60001/google_apis/gcm/engine/r...
google_apis/gcm/engine/registration_request.cc:259:
UMA_HISTOGRAM_COUNTS("GCM.RegistrationRetryNumber",
On 2014/05/08 21:28:19, Nicolas Zea wrote:
> Number -> Count here too

Done.

https://codereview.chromium.org/270783002/diff/60001/google_apis/gcm/engine/u...
File google_apis/gcm/engine/unregistration_request.cc (right):

https://codereview.chromium.org/270783002/diff/60001/google_apis/gcm/engine/u...
google_apis/gcm/engine/unregistration_request.cc:215:
UMA_HISTOGRAM_COUNTS("GCM.UnregistrationRetryNumber",
On 2014/05/08 21:28:19, Nicolas Zea wrote:
> Number -> Count

Done.

https://codereview.chromium.org/270783002/diff/60001/google_apis/gcm/gcm_clie...
File google_apis/gcm/gcm_client_impl.cc (right):

https://codereview.chromium.org/270783002/diff/60001/google_apis/gcm/gcm_clie...
google_apis/gcm/gcm_client_impl.cc:597: RecordROutgoingMessageToUMA(message);
On 2014/05/08 21:28:19, Nicolas Zea wrote:
> RecordROutgoingMessageToUma -> remove the extra R

Done.

[jianli, 2014-05-08 21:47:17]

The CQ bit was checked by jianli@chromium.org

[commit-bot: I haz the power, 2014-05-08 21:53:20]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jianli@chromium.org/270783002/70016

[commit-bot: I haz the power, 2014-05-09 03:25:16]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  linux_chromium_chromeos_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_clang_...)
  linux_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)
  mac_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_compile_...)
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)

[commit-bot: I haz the power, 2014-05-09 04:33:52]

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

[commit-bot: I haz the power, 2014-05-09 04:33:52]

Try jobs failed on following builders:
  linux_chromium_chromeos_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)

[jianli, 2014-05-09 17:04:24]

The CQ bit was checked by jianli@chromium.org

[commit-bot: I haz the power, 2014-05-09 17:09:57]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jianli@chromium.org/270783002/110001

[commit-bot: I haz the power, 2014-05-09 22:34:11]

Change committed as 269439