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..e3d054531b23834ba1f31b87ebb1ae991928a3a7 100644
--- a/google_apis/gcm/engine/connection_factory_impl.cc
+++ b/google_apis/gcm/engine/connection_factory_impl.cc
@@ -45,6 +45,8 @@ bool ShouldRestorePreviousBackoff(const base::TimeTicks& login_time,
           base::TimeDelta::FromSeconds(kConnectionResetWindowSecs);
 }
 
+const int kMaxClientEvents = 30;

[Peter Beverloo, 2016/11/28 15:05:15]

++docs. Explain what it is and how this number was decided on.

[harkness, 2016/12/01 10:34:34]

Done.

+
 }  // namespace
 
 ConnectionFactoryImpl::ConnectionFactoryImpl(
@@ -205,6 +207,17 @@ void ConnectionFactoryImpl::SignalConnectionReset(
     // connection.
   }
 
+  // Record that the connection has ended.
+  if (logging_in_) {
+    // If this was a login failure, the connection established time would have
+    // been set and it would have marked a successful connection, so explicitly
+    // clear that.
+    current_event_.set_type(mcs_proto::ClientEvent::FAILED_CONNECTION);
+    current_event_.clear_time_connection_established_ms();
+    // TODO(harkness): What error code do we want to set?
+  }
+  current_event_.set_time_connection_ended_ms(NowTicks().ToInternalValue());

[Peter Beverloo, 2016/11/28 15:05:14]

Here and elsewhere:

I'm not comfortable using ToInternalValue() for values that we send to and store
on the server. There is no guarantee that this value is portable between
platforms, nor that it will remain so in the future if it is.

Can we use base::Time::Now().ToJavaTime() instead, which returns the number of
elapsed milliseconds since the Unix epoch?

[harkness, 2016/12/01 10:34:34]

Sure, that seems reasonable.

+
   CloseSocket();
   DCHECK(!IsEndpointReachable());
 
@@ -260,6 +273,9 @@ base::TimeTicks ConnectionFactoryImpl::NextRetryAttempt() const {
 
 void ConnectionFactoryImpl::OnNetworkChanged(
     net::NetworkChangeNotifier::ConnectionType type) {
+  // Record the current network type.
+  network_type_ = type;

[Peter Beverloo, 2016/11/28 15:05:15]

Since we only fire OnNetworkChanged() when it actually changes, presumably this
will cause a huge bias to CONNECTION_NONE in our stats, especially on desktop.

Is there any way we can init |network_type_| with a real value?

[harkness, 2016/12/01 10:34:34]

You're right, that is going to be a problem. I haven't found a way to query for
the current state. I'll ask zea.

+
   if (type == net::NetworkChangeNotifier::CONNECTION_NONE) {
     DVLOG(1) << "Network lost, resettion connection.";
     waiting_for_network_online_ = true;
@@ -296,6 +312,31 @@ net::IPEndPoint ConnectionFactoryImpl::GetPeerIP() {
 }
 
 void ConnectionFactoryImpl::ConnectImpl() {
+  if (total_client_events_ >= 0) {

[Peter Beverloo, 2016/11/28 15:05:15]

Instead of having |total_client_events_|, I think it'd be a lot clearer to just
use |client_events_.size()| in the code, and maintaining a
|discarded_client_events_| counter when we override a previous entry (which
matches the semantics of DISCARDED_EVENTS).

That also allows you to get rid of the magic value of "0".

[harkness, 2016/12/01 10:34:34]

Refactored this into the ClientEventTracker, which just uses a STL structure to
hold the events. This reduces the need for tracking at the expense of copies,
but the connections are rare, so that's worth it for the code complexity
reduction.

+    // If starting a new connection, the previous connection is completed, so
+    // move
+    // the current client event onto the list of past events.
+    mcs_proto::ClientEvent* client_event = nullptr;
+    if (total_client_events_ < kMaxClientEvents) {
+      client_event = client_events_.Add();
+    } else {
+      // If the maximum number of stored events has been exceeded, start
+      // overwriting older entries.
+      client_event =
+          client_events_.Mutable(total_client_events_ % kMaxClientEvents);
+    }
+    client_event->CopyFrom(current_event_);
+    current_event_.Clear();

[Peter Beverloo, 2016/11/28 15:05:15]

it seems to me like this code would become *a lot* clearer if we'd store the
mcs_proto::ClientEvent in a std::deque<> and use an anonymous function to do the
insertion for us:

https://cs.chromium.org/chromium/src/components/gcm_driver/gcm_stats_recorder...

It'd create some data copies in the worst case, but this is (a) data of tiny
size, and (b) a very infrequent event.

[harkness, 2016/12/01 10:34:34]

Done.

+  }
+  total_client_events_++;
+
+  current_event_.set_time_connection_started_ms(NowTicks().ToInternalValue());
+  current_event_.set_network_type(network_type_);
+
+  StartConnection();
+}
+
+void ConnectionFactoryImpl::StartConnection() {
   DCHECK(!IsEndpointReachable());
   // TODO(zea): Make this a dcheck again. crbug.com/462319
   CHECK(!socket_handle_.socket());
@@ -324,6 +365,7 @@ void ConnectionFactoryImpl::InitHandler() {
   // May be null in tests.
   mcs_proto::LoginRequest login_request;
   if (!request_builder_.is_null()) {
+    // TODO(harkness): Pass the previous ClientEvents into the request builder.

[Peter Beverloo, 2016/11/28 15:05:15]

This also needs to build a ClientEvent for DISCARDED_EVENTS.

[harkness, 2016/12/01 10:34:34]

TODO added.

     request_builder_.Run(&login_request);
     DCHECK(login_request.IsInitialized());
   }
@@ -371,6 +413,12 @@ void ConnectionFactoryImpl::OnConnectDone(int result) {
     backoff_entry_->InformOfRequest(false);
     UMA_HISTOGRAM_SPARSE_SLOWLY("GCM.ConnectionFailureErrorCode", result);
 
+    // Record the failed connection so information about it can be sent in the
+    // next login request.
+    current_event_.set_type(mcs_proto::ClientEvent::FAILED_CONNECTION);
+    current_event_.set_time_connection_ended_ms(NowTicks().ToInternalValue());
+    current_event_.set_error_code(result);
+
     // If there are other endpoints available, use the next endpoint on the
     // subsequent retry.
     next_endpoint_++;
@@ -419,6 +467,18 @@ void ConnectionFactoryImpl::ConnectionHandlerCallback(int result) {
   backoff_entry_->Reset();
   logging_in_ = false;
 
+  // Record the successful connection so information about it can be sent in the
+  // next login request. If there is a login failure, this will need to be
+  // updated to a failed connection.
+  current_event_.set_type(mcs_proto::ClientEvent::SUCCESSFUL_CONNECTION);
+  current_event_.set_time_connection_established_ms(
+      NowTicks().ToInternalValue());
+
+  // A completed connection means that the old client event data has now been
+  // sent to GCM. Delete old data and reinitialize the count.
+  client_events_.Clear();
+  total_client_events_ = 0;
+
   if (listener_)
     listener_->OnConnected(GetCurrentEndpoint(), GetPeerIP());
 }