[GCM] Track connection failures properly

[GCM] Track connection failures properly

This patch consolidates the reset logic and adds a reason for the reset,
for use in histograms. Additionally, connection uptime and overall success
rate are now tracked as well.

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

[Nicolas Zea, 2014-02-12 18:56:34]

PTAL

[fgorski, 2014-02-12 19:24:01]

a couple of comments.

https://codereview.chromium.org/160703002/diff/30001/google_apis/gcm/engine/c...
File google_apis/gcm/engine/connection_factory_impl.cc (right):

https://codereview.chromium.org/160703002/diff/30001/google_apis/gcm/engine/c...
google_apis/gcm/engine/connection_factory_impl.cc:118:
UMA_HISTOGRAM_ENUMERATION("GCM.ConnectionResetReason",
Perhaps we could wrap that in LogConnectionResetReason so that we can laster add
diagnostic observers for GCM-internals in the Logging methods. It applies to
other UMA Methods, like the whole if below (could be LogConnectionUptime or
something like that)

https://codereview.chromium.org/160703002/diff/30001/google_apis/gcm/engine/c...
File google_apis/gcm/engine/connection_factory_impl.h (right):

https://codereview.chromium.org/160703002/diff/30001/google_apis/gcm/engine/c...
google_apis/gcm/engine/connection_factory_impl.h:96: // Backoff policy from
previous backoff attempt. Updated on each login
nit: backoff entry 
not backoff policy I think.

[Nicolas Zea, 2014-02-12 21:50:15]

https://codereview.chromium.org/160703002/diff/30001/google_apis/gcm/engine/c...
File google_apis/gcm/engine/connection_factory_impl.cc (right):

https://codereview.chromium.org/160703002/diff/30001/google_apis/gcm/engine/c...
google_apis/gcm/engine/connection_factory_impl.cc:118:
UMA_HISTOGRAM_ENUMERATION("GCM.ConnectionResetReason",
On 2014/02/12 19:24:01, Filip Gorski wrote:
> Perhaps we could wrap that in LogConnectionResetReason so that we can laster
add
> diagnostic observers for GCM-internals in the Logging methods. It applies to
> other UMA Methods, like the whole if below (could be LogConnectionUptime or
> something like that)

I think that these methods are already the ones that would hook up to diagnostic
observers. None of the histograms are repeated (unlike in the
registration/checkin requests); they're all for different kinds of events
(external connection reset, internal reset, connection complete, etc).

https://codereview.chromium.org/160703002/diff/30001/google_apis/gcm/engine/c...
File google_apis/gcm/engine/connection_factory_impl.h (right):

https://codereview.chromium.org/160703002/diff/30001/google_apis/gcm/engine/c...
google_apis/gcm/engine/connection_factory_impl.h:96: // Backoff policy from
previous backoff attempt. Updated on each login
On 2014/02/12 19:24:01, Filip Gorski wrote:
> nit: backoff entry 
> not backoff policy I think.

Done.

[fgorski, 2014-02-12 21:56:27]

lgtm with nit

https://codereview.chromium.org/160703002/diff/30001/google_apis/gcm/engine/c...
File google_apis/gcm/engine/connection_factory_impl.cc (right):

https://codereview.chromium.org/160703002/diff/30001/google_apis/gcm/engine/c...
google_apis/gcm/engine/connection_factory_impl.cc:118:
UMA_HISTOGRAM_ENUMERATION("GCM.ConnectionResetReason",
On 2014/02/12 21:50:16, Nicolas Zea wrote:
> On 2014/02/12 19:24:01, Filip Gorski wrote:
> > Perhaps we could wrap that in LogConnectionResetReason so that we can laster
> add
> > diagnostic observers for GCM-internals in the Logging methods. It applies to
> > other UMA Methods, like the whole if below (could be LogConnectionUptime or
> > something like that)
> 
> I think that these methods are already the ones that would hook up to
diagnostic
> observers. None of the histograms are repeated (unlike in the
> registration/checkin requests); they're all for different kinds of events
> (external connection reset, internal reset, connection complete, etc).

OK, I guess we can figure it out when we are closer to actually showing the data
on the internals page.

https://codereview.chromium.org/160703002/diff/100001/google_apis/gcm/engine/...
File google_apis/gcm/engine/connection_factory_impl.h (right):

https://codereview.chromium.org/160703002/diff/100001/google_apis/gcm/engine/...
google_apis/gcm/engine/connection_factory_impl.h:94: // Connection attempt
backoff policy.
nit: backoff entry.

I should have noticed that earlier.

[Nicolas Zea, 2014-02-12 22:04:12]

Done. +Alexei for histograms

[Alexei Svitkine (slow), 2014-02-12 22:15:36]

lgtm

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

https://codereview.chromium.org/160703002/diff/160001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:5502: +    Time (from login until reset)
that a GCM connection was active
Nit: add a .

[jianli, 2014-02-12 22:20:58]

lgtm

https://codereview.chromium.org/160703002/diff/100001/google_apis/gcm/engine/...
File google_apis/gcm/engine/connection_factory_impl.h (right):

https://codereview.chromium.org/160703002/diff/100001/google_apis/gcm/engine/...
google_apis/gcm/engine/connection_factory_impl.h:112: base::TimeTicks
last_login_time_;
nit: include file for base::TimeTicks

[Nicolas Zea, 2014-02-12 22:27:34]

All comments addressed, committing via CQ

https://codereview.chromium.org/160703002/diff/100001/google_apis/gcm/engine/...
File google_apis/gcm/engine/connection_factory_impl.h (right):

https://codereview.chromium.org/160703002/diff/100001/google_apis/gcm/engine/...
google_apis/gcm/engine/connection_factory_impl.h:94: // Connection attempt
backoff policy.
On 2014/02/12 21:56:27, Filip Gorski wrote:
> nit: backoff entry.
> 
> I should have noticed that earlier.

Done.

https://codereview.chromium.org/160703002/diff/100001/google_apis/gcm/engine/...
google_apis/gcm/engine/connection_factory_impl.h:112: base::TimeTicks
last_login_time_;
On 2014/02/12 22:20:59, jianli wrote:
> nit: include file for base::TimeTicks

Done.

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

https://codereview.chromium.org/160703002/diff/160001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:5502: +    Time (from login until reset)
that a GCM connection was active
On 2014/02/12 22:15:37, Alexei Svitkine wrote:
> Nit: add a .

Done.

[Nicolas Zea, 2014-02-12 22:27:40]

The CQ bit was checked by zea@chromium.org

[commit-bot: I haz the power, 2014-02-12 22:29:43]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/zea@chromium.org/160703002/280001

[commit-bot: I haz the power, 2014-02-12 22:29:46]

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

[commit-bot: I haz the power, 2014-02-12 22:29:47]

No file was found in this patchset.

[Nicolas Zea, 2014-02-12 22:42:21]

The CQ bit was checked by zea@chromium.org

[commit-bot: I haz the power, 2014-02-12 23:01:38]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/zea@chromium.org/160703002/160001

[commit-bot: I haz the power, 2014-02-12 23:01:52]

Failed to trigger a try job on win_rel
HTTP Error 400: Bad Request

[commit-bot: I haz the power, 2014-02-12 23:08:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/zea@chromium.org/160703002/380001

[commit-bot: I haz the power, 2014-02-13 01:17:04]

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

[commit-bot: I haz the power, 2014-02-13 01:17:05]

Retried try job too often on win_rel for step(s) content_browsertests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[Nicolas Zea, 2014-02-13 18:03:29]

The CQ bit was checked by zea@chromium.org

[commit-bot: I haz the power, 2014-02-13 18:04:03]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/zea@chromium.org/160703002/380001

[commit-bot: I haz the power, 2014-02-13 19:54:51]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/zea@chromium.org/160703002/380001

[Nicolas Zea, 2014-02-13 20:55:49]

The CQ bit was checked by zea@chromium.org

[commit-bot: I haz the power, 2014-02-13 21:37:39]

Commit queue failed due to new patchset.

[Nicolas Zea, 2014-02-13 21:39:05]

The CQ bit was unchecked by zea@chromium.org

[Nicolas Zea, 2014-02-13 21:39:06]

The CQ bit was checked by zea@chromium.org

[commit-bot: I haz the power, 2014-02-13 21:43:07]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/zea@chromium.org/160703002/830001

[commit-bot: I haz the power, 2014-02-13 23:36:45]

Change committed as 251186