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

google_apis/gcm/engine/connection_event_tracker.cc

Index: google_apis/gcm/engine/connection_event_tracker.cc
diff --git a/google_apis/gcm/engine/connection_event_tracker.cc b/google_apis/gcm/engine/connection_event_tracker.cc
new file mode 100644
index 0000000000000000000000000000000000000000..f947013efc69f9338890c218f80596fd48802727
--- /dev/null
+++ b/google_apis/gcm/engine/connection_event_tracker.cc
@@ -0,0 +1,85 @@
+// Copyright 2016 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "google_apis/gcm/engine/connection_event_tracker.h"
+
+#include "base/time/time.h"
+#include "google_apis/gcm/protocol/mcs.pb.h"
+
+namespace {
+
+// The maxiumum number of events which are stored before deleting old ones.
+// This mirrors the behaviour of the GMS Core connection tracking.
+const int kMaxClientEvents = 30;

[Nicolas Zea, 2016/12/01 18:51:49]

nit: newline after

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

nit: constexpr size_t kMaxClientEvents

(std::deque::size returns an unsigned integral type.)

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

Done.

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

Done.

+}

[Nicolas Zea, 2016/12/01 18:51:49]

nit: 
}  // namespace

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

Done.

+
+namespace gcm {
+
+ConnectionEventTracker::ConnectionEventTracker() {}

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

{} --> = default;

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

Done.

+
+ConnectionEventTracker::~ConnectionEventTracker() {
+  // If there is currently a successful connection that is being closed as a
+  // result of the shutdown, mark the connection as completed.
+  if (current_event_.type() == mcs_proto::ClientEvent::SUCCESSFUL_CONNECTION)
+    EndConnectionAttempt();

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

Delete this and just have the default destructor.

While it's semantically correct what you're doing, the destructor is called when
the memory associated with this class is being deleted. That means that we won't
be able to access the completed events anymore anyway.

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

My original plan was to queue this for writing (once I add persistence in the
next patch), but after a discussion with Peter, we decided to handle persisting
completed connections another way.

Removing the destructor implementation.

+}
+
+void ConnectionEventTracker::StartConnectionAttempt() {
+  // TODO(harkness): Can we dcheck here that there is not an in progress
+  // connection?

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

I suppose this question relates to the rest of the GCM Engine more than the
connection event tracker implementation? In that case it translates to "can
there be multiple simultaneous connection attempts".

Since the ConnectionFactoryImpl maintains this state, as well as the back-off
and other connection logic, it's probably "yes". You could upload a follow-up CL
and see whether the try-bots agree.

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

It's mostly a TODO for me to follow up with zea. I believe that I need to be
able to support multiple connection attempts because of a comment in
ConnectionFactoryImpl::ConnectWithBackoff(), but I don't understand the exact
way that can happen. 

+  current_event_.set_time_connection_started_ms(base::Time::Now().ToJavaTime());
+  current_event_.set_network_type(network_type_);
+}
+
+void ConnectionEventTracker::EndConnectionAttempt() {
+  if (completed_events_.size() == kMaxClientEvents) {
+    // Don't let the completed events grow beyond the max.
+    completed_events_.pop_front();
+    // TODO(harkness): Keep track of deleted events.
+  }
+
+  // Current event is now completed, so add it to our list of completed events.
+  current_event_.set_time_connection_ended_ms(base::Time::Now().ToJavaTime());
+  completed_events_.push_back(current_event_);
+  current_event_.Clear();
+}
+
+void ConnectionEventTracker::ConnectionAttemptSucceeded() {
+  // 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(
+      base::Time::Now().ToJavaTime());
+
+  // A completed connection means that the old client event data has now been
+  // sent to GCM. Delete old data.
+  completed_events_.resize(0);

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

nit: why not clear()?

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

I was aiming for a method that would deallocate the storage and I didn't go look
up the guarantees for deque. It looks like they're equivalent now that I look,
so I can change it to clear().

+}
+
+void ConnectionEventTracker::ConnectionLoginFailed() {
+  // A login failure would have originally been marked as a successful
+  // connection, so now that it failed, that needs to be updated.
+  DCHECK(current_event_.type() ==
+         mcs_proto::ClientEvent::SUCCESSFUL_CONNECTION);

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

nit: I think we should be able to use DCHECK_EQ() here now. Dito elsewhere.

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

Done.

+
+  current_event_.set_type(mcs_proto::ClientEvent::FAILED_CONNECTION);
+  current_event_.clear_time_connection_established_ms();
+  // TODO(harkness): What error code should be used here?

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

Options I'm considering are ERR_ACCESS_DENIED and ERR_CONNECTION_FAILED. The
drawback of connection failed is that it's going to be used in other situations,
and I'd like to isolate the login failures.

[Nicolas Zea, 2016/12/01 18:51:49]

You're currently only calling this from SignalConnectionReset. Perhaps
CONNECTION_RESET?

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

That sounds good, thanks!

+}
+
+void ConnectionEventTracker::ConnectionAttemptFailed(int error) {
+  DCHECK(error != net::OK);
+
+  current_event_.set_type(mcs_proto::ClientEvent::FAILED_CONNECTION);
+  current_event_.set_error_code(error);
+}
+
+void ConnectionEventTracker::WriteToLoginRequest(
+    mcs_proto::LoginRequest* request) {

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

pedantic nit: DCHECK(request)?

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

Done.

+  for (const mcs_proto::ClientEvent& event : completed_events_)
+    request->add_client_event()->CopyFrom(event);
+}
+
+}  // namespace gcm