Added ClientEvent proto and structure for storing events in the factory.

google_apis/gcm/engine/connection_factory_impl.cc

Index: google_apis/gcm/engine/connection_factory_impl.cc
diff --git a/google_apis/gcm/engine/connection_factory_impl.cc b/google_apis/gcm/engine/connection_factory_impl.cc
index 67dab986357359282be3801f335d061de23c1f9d..feb16367edae64f8472652b087b1511690ecadfa 100644
--- a/google_apis/gcm/engine/connection_factory_impl.cc
+++ b/google_apis/gcm/engine/connection_factory_impl.cc
@@ -205,6 +205,10 @@ void ConnectionFactoryImpl::SignalConnectionReset(
     // connection.
   }
 
+  if (logging_in_)
+    event_tracker_.ConnectionLoginFailed();
+  event_tracker_.EndConnectionAttempt();

[Peter Beverloo, 2016/12/02 15:19:13]

It seems to me like you'd want this to live in ConnectionHandlerCallback
instead?

SignalConnectionReset is called when the connection has been *reset* because of
an error. That could be hours after the connection attempt started, which aren't
really the semantics we want. Instead, ConnectionHandlerCallback is called when
(among other reasons) the handshake has been completed.

[harkness, 2016/12/05 15:39:01]

The problem here may be naming. The issue is that one of the fields to be filled
out is time_connection_ended_ms, which for a successful connection could be
hours long. We could fill out almost all information in
ConnectionHandlerCallback (and we do, because we call
ConnectionAttemptSucceeded() there), but we wouldn't have that info.

I could change the name of the method to EndConnection(), but the functionality
is identical for when a connection attempt fails and we end it, and in that case
EndConnectionAttempt() is definitely the right naming. 

[Peter Beverloo, 2016/12/05 16:29:34]

When a connection attempt fails, the connection itself also ends. (Following a
line of thought where a connection is not per se established.) Let's do
EndConnection()?

As an aside, consider prefixing these methods with Did to avoid ambiguity with
*actually* ending the connection:

- DidStartConnectionAttempt
- DidFinishConnectionAttempt
- ...

Not super pretty, but it would increase clarity.

+
   CloseSocket();
   DCHECK(!IsEndpointReachable());
 
@@ -260,6 +264,9 @@ base::TimeTicks ConnectionFactoryImpl::NextRetryAttempt() const {
 
 void ConnectionFactoryImpl::OnNetworkChanged(
     net::NetworkChangeNotifier::ConnectionType type) {
+  // Let the connection tracker know about the network type.

[Peter Beverloo, 2016/12/02 15:19:13]

nit: I'd drop the comment. It's already clear from the call.

[harkness, 2016/12/05 15:39:00]

Removed.

+  event_tracker_.OnNetworkChanged(type);
+
   if (type == net::NetworkChangeNotifier::CONNECTION_NONE) {
     DVLOG(1) << "Network lost, resettion connection.";
     waiting_for_network_online_ = true;
@@ -296,6 +303,11 @@ net::IPEndPoint ConnectionFactoryImpl::GetPeerIP() {
 }
 
 void ConnectionFactoryImpl::ConnectImpl() {
+  event_tracker_.StartConnectionAttempt();
+  StartConnection();
+}
+
+void ConnectionFactoryImpl::StartConnection() {
   DCHECK(!IsEndpointReachable());
   // TODO(zea): Make this a dcheck again. crbug.com/462319
   CHECK(!socket_handle_.socket());
@@ -324,6 +336,7 @@ void ConnectionFactoryImpl::InitHandler() {
   // May be null in tests.
   mcs_proto::LoginRequest login_request;
   if (!request_builder_.is_null()) {
+    event_tracker_.WriteToLoginRequest(&login_request);

[Peter Beverloo, 2016/12/02 15:19:13]

MCSClient::ResetStateAndBuildLoginRequest actually swaps out the passed
|*login_request| with another instance, so the stuff you're adding here is being
overwritten by line 340.

Instead, call WriteToLoginRequest after the DCHECK on line 341.

[harkness, 2016/12/05 15:39:00]

Nice catch! It looks like this isn't caught by the tests because they use an
empty BuildLoginRequestCallback method.

     request_builder_.Run(&login_request);
     DCHECK(login_request.IsInitialized());
   }
@@ -371,6 +384,9 @@ void ConnectionFactoryImpl::OnConnectDone(int result) {
     backoff_entry_->InformOfRequest(false);
     UMA_HISTOGRAM_SPARSE_SLOWLY("GCM.ConnectionFailureErrorCode", result);
 
+    event_tracker_.ConnectionAttemptFailed(result);
+    event_tracker_.EndConnectionAttempt();
+
     // If there are other endpoints available, use the next endpoint on the
     // subsequent retry.
     next_endpoint_++;
@@ -419,6 +435,8 @@ void ConnectionFactoryImpl::ConnectionHandlerCallback(int result) {
   backoff_entry_->Reset();
   logging_in_ = false;
 
+  event_tracker_.ConnectionAttemptSucceeded();

[Peter Beverloo, 2016/12/02 15:19:13]

It seems to me that, logically, this would be a better fit in OnConnectDone().
(Around line 406, supposedly.)

[harkness, 2016/12/05 15:39:01]

Actually, I think this placement is correct. The most important thing that
happens in ConnectionAttemptSucceeded() is that the previous set of events is
cleared. However, in OnConnectDone() the login request hasn't been sent yet. So
if we clear it here, we'll never send any events upstream.

OnConnectDone just means that the physical connection is established. It's not
until InitHandler() is procecessed that ConnectionHandlerImpl::Login gets called
and exchanges the Login/Login ack. The result of the Login response is what
causes ConnectionHandlerCallback to be called, so that's the first place it's
safe to clear the previous event data.

[Peter Beverloo, 2016/12/05 16:29:34]

Acknowledged.

+
   if (listener_)
     listener_->OnConnected(GetCurrentEndpoint(), GetPeerIP());
 }