GCM: Add Android UMA, and UMA for deleted/collapsed messages

GCM: Add Android UMA, and UMA for deleted/collapsed messages

- Adds GCM.DeletedMessagesReceived UMA for incoming DELETED_MESSAGES,
  which happens when the 100 non-collapsible messages limit is exceeded
  on the GCM server.

- Adds GCM.DataMessageReceivedHasCollapseKey UMA for incoming messages
  with collapse_key, giving a baseline to compare the previous UMA with.

- Makes the existing GCM.DataMessageReceivedHasRegisteredApp and
  GCM.DataMessageReceived UMAs only count DATA_MESSAGE rather than
  DELETED_MESSAGES, to better match their names (and be more useful).

- Also adds all of the above UMAs to Android, which used to have no UMA.

BUG=672067
Committed: https://crrev.com/409dc538bb1495c458a72f7e93eb9e1dab102036
Cr-Commit-Position: refs/heads/master@{#438339}

[johnme, 2016-12-06 19:25:35]

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

[johnme, 2016-12-06 19:25:35]

Haven't yet fixed up tests, sending out early to validate approach.

[Peter Beverloo, 2016-12-07 13:44:12]

Can we significantly simplify this by counting the histograms directly in
GCMClientImpl?

While it's a noble goal to histogram counting to a single file, there's a
costs/gains benefit we have to make. There is no code being deduplicated here,
so I'm going to argue we should favor readability and keep it inline.

https://codereview.chromium.org/2558553002/diff/1/components/gcm_driver/gcm_c...
File components/gcm_driver/gcm_client_impl.cc (right):

https://codereview.chromium.org/2558553002/diff/1/components/gcm_driver/gcm_c...
components/gcm_driver/gcm_client_impl.cc:1350:
delegate_->OnMessagesDeleted(app_id);
nit: please split this block out into an HandleIncomingDeletedMessages method.

https://codereview.chromium.org/2558553002/diff/1/components/gcm_driver/gcm_s...
File components/gcm_driver/gcm_stats_recorder_impl.cc (right):

https://codereview.chromium.org/2558553002/diff/1/components/gcm_driver/gcm_s...
components/gcm_driver/gcm_stats_recorder_impl.cc:423:
UMA_HISTOGRAM_BOOLEAN("GCM.DataMessageReceivedHasRegisteredApp",
This means that we're not recording this histogram anymore for deleted messages
messages.

[johnme, 2016-12-07 14:29:16]

Addressed review comments. New/modified histograms are moved to GCMClientImpl as
discussed.

https://codereview.chromium.org/2558553002/diff/1/components/gcm_driver/gcm_c...
File components/gcm_driver/gcm_client_impl.cc (right):

https://codereview.chromium.org/2558553002/diff/1/components/gcm_driver/gcm_c...
components/gcm_driver/gcm_client_impl.cc:1350:
delegate_->OnMessagesDeleted(app_id);
On 2016/12/07 13:44:12, Peter Beverloo wrote:
> nit: please split this block out into an HandleIncomingDeletedMessages method.

Done.

https://codereview.chromium.org/2558553002/diff/1/components/gcm_driver/gcm_s...
File components/gcm_driver/gcm_stats_recorder_impl.cc (right):

https://codereview.chromium.org/2558553002/diff/1/components/gcm_driver/gcm_s...
components/gcm_driver/gcm_stats_recorder_impl.cc:423:
UMA_HISTOGRAM_BOOLEAN("GCM.DataMessageReceivedHasRegisteredApp",
On 2016/12/07 13:44:12, Peter Beverloo wrote:
> This means that we're not recording this histogram anymore for deleted
messages
> messages.

That's deliberate - we don't really care about messages getting deleted for
non-existing registrations, and those registrations will still show up in UMA
stats when receive non-deleted messages; however it would be problematic (and
confusing given the name of the UMA) if the DataMessageReceivedHasRegisteredApp
count/ratio were skewed by DELETED_MESSAGES messages.

In practice I don't expect the UMA graphs to jump since DELETED_MESSAGES should
be very infrequent, but you'll see I added a comment to the histograms noting
when this changed.

[johnme, 2016-12-07 14:29:25]

Description was changed from

==========
GCM: Add UMA for deleted messages and collapse_keys

FIXME: Fix tests

BUG=none
==========

to

==========
GCM: Add UMA for deleted messages and collapse_keys

BUG=none
==========

[johnme, 2016-12-07 14:35:06]

Description was changed from

==========
GCM: Add UMA for deleted messages and collapse_keys

BUG=none
==========

to

==========
GCM: Add UMA for deleted messages and collapse_keys

BUG=672067
==========

[johnme, 2016-12-07 15:12:09]

Description was changed from

==========
GCM: Add UMA for deleted messages and collapse_keys

BUG=672067
==========

to

==========
GCM: Add UMA for deleted messages, collapse_keys and Android

BUG=672067
==========

[johnme, 2016-12-07 15:32:02]

Description was changed from

==========
GCM: Add UMA for deleted messages, collapse_keys and Android

BUG=672067
==========

to

==========
GCM: Add Android UMA, and UMA for deleted/collapsed messages

- Adds GCM.DeletedMessagesReceived UMA for incoming DELETED_MESSAGES,
  which happens when the 100 non-collapsible messages limit is exceeded
  on the GCM server.

- Adds GCM.DataMessageReceivedHasRegisteredApp UMA for incoming messages
  with collapse_key, giving a baseline to compare the previous UMA with.

- Makes the existing GCM.DataMessageBurstReceivedInterval and
  GCM.DataMessageReceived UMAs only count DATA_MESSAGE rather than
  DELETED_MESSAGES, to better match their names (and be more useful).

- Also adds all of the above UMAs to Android, which used to have no UMA.

BUG=672067
==========

[johnme, 2016-12-07 18:54:53]

Description was changed from

==========
GCM: Add Android UMA, and UMA for deleted/collapsed messages

- Adds GCM.DeletedMessagesReceived UMA for incoming DELETED_MESSAGES,
  which happens when the 100 non-collapsible messages limit is exceeded
  on the GCM server.

- Adds GCM.DataMessageReceivedHasRegisteredApp UMA for incoming messages
  with collapse_key, giving a baseline to compare the previous UMA with.

- Makes the existing GCM.DataMessageBurstReceivedInterval and
  GCM.DataMessageReceived UMAs only count DATA_MESSAGE rather than
  DELETED_MESSAGES, to better match their names (and be more useful).

- Also adds all of the above UMAs to Android, which used to have no UMA.

BUG=672067
==========

to

==========
GCM: Add Android UMA, and UMA for deleted/collapsed messages

- Adds GCM.DeletedMessagesReceived UMA for incoming DELETED_MESSAGES,
  which happens when the 100 non-collapsible messages limit is exceeded
  on the GCM server.

- Adds GCM.DataMessageReceivedHasCollapseKey UMA for incoming messages
  with collapse_key, giving a baseline to compare the previous UMA with.

- Makes the existing GCM.DataMessageReceivedHasRegisteredApp and
  GCM.DataMessageReceived UMAs only count DATA_MESSAGE rather than
  DELETED_MESSAGES, to better match their names (and be more useful).

- Also adds all of the above UMAs to Android, which used to have no UMA.

BUG=672067
==========

[Peter Beverloo, 2016-12-07 19:36:21]

lgtm

https://codereview.chromium.org/2558553002/diff/20001/components/gcm_driver/g...
File components/gcm_driver/gcm_client_impl.cc (right):

https://codereview.chromium.org/2558553002/diff/20001/components/gcm_driver/g...
components/gcm_driver/gcm_client_impl.cc:1433: if
(!base::StringToInt(count_iter->second, &deleted_count))
Should we LOG(WARN)? I'm not sure if that's useful. This means GCM sent us
invalid data...

https://codereview.chromium.org/2558553002/diff/20001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2558553002/diff/20001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:18889: +    registered application IDs,
this records the provided count of how many
"... registered application IDs..." - Is this correct? We don't apply the
is-registered-app logic that we have for incoming data messages to incoming
deleted messages messages.

[johnme, 2016-12-07 20:00:38]

Addressed review comments

https://codereview.chromium.org/2558553002/diff/20001/components/gcm_driver/g...
File components/gcm_driver/gcm_client_impl.cc (right):

https://codereview.chromium.org/2558553002/diff/20001/components/gcm_driver/g...
components/gcm_driver/gcm_client_impl.cc:1433: if
(!base::StringToInt(count_iter->second, &deleted_count))
On 2016/12/07 19:36:20, Peter Beverloo wrote:
> Should we LOG(WARN)? I'm not sure if that's useful. This means GCM sent us
> invalid data...

Well, or just that GCM changed the format of this data. It's just an app data
key-value pair, so potentially less stable than protobuf fields.

Logging zero (unknown count, which is what we always log on Android) is a fine
fallback, and we'll be able to see from UMA if GCM stops giving us sensible
counts, though realistically it won't be a high priority fix.

https://codereview.chromium.org/2558553002/diff/20001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2558553002/diff/20001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:18889: +    registered application IDs,
this records the provided count of how many
On 2016/12/07 19:36:20, Peter Beverloo wrote:
> "... registered application IDs..." - Is this correct? We don't apply the
> is-registered-app logic that we have for incoming data messages to incoming
> deleted messages messages.

Done - in fact reworded a bunch of these to be clearer (since the term
"application ID" is rather unusual, and also not technically accurate since the
sender ID also needs to match for IID registrations).

[johnme, 2016-12-07 20:03:08]

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

[johnme, 2016-12-07 20:03:08]

isherman: please review histogram changes

zea: please approve google_apis/gcm/monitoring/gcm_stats_recorder.h (though you
may also be interested in the rest of the patch)

[Nicolas Zea, 2016-12-07 20:58:50]

lgtm

[Ilya Sherman, 2016-12-08 06:54:56]

https://codereview.chromium.org/2558553002/diff/40001/components/gcm_driver/g...
File components/gcm_driver/gcm_client_impl.cc (right):

https://codereview.chromium.org/2558553002/diff/40001/components/gcm_driver/g...
components/gcm_driver/gcm_client_impl.cc:1404:
UMA_HISTOGRAM_COUNTS("GCM.DataMessageReceived", 1);
Hmm, this is a very odd histogram -- it allocates 50 buckets, and then only uses
one of them.  Is this histogram actually used?  Could it be restructured as an
UMA_HISTOGRAM_BOOLEAN instead?  (I *think* the bucket layouts are compatible for
the first bucket, though I'm not 100% sure.)

https://codereview.chromium.org/2558553002/diff/40001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2558553002/diff/40001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:18860: +    (whether or not a
registration existed).
Typically I'd recommend renaming a histogram when its meaning changes.  I'd
encourage you to do so in this case as well, unless you have a strong reason not
to.

[johnme, 2016-12-12 20:43:33]

Thanks both; Ilya, I've replied to your comments.

https://codereview.chromium.org/2558553002/diff/40001/components/gcm_driver/g...
File components/gcm_driver/gcm_client_impl.cc (right):

https://codereview.chromium.org/2558553002/diff/40001/components/gcm_driver/g...
components/gcm_driver/gcm_client_impl.cc:1404:
UMA_HISTOGRAM_COUNTS("GCM.DataMessageReceived", 1);
On 2016/12/08 06:54:56, Ilya Sherman wrote:
> Hmm, this is a very odd histogram -- it allocates 50 buckets, and then only
uses
> one of them.  Is this histogram actually used?  Could it be restructured as an
> UMA_HISTOGRAM_BOOLEAN instead?  (I *think* the bucket layouts are compatible
for
> the first bucket, though I'm not 100% sure.)

I'd be happy to do so if that's safe. How would I check if the bucket layouts
are compatible?

https://codereview.chromium.org/2558553002/diff/40001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2558553002/diff/40001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:18860: +    (whether or not a
registration existed).
On 2016/12/08 06:54:56, Ilya Sherman wrote:
> Typically I'd recommend renaming a histogram when its meaning changes.  I'd
> encourage you to do so in this case as well, unless you have a strong reason
not
> to.

Yes, I've renamed things in the past. But in this case the DELETED_MESSAGES case
is believed to be such a tiny fraction of the DATA_MESSAGE volume, that this
shouldn't cause any meaningful jump in the graph. Or to put it another way, the
effective meaning of the metric isn't changing, this should just make its
interpretation simpler.

[Ilya Sherman, 2016-12-12 21:07:16]

https://codereview.chromium.org/2558553002/diff/40001/components/gcm_driver/g...
File components/gcm_driver/gcm_client_impl.cc (right):

https://codereview.chromium.org/2558553002/diff/40001/components/gcm_driver/g...
components/gcm_driver/gcm_client_impl.cc:1404:
UMA_HISTOGRAM_COUNTS("GCM.DataMessageReceived", 1);
On 2016/12/12 20:43:33, johnme wrote:
> On 2016/12/08 06:54:56, Ilya Sherman wrote:
> > Hmm, this is a very odd histogram -- it allocates 50 buckets, and then only
> uses
> > one of them.  Is this histogram actually used?  Could it be restructured as
an
> > UMA_HISTOGRAM_BOOLEAN instead?  (I *think* the bucket layouts are compatible
> for
> > the first bucket, though I'm not 100% sure.)
> 
> I'd be happy to do so if that's safe. How would I check if the bucket layouts
> are compatible?

Thanks!  I double-checked, and it does seem safe.  For example, you can view the
histogram bucket min and max values by viewing this histogram on the histograms
dashboard.  The bucket min is 1, and the max is 2, which is compatible with
UMA_HISTOGRAM_BOOLEAN("GCM.DataMessageReceived", true).

[johnme, 2016-12-13 16:04:34]

https://codereview.chromium.org/2558553002/diff/40001/components/gcm_driver/g...
File components/gcm_driver/gcm_client_impl.cc (right):

https://codereview.chromium.org/2558553002/diff/40001/components/gcm_driver/g...
components/gcm_driver/gcm_client_impl.cc:1404:
UMA_HISTOGRAM_COUNTS("GCM.DataMessageReceived", 1);
On 2016/12/12 21:07:16, Ilya Sherman wrote:
> On 2016/12/12 20:43:33, johnme wrote:
> > On 2016/12/08 06:54:56, Ilya Sherman wrote:
> > > Hmm, this is a very odd histogram -- it allocates 50 buckets, and then
only
> > uses
> > > one of them.  Is this histogram actually used?  Could it be restructured
as
> an
> > > UMA_HISTOGRAM_BOOLEAN instead?  (I *think* the bucket layouts are
compatible
> > for
> > > the first bucket, though I'm not 100% sure.)
> > 
> > I'd be happy to do so if that's safe. How would I check if the bucket
layouts
> > are compatible?
> 
> Thanks!  I double-checked, and it does seem safe.  For example, you can view
the
> histogram bucket min and max values by viewing this histogram on the
histograms
> dashboard.  The bucket min is 1, and the max is 2, which is compatible with
> UMA_HISTOGRAM_BOOLEAN("GCM.DataMessageReceived", true).

Thanks, done. I updated the xml accordingly to be enum="BooleanReceived" (which
already existed).

[Ilya Sherman, 2016-12-13 21:13:15]

Metrics LGTM, thanks.

[johnme, 2016-12-13 22:27:32]

The CQ bit was checked by johnme@chromium.org

[johnme, 2016-12-13 22:27:33]

The patchset sent to the CQ was uploaded after l-g-t-m from peter@chromium.org,
zea@chromium.org
Link to the patchset: https://codereview.chromium.org/2558553002/#ps60001
(title: "UMA_HISTOGRAM_BOOLEAN("GCM.DataMessageReceived", true);")

[commit-bot: I haz the power, 2016-12-13 22:28:12]

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-12-13 23:43:50]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1481668052908220,
"parent_rev": "9253203197421f6b46702bd70d8c7f37743952ce", "commit_rev":
"7aee73d522469c8be9bc118eb506d4ba69a564df"}

[commit-bot: I haz the power, 2016-12-13 23:44:15]

Description was changed from

==========
GCM: Add Android UMA, and UMA for deleted/collapsed messages

- Adds GCM.DeletedMessagesReceived UMA for incoming DELETED_MESSAGES,
  which happens when the 100 non-collapsible messages limit is exceeded
  on the GCM server.

- Adds GCM.DataMessageReceivedHasCollapseKey UMA for incoming messages
  with collapse_key, giving a baseline to compare the previous UMA with.

- Makes the existing GCM.DataMessageReceivedHasRegisteredApp and
  GCM.DataMessageReceived UMAs only count DATA_MESSAGE rather than
  DELETED_MESSAGES, to better match their names (and be more useful).

- Also adds all of the above UMAs to Android, which used to have no UMA.

BUG=672067
==========

to

==========
GCM: Add Android UMA, and UMA for deleted/collapsed messages

- Adds GCM.DeletedMessagesReceived UMA for incoming DELETED_MESSAGES,
  which happens when the 100 non-collapsible messages limit is exceeded
  on the GCM server.

- Adds GCM.DataMessageReceivedHasCollapseKey UMA for incoming messages
  with collapse_key, giving a baseline to compare the previous UMA with.

- Makes the existing GCM.DataMessageReceivedHasRegisteredApp and
  GCM.DataMessageReceived UMAs only count DATA_MESSAGE rather than
  DELETED_MESSAGES, to better match their names (and be more useful).

- Also adds all of the above UMAs to Android, which used to have no UMA.

BUG=672067

Review-Url: https://codereview.chromium.org/2558553002
==========

[commit-bot: I haz the power, 2016-12-13 23:44:16]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-12-13 23:47:14]

Description was changed from

==========
GCM: Add Android UMA, and UMA for deleted/collapsed messages

- Adds GCM.DeletedMessagesReceived UMA for incoming DELETED_MESSAGES,
  which happens when the 100 non-collapsible messages limit is exceeded
  on the GCM server.

- Adds GCM.DataMessageReceivedHasCollapseKey UMA for incoming messages
  with collapse_key, giving a baseline to compare the previous UMA with.

- Makes the existing GCM.DataMessageReceivedHasRegisteredApp and
  GCM.DataMessageReceived UMAs only count DATA_MESSAGE rather than
  DELETED_MESSAGES, to better match their names (and be more useful).

- Also adds all of the above UMAs to Android, which used to have no UMA.

BUG=672067

Review-Url: https://codereview.chromium.org/2558553002
==========

to

==========
GCM: Add Android UMA, and UMA for deleted/collapsed messages

- Adds GCM.DeletedMessagesReceived UMA for incoming DELETED_MESSAGES,
  which happens when the 100 non-collapsible messages limit is exceeded
  on the GCM server.

- Adds GCM.DataMessageReceivedHasCollapseKey UMA for incoming messages
  with collapse_key, giving a baseline to compare the previous UMA with.

- Makes the existing GCM.DataMessageReceivedHasRegisteredApp and
  GCM.DataMessageReceived UMAs only count DATA_MESSAGE rather than
  DELETED_MESSAGES, to better match their names (and be more useful).

- Also adds all of the above UMAs to Android, which used to have no UMA.

BUG=672067

Committed: https://crrev.com/409dc538bb1495c458a72f7e93eb9e1dab102036
Cr-Commit-Position: refs/heads/master@{#438339}
==========

[commit-bot: I haz the power, 2016-12-13 23:47:15]

Patchset 4 (id:??) landed as
https://crrev.com/409dc538bb1495c458a72f7e93eb9e1dab102036
Cr-Commit-Position: refs/heads/master@{#438339}