[GCM] Add port 443 fallback logic and histograms

[GCM] Add port 443 fallback logic and histograms

Alternate between the default (5228) endpoint and the fallback (443) endpoint.
Also introduce histograms for tracking endpoint and proxy success rate.

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

[Nicolas Zea, 2014-03-19 23:16:04]

PTAL

[jianli, 2014-03-20 01:11:53]

https://codereview.chromium.org/205343003/diff/1/google_apis/gcm/engine/conne...
File google_apis/gcm/engine/connection_factory_impl.h (right):

https://codereview.chromium.org/205343003/diff/1/google_apis/gcm/engine/conne...
google_apis/gcm/engine/connection_factory_impl.h:94: // Index to the endpoint
for a which a connection should be attempted next.
nit: for which

https://codereview.chromium.org/205343003/diff/1/google_apis/gcm/engine/conne...
File google_apis/gcm/engine/connection_factory_impl_unittest.cc (right):

https://codereview.chromium.org/205343003/diff/1/google_apis/gcm/engine/conne...
google_apis/gcm/engine/connection_factory_impl_unittest.cc:143: :
ConnectionFactoryImpl(BuildEndpoints(),
Do we have a test to verify that the 2nd endpoint is used when the 1st endpoint
fails?

[fgorski, 2014-03-20 16:45:41]

https://codereview.chromium.org/205343003/diff/1/google_apis/gcm/engine/conne...
File google_apis/gcm/engine/connection_factory_impl.cc (right):

https://codereview.chromium.org/205343003/diff/1/google_apis/gcm/engine/conne...
google_apis/gcm/engine/connection_factory_impl.cc:250:
UMA_HISTOGRAM_COUNTS("GCM.ConnectionEndpoint", current_endpoint_);
How/what exactly do you mean to measure here? (this is related to your TODO that
follows)

https://codereview.chromium.org/205343003/diff/1/google_apis/gcm/gcm_client_i...
File google_apis/gcm/gcm_client_impl.cc (right):

https://codereview.chromium.org/205343003/diff/1/google_apis/gcm/gcm_client_i...
google_apis/gcm/gcm_client_impl.cc:74: const char kMCSEndpointMain[] =
"https://bla.google.com:5228";
bla.google.com?

[Nicolas Zea, 2014-03-20 20:10:02]

PTAL

https://codereview.chromium.org/205343003/diff/1/google_apis/gcm/engine/conne...
File google_apis/gcm/engine/connection_factory_impl.cc (right):

https://codereview.chromium.org/205343003/diff/1/google_apis/gcm/engine/conne...
google_apis/gcm/engine/connection_factory_impl.cc:250:
UMA_HISTOGRAM_COUNTS("GCM.ConnectionEndpoint", current_endpoint_);
On 2014/03/20 16:45:42, Filip Gorski wrote:
> How/what exactly do you mean to measure here? (this is related to your TODO
that
> follows)

I'd like to understand what % of users are able to connect to port 443 but
unable to connect to port 5228. If it's a large amount, at some point we should
probably swap the priority order.

Granted, because this is just alternating, there'll be some noise due to those
users who are in backoff and, when their network comes online or something,
happen to then be attempting the connection to the 443 endpoint. I'm not sure
there's a better way to measure though?

https://codereview.chromium.org/205343003/diff/1/google_apis/gcm/engine/conne...
File google_apis/gcm/engine/connection_factory_impl.h (right):

https://codereview.chromium.org/205343003/diff/1/google_apis/gcm/engine/conne...
google_apis/gcm/engine/connection_factory_impl.h:94: // Index to the endpoint
for a which a connection should be attempted next.
On 2014/03/20 01:11:53, jianli wrote:
> nit: for which

Done.

https://codereview.chromium.org/205343003/diff/1/google_apis/gcm/engine/conne...
File google_apis/gcm/engine/connection_factory_impl_unittest.cc (right):

https://codereview.chromium.org/205343003/diff/1/google_apis/gcm/engine/conne...
google_apis/gcm/engine/connection_factory_impl_unittest.cc:143: :
ConnectionFactoryImpl(BuildEndpoints(),
On 2014/03/20 01:11:53, jianli wrote:
> Do we have a test to verify that the 2nd endpoint is used when the 1st
endpoint
> fails?

Done.

https://codereview.chromium.org/205343003/diff/1/google_apis/gcm/gcm_client_i...
File google_apis/gcm/gcm_client_impl.cc (right):

https://codereview.chromium.org/205343003/diff/1/google_apis/gcm/gcm_client_i...
google_apis/gcm/gcm_client_impl.cc:74: const char kMCSEndpointMain[] =
"https://bla.google.com:5228";
On 2014/03/20 16:45:42, Filip Gorski wrote:
> bla.google.com?

Whoops, good catch, that was from some manual testing. Done.

[jianli, 2014-03-20 23:10:21]

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

https://codereview.chromium.org/205343003/diff/20001/google_apis/gcm/engine/c...
google_apis/gcm/engine/connection_factory_impl.cc:249: next_endpoint_++;
What if GetCurrentEndpoint() uses last_successful_endpoint_ and it failed? We're
indeed not trying next_endpoint_ but simply moving to the next one.

https://codereview.chromium.org/205343003/diff/20001/google_apis/gcm/engine/c...
google_apis/gcm/engine/connection_factory_impl.cc:266: last_successful_endpoint_
= next_endpoint_;
What if GetCurrentEndpoint() uses last_successful_endpoint_?

[Nicolas Zea, 2014-03-20 23:53:29]

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

https://codereview.chromium.org/205343003/diff/20001/google_apis/gcm/engine/c...
google_apis/gcm/engine/connection_factory_impl.cc:249: next_endpoint_++;
On 2014/03/20 23:10:21, jianli wrote:
> What if GetCurrentEndpoint() uses last_successful_endpoint_ and it failed?
We're
> indeed not trying next_endpoint_ but simply moving to the next one.

While connecting, GetCurrentEndpoint() will not return
last_successful_endpoint_, due to IsEndpointReachable() being dependent on
!connecting_. Therefore, all connections are based on next_endpoint_.

https://codereview.chromium.org/205343003/diff/20001/google_apis/gcm/engine/c...
google_apis/gcm/engine/connection_factory_impl.cc:266: last_successful_endpoint_
= next_endpoint_;
On 2014/03/20 23:10:21, jianli wrote:
> What if GetCurrentEndpoint() uses last_successful_endpoint_?

See above.

[jianli, 2014-03-21 00:13:18]

lgtm

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

https://codereview.chromium.org/205343003/diff/20001/google_apis/gcm/engine/c...
google_apis/gcm/engine/connection_factory_impl.cc:249: next_endpoint_++;
On 2014/03/20 23:53:30, Nicolas Zea wrote:
> On 2014/03/20 23:10:21, jianli wrote:
> > What if GetCurrentEndpoint() uses last_successful_endpoint_ and it failed?
> We're
> > indeed not trying next_endpoint_ but simply moving to the next one.
> 
> While connecting, GetCurrentEndpoint() will not return
> last_successful_endpoint_, due to IsEndpointReachable() being dependent on
> !connecting_. Therefore, all connections are based on next_endpoint_.

Could you please add a comment in GetCurrentEndpoint?

[Nicolas Zea, 2014-03-21 00:35:10]

Done. Filip, did you have any further comments?

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

https://codereview.chromium.org/205343003/diff/20001/google_apis/gcm/engine/c...
google_apis/gcm/engine/connection_factory_impl.cc:249: next_endpoint_++;
On 2014/03/21 00:13:18, jianli wrote:
> On 2014/03/20 23:53:30, Nicolas Zea wrote:
> > On 2014/03/20 23:10:21, jianli wrote:
> > > What if GetCurrentEndpoint() uses last_successful_endpoint_ and it failed?
> > We're
> > > indeed not trying next_endpoint_ but simply moving to the next one.
> > 
> > While connecting, GetCurrentEndpoint() will not return
> > last_successful_endpoint_, due to IsEndpointReachable() being dependent on
> > !connecting_. Therefore, all connections are based on next_endpoint_.
> 
> Could you please add a comment in GetCurrentEndpoint?

Done.

[Nicolas Zea, 2014-03-21 01:38:27]

The CQ bit was checked by zea@chromium.org

[commit-bot: I haz the power, 2014-03-21 01:39:56]

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

[commit-bot: I haz the power, 2014-03-21 02:16:39]

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

[commit-bot: I haz the power, 2014-03-21 02:16:40]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[Nicolas Zea, 2014-03-21 02:18:03]

+Alexei for histograms

[Alexei Svitkine (slow), 2014-03-21 15:31:43]

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

https://codereview.chromium.org/205343003/diff/40001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:6578: +    gcm_client_impl.cc for
endpoint definitions.
Can they be enumerated with an enum entry in histograms.xml?

[fgorski, 2014-03-21 15:39:27]

lgtm

[Nicolas Zea, 2014-03-21 17:47:02]

Histogram enums updated, PTAL

[Nicolas Zea, 2014-03-21 17:47:16]

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

https://codereview.chromium.org/205343003/diff/40001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:6578: +    gcm_client_impl.cc for
endpoint definitions.
On 2014/03/21 15:31:43, Alexei Svitkine wrote:
> Can they be enumerated with an enum entry in histograms.xml?

Done.

[Alexei Svitkine (slow), 2014-03-21 17:52:06]

lgtm

[Nicolas Zea, 2014-03-21 21:38:53]

The CQ bit was checked by zea@chromium.org

[commit-bot: I haz the power, 2014-03-21 21:40:40]

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

[commit-bot: I haz the power, 2014-03-22 00:35:34]

Change committed as 258720

[Lei Zhang, 2014-03-22 01:11:03]

FYI, this broke the build. The trybots don't build the mcs_probe target, but the
real bots do.