Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2481873002:
Added ClientEvent proto and structure for storing events in the factory. (Closed)
DescriptionAdded ClientEvent proto and structure for storing events in the factory.
As part of the project to record GCM connection errors in Chrome, this CL
adds the ClientEvent to the proto for the LoginRequest that goes to GCM.
It also adds local structures to record previous connection attempts,
although persisting the information and using it to construct the login
request is in a subsequent CL.
Tests were also updated to check that events were being recorded corectly
in old test cases, and a new test case was added to check wrapping on the
storage.
BUG=662983
Committed: https://crrev.com/1a49d60bd22ef9ccb97619ba3441bbd89f4b0c3e
Cr-Commit-Position: refs/heads/master@{#436584}
Patch Set 1 #
Total comments: 22
Patch Set 2 : Added ConnectionEventTracker to manage all the events. #
Total comments: 49
Patch Set 3 : Rebase and address code review comments. #
Total comments: 24
Patch Set 4 : Integrated code review comments #
Messages
Total messages: 20 (8 generated)
Description was changed from ========== Added ClientEvent proto and structure for storing events in the factory. As part of the project to record GCM connection errors in Chrome, this CL adds the ClientEvent to the proto for the LoginRequest that goes to GCM. It also adds local structures to record previous connection attempts, although persisting the information and using it to construct the login request is in a subsequent CL. Tests were also updated to check that events were being recorded corectly in old test cases, and a new test case was added to check wrapping on the storage. BUG= ========== to ========== Added ClientEvent proto and structure for storing events in the factory. As part of the project to record GCM connection errors in Chrome, this CL adds the ClientEvent to the proto for the LoginRequest that goes to GCM. It also adds local structures to record previous connection attempts, although persisting the information and using it to construct the login request is in a subsequent CL. Tests were also updated to check that events were being recorded corectly in old test cases, and a new test case was added to check wrapping on the storage. BUG=662983 ==========
harkness@chromium.org changed reviewers: + peter@chromium.org
CL 1 of 3, this tracks the previous ClientEvents. Next will be storing them, then sending them to GCM in the Login Request.
Thanks! Please read the comments before making changes -- depending on what you choose to do, certain comments may end up being contradictory or redundant. At a higher level, we should be able to split out a dedicated ClientEventTracker. It would build the appropriate ClientEvents based on a bit of a state machine (which would also give us awesome DCHECKs), encapsulate the logic for the size-limited container and hide the code required for writing the events (including the DISCARDED_EVENTS one) to the LoginRequest from the ConnectionFactory. Something like the following: ClientEventTracker - StartConnectionAttempt() - ConnectionAttemptSucceeded() - ConnectionAttemptFailed(int32_t error_code) - OnNetworkChanged(int32_t network_type) - WriteToLoginRequest(mcs_proto::LoginRequest*) WDYT? https://codereview.chromium.org/2481873002/diff/1/google_apis/gcm/engine/conn... File google_apis/gcm/engine/connection_factory_impl.cc (right): https://codereview.chromium.org/2481873002/diff/1/google_apis/gcm/engine/conn... google_apis/gcm/engine/connection_factory_impl.cc:48: const int kMaxClientEvents = 30; ++docs. Explain what it is and how this number was decided on. https://codereview.chromium.org/2481873002/diff/1/google_apis/gcm/engine/conn... google_apis/gcm/engine/connection_factory_impl.cc:219: current_event_.set_time_connection_ended_ms(NowTicks().ToInternalValue()); 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? https://codereview.chromium.org/2481873002/diff/1/google_apis/gcm/engine/conn... google_apis/gcm/engine/connection_factory_impl.cc:277: network_type_ = type; 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? https://codereview.chromium.org/2481873002/diff/1/google_apis/gcm/engine/conn... google_apis/gcm/engine/connection_factory_impl.cc:315: if (total_client_events_ >= 0) { 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". https://codereview.chromium.org/2481873002/diff/1/google_apis/gcm/engine/conn... google_apis/gcm/engine/connection_factory_impl.cc:329: current_event_.Clear(); 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. https://codereview.chromium.org/2481873002/diff/1/google_apis/gcm/engine/conn... google_apis/gcm/engine/connection_factory_impl.cc:368: // TODO(harkness): Pass the previous ClientEvents into the request builder. This also needs to build a ClientEvent for DISCARDED_EVENTS. https://codereview.chromium.org/2481873002/diff/1/google_apis/gcm/protocol/mc... File google_apis/gcm/protocol/mcs.proto (right): https://codereview.chromium.org/2481873002/diff/1/google_apis/gcm/protocol/mc... google_apis/gcm/protocol/mcs.proto:73: // defined in google3/buzz/mobile/proto/gtalk_core.proto. Let's not refer to google3 paths, as useful as may be. The concern you're expressing goes for any other message in this file as well. Since we're the MCS client for a server maintained elsewhere, it's up to the mutual OWNERS to ensure that this keeps in sync. https://codereview.chromium.org/2481873002/diff/1/google_apis/gcm/protocol/mc... google_apis/gcm/protocol/mcs.proto:79: // Failed connection event: the connection faliled to be established or we failed https://codereview.chromium.org/2481873002/diff/1/google_apis/gcm/protocol/mc... google_apis/gcm/protocol/mcs.proto:95: optional int32 network_port = 201; nit: this is unused. maybe mark as "reserved" until we use it? (Can we?) https://codereview.chromium.org/2481873002/diff/1/google_apis/gcm/protocol/mc... google_apis/gcm/protocol/mcs.proto:98: optional int32 error_code = 204; While the server-side does not, I would very much prefer if we can comment on what values we're going to store in these int32s. E.g. is |error_code| going to be a net::Error? (Do we even know yet?) https://codereview.chromium.org/2481873002/diff/1/google_apis/gcm/protocol/mc... google_apis/gcm/protocol/mcs.proto:149: // 19, 20, and 21 are not currently populated by Chrome. Protobufs have a statement to reserve fields, which is good to use together with a comment like this. reserved 19, 20, 21;
https://codereview.chromium.org/2481873002/diff/1/google_apis/gcm/engine/conn... File google_apis/gcm/engine/connection_factory_impl.cc (right): https://codereview.chromium.org/2481873002/diff/1/google_apis/gcm/engine/conn... google_apis/gcm/engine/connection_factory_impl.cc:48: const int kMaxClientEvents = 30; On 2016/11/28 15:05:15, Peter Beverloo wrote: > ++docs. Explain what it is and how this number was decided on. Done. https://codereview.chromium.org/2481873002/diff/1/google_apis/gcm/engine/conn... google_apis/gcm/engine/connection_factory_impl.cc:219: current_event_.set_time_connection_ended_ms(NowTicks().ToInternalValue()); On 2016/11/28 15:05:14, Peter Beverloo wrote: > 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? Sure, that seems reasonable. https://codereview.chromium.org/2481873002/diff/1/google_apis/gcm/engine/conn... google_apis/gcm/engine/connection_factory_impl.cc:277: network_type_ = type; On 2016/11/28 15:05:15, Peter Beverloo wrote: > 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? 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. https://codereview.chromium.org/2481873002/diff/1/google_apis/gcm/engine/conn... google_apis/gcm/engine/connection_factory_impl.cc:315: if (total_client_events_ >= 0) { On 2016/11/28 15:05:15, Peter Beverloo wrote: > 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". 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. https://codereview.chromium.org/2481873002/diff/1/google_apis/gcm/engine/conn... google_apis/gcm/engine/connection_factory_impl.cc:329: current_event_.Clear(); On 2016/11/28 15:05:15, Peter Beverloo wrote: > 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. Done. https://codereview.chromium.org/2481873002/diff/1/google_apis/gcm/engine/conn... google_apis/gcm/engine/connection_factory_impl.cc:368: // TODO(harkness): Pass the previous ClientEvents into the request builder. On 2016/11/28 15:05:15, Peter Beverloo wrote: > This also needs to build a ClientEvent for DISCARDED_EVENTS. TODO added. https://codereview.chromium.org/2481873002/diff/1/google_apis/gcm/protocol/mc... File google_apis/gcm/protocol/mcs.proto (right): https://codereview.chromium.org/2481873002/diff/1/google_apis/gcm/protocol/mc... google_apis/gcm/protocol/mcs.proto:73: // defined in google3/buzz/mobile/proto/gtalk_core.proto. On 2016/11/28 15:05:15, Peter Beverloo wrote: > Let's not refer to google3 paths, as useful as may be. > > The concern you're expressing goes for any other message in this file as well. > Since we're the MCS client for a server maintained elsewhere, it's up to the > mutual OWNERS to ensure that this keeps in sync. Done. https://codereview.chromium.org/2481873002/diff/1/google_apis/gcm/protocol/mc... google_apis/gcm/protocol/mcs.proto:79: // Failed connection event: the connection faliled to be established or we On 2016/11/28 15:05:15, Peter Beverloo wrote: > failed Done. https://codereview.chromium.org/2481873002/diff/1/google_apis/gcm/protocol/mc... google_apis/gcm/protocol/mcs.proto:95: optional int32 network_port = 201; On 2016/11/28 15:05:15, Peter Beverloo wrote: > nit: this is unused. maybe mark as "reserved" until we use it? (Can we?) Done. https://codereview.chromium.org/2481873002/diff/1/google_apis/gcm/protocol/mc... google_apis/gcm/protocol/mcs.proto:98: optional int32 error_code = 204; On 2016/11/28 15:05:15, Peter Beverloo wrote: > While the server-side does not, I would very much prefer if we can comment on > what values we're going to store in these int32s. E.g. is |error_code| going to > be a net::Error? (Do we even know yet?) Done. https://codereview.chromium.org/2481873002/diff/1/google_apis/gcm/protocol/mc... google_apis/gcm/protocol/mcs.proto:149: // 19, 20, and 21 are not currently populated by Chrome. On 2016/11/28 15:05:15, Peter Beverloo wrote: > Protobufs have a statement to reserve fields, which is good to use together with > a comment like this. > > reserved 19, 20, 21; Done. https://codereview.chromium.org/2481873002/diff/20001/google_apis/gcm/engine/... File google_apis/gcm/engine/connection_event_tracker.cc (right): https://codereview.chromium.org/2481873002/diff/20001/google_apis/gcm/engine/... google_apis/gcm/engine/connection_event_tracker.cc:69: // TODO(harkness): What error code should be used here? 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. https://codereview.chromium.org/2481873002/diff/20001/google_apis/gcm/engine/... File google_apis/gcm/engine/connection_event_tracker_unittest.cc (right): https://codereview.chromium.org/2481873002/diff/20001/google_apis/gcm/engine/... google_apis/gcm/engine/connection_event_tracker_unittest.cc:23: TEST_F(ConnectionEventTrackerTest, SuccessfulAttempt) { I'm planning to add more tests to this when I add in the persistence. For now, the tracker is mostly tested via the connection_factory_impl_unittest.cc.
zea@chromium.org changed reviewers: + zea@chromium.org
Drive-by with some small comments and a suggestion for network type handling. https://codereview.chromium.org/2481873002/diff/20001/google_apis/gcm/engine/... File google_apis/gcm/engine/connection_event_tracker.cc (right): https://codereview.chromium.org/2481873002/diff/20001/google_apis/gcm/engine/... google_apis/gcm/engine/connection_event_tracker.cc:14: const int kMaxClientEvents = 30; nit: newline after https://codereview.chromium.org/2481873002/diff/20001/google_apis/gcm/engine/... google_apis/gcm/engine/connection_event_tracker.cc:15: } nit: } // namespace https://codereview.chromium.org/2481873002/diff/20001/google_apis/gcm/engine/... google_apis/gcm/engine/connection_event_tracker.cc:69: // TODO(harkness): What error code should be used here? On 2016/12/01 10:34:34, harkness wrote: > 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. You're currently only calling this from SignalConnectionReset. Perhaps CONNECTION_RESET? https://codereview.chromium.org/2481873002/diff/20001/google_apis/gcm/engine/... File google_apis/gcm/engine/connection_event_tracker.h (right): https://codereview.chromium.org/2481873002/diff/20001/google_apis/gcm/engine/... google_apis/gcm/engine/connection_event_tracker.h:47: void OnNetworkChanged( nit: Only simple accessors should be inlined (and in that case named with unix_hacker_style). Prefer out-of-lining this method. https://codereview.chromium.org/2481873002/diff/20001/google_apis/gcm/engine/... File google_apis/gcm/engine/connection_factory_impl.cc (right): https://codereview.chromium.org/2481873002/diff/20001/google_apis/gcm/engine/... google_apis/gcm/engine/connection_factory_impl.cc:102: waiting_for_network_online_ = net::NetworkChangeNotifier::IsOffline(); You probably need to notify the event tracker that network type changed here to net::NetworkChangeNotifier::GetConnectionType() https://codereview.chromium.org/2481873002/diff/20001/google_apis/gcm/engine/... File google_apis/gcm/engine/connection_factory_impl.h (right): https://codereview.chromium.org/2481873002/diff/20001/google_apis/gcm/engine/... google_apis/gcm/engine/connection_factory_impl.h:116: ConnectionEventTracker event_tracker_; nit: protected members are kind of discouraged. I think it would be cleaner to just inject this in at construction time if you want access to it in a test.
https://codereview.chromium.org/2481873002/diff/20001/google_apis/gcm/engine/... File google_apis/gcm/engine/connection_event_tracker.cc (right): https://codereview.chromium.org/2481873002/diff/20001/google_apis/gcm/engine/... google_apis/gcm/engine/connection_event_tracker.cc:14: const int kMaxClientEvents = 30; nit: constexpr size_t kMaxClientEvents (std::deque::size returns an unsigned integral type.) https://codereview.chromium.org/2481873002/diff/20001/google_apis/gcm/engine/... google_apis/gcm/engine/connection_event_tracker.cc:19: ConnectionEventTracker::ConnectionEventTracker() {} {} --> = default; https://codereview.chromium.org/2481873002/diff/20001/google_apis/gcm/engine/... google_apis/gcm/engine/connection_event_tracker.cc:25: EndConnectionAttempt(); 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. https://codereview.chromium.org/2481873002/diff/20001/google_apis/gcm/engine/... google_apis/gcm/engine/connection_event_tracker.cc:30: // connection? 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. https://codereview.chromium.org/2481873002/diff/20001/google_apis/gcm/engine/... google_apis/gcm/engine/connection_event_tracker.cc:58: completed_events_.resize(0); nit: why not clear()? https://codereview.chromium.org/2481873002/diff/20001/google_apis/gcm/engine/... google_apis/gcm/engine/connection_event_tracker.cc:65: mcs_proto::ClientEvent::SUCCESSFUL_CONNECTION); nit: I think we should be able to use DCHECK_EQ() here now. Dito elsewhere. https://codereview.chromium.org/2481873002/diff/20001/google_apis/gcm/engine/... google_apis/gcm/engine/connection_event_tracker.cc:80: mcs_proto::LoginRequest* request) { pedantic nit: DCHECK(request)? https://codereview.chromium.org/2481873002/diff/20001/google_apis/gcm/engine/... File google_apis/gcm/engine/connection_event_tracker.h (right): https://codereview.chromium.org/2481873002/diff/20001/google_apis/gcm/engine/... google_apis/gcm/engine/connection_event_tracker.h:46: // Records the current type of network connection. micro nit: // To be called when the network connectivity changes. (I don't think "Records" is completely appropriate.) Or just remove altogether. I don't think either is particularly valuable. https://codereview.chromium.org/2481873002/diff/20001/google_apis/gcm/engine/... google_apis/gcm/engine/connection_event_tracker.h:52: // Write any previous connection information to the login request. nit: ... information to the |*login_request|. and rename the argument to login_request https://codereview.chromium.org/2481873002/diff/20001/google_apis/gcm/engine/... google_apis/gcm/engine/connection_event_tracker.h:65: }; DISALLOW_COPY_AND_ASSIGN https://codereview.chromium.org/2481873002/diff/20001/google_apis/gcm/engine/... File google_apis/gcm/engine/connection_event_tracker_unittest.cc (right): https://codereview.chromium.org/2481873002/diff/20001/google_apis/gcm/engine/... google_apis/gcm/engine/connection_event_tracker_unittest.cc:11: namespace gcm { micro nit: wrap in an inner anonymous namespace https://codereview.chromium.org/2481873002/diff/20001/google_apis/gcm/engine/... google_apis/gcm/engine/connection_event_tracker_unittest.cc:15: ConnectionEventTrackerTest() {} nit: I think you can just delete the constructor. Since you're in a .cc, the compiler will do the right thing. https://codereview.chromium.org/2481873002/diff/20001/google_apis/gcm/engine/... File google_apis/gcm/engine/connection_factory_impl.cc (right): https://codereview.chromium.org/2481873002/diff/20001/google_apis/gcm/engine/... google_apis/gcm/engine/connection_factory_impl.cc:210: event_tracker_.EndConnectionAttempt(); 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. https://codereview.chromium.org/2481873002/diff/20001/google_apis/gcm/engine/... google_apis/gcm/engine/connection_factory_impl.cc:267: // Let the connection tracker know about the network type. nit: I'd drop the comment. It's already clear from the call. https://codereview.chromium.org/2481873002/diff/20001/google_apis/gcm/engine/... google_apis/gcm/engine/connection_factory_impl.cc:339: event_tracker_.WriteToLoginRequest(&login_request); 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. https://codereview.chromium.org/2481873002/diff/20001/google_apis/gcm/engine/... google_apis/gcm/engine/connection_factory_impl.cc:438: event_tracker_.ConnectionAttemptSucceeded(); It seems to me that, logically, this would be a better fit in OnConnectDone(). (Around line 406, supposedly.) https://codereview.chromium.org/2481873002/diff/20001/google_apis/gcm/engine/... File google_apis/gcm/engine/connection_factory_impl.h (right): https://codereview.chromium.org/2481873002/diff/20001/google_apis/gcm/engine/... google_apis/gcm/engine/connection_factory_impl.h:116: ConnectionEventTracker event_tracker_; On 2016/12/01 18:51:49, Nicolas Zea wrote: > nit: protected members are kind of discouraged. I think it would be cleaner to > just inject this in at construction time if you want access to it in a test. +1 (our C++ style guide outright forbids this except for testing)
https://codereview.chromium.org/2481873002/diff/20001/google_apis/gcm/engine/... File google_apis/gcm/engine/connection_event_tracker.cc (right): https://codereview.chromium.org/2481873002/diff/20001/google_apis/gcm/engine/... google_apis/gcm/engine/connection_event_tracker.cc:14: const int kMaxClientEvents = 30; On 2016/12/01 18:51:49, Nicolas Zea wrote: > nit: newline after Done. https://codereview.chromium.org/2481873002/diff/20001/google_apis/gcm/engine/... google_apis/gcm/engine/connection_event_tracker.cc:14: const int kMaxClientEvents = 30; On 2016/12/02 15:19:13, Peter Beverloo wrote: > nit: constexpr size_t kMaxClientEvents > > (std::deque::size returns an unsigned integral type.) Done. https://codereview.chromium.org/2481873002/diff/20001/google_apis/gcm/engine/... google_apis/gcm/engine/connection_event_tracker.cc:15: } On 2016/12/01 18:51:49, Nicolas Zea wrote: > nit: > } // namespace Done. https://codereview.chromium.org/2481873002/diff/20001/google_apis/gcm/engine/... google_apis/gcm/engine/connection_event_tracker.cc:19: ConnectionEventTracker::ConnectionEventTracker() {} On 2016/12/02 15:19:13, Peter Beverloo wrote: > {} --> = default; Done. https://codereview.chromium.org/2481873002/diff/20001/google_apis/gcm/engine/... google_apis/gcm/engine/connection_event_tracker.cc:25: EndConnectionAttempt(); On 2016/12/02 15:19:13, Peter Beverloo wrote: > 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. 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. https://codereview.chromium.org/2481873002/diff/20001/google_apis/gcm/engine/... google_apis/gcm/engine/connection_event_tracker.cc:30: // connection? On 2016/12/02 15:19:13, Peter Beverloo wrote: > 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. 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. https://codereview.chromium.org/2481873002/diff/20001/google_apis/gcm/engine/... google_apis/gcm/engine/connection_event_tracker.cc:58: completed_events_.resize(0); On 2016/12/02 15:19:13, Peter Beverloo wrote: > nit: why not clear()? 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(). https://codereview.chromium.org/2481873002/diff/20001/google_apis/gcm/engine/... google_apis/gcm/engine/connection_event_tracker.cc:65: mcs_proto::ClientEvent::SUCCESSFUL_CONNECTION); On 2016/12/02 15:19:13, Peter Beverloo wrote: > nit: I think we should be able to use DCHECK_EQ() here now. Dito elsewhere. Done. https://codereview.chromium.org/2481873002/diff/20001/google_apis/gcm/engine/... google_apis/gcm/engine/connection_event_tracker.cc:69: // TODO(harkness): What error code should be used here? On 2016/12/01 18:51:49, Nicolas Zea wrote: > On 2016/12/01 10:34:34, harkness wrote: > > 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. > > You're currently only calling this from SignalConnectionReset. Perhaps > CONNECTION_RESET? That sounds good, thanks! https://codereview.chromium.org/2481873002/diff/20001/google_apis/gcm/engine/... google_apis/gcm/engine/connection_event_tracker.cc:80: mcs_proto::LoginRequest* request) { On 2016/12/02 15:19:13, Peter Beverloo wrote: > pedantic nit: DCHECK(request)? Done. https://codereview.chromium.org/2481873002/diff/20001/google_apis/gcm/engine/... File google_apis/gcm/engine/connection_event_tracker.h (right): https://codereview.chromium.org/2481873002/diff/20001/google_apis/gcm/engine/... google_apis/gcm/engine/connection_event_tracker.h:46: // Records the current type of network connection. On 2016/12/02 15:19:13, Peter Beverloo wrote: > micro nit: > // To be called when the network connectivity changes. > > (I don't think "Records" is completely appropriate.) Or just remove altogether. > I don't think either is particularly valuable. Removing entirely as Nicolas referred me to a method I can use on demand instead. https://codereview.chromium.org/2481873002/diff/20001/google_apis/gcm/engine/... google_apis/gcm/engine/connection_event_tracker.h:47: void OnNetworkChanged( On 2016/12/01 18:51:49, Nicolas Zea wrote: > nit: Only simple accessors should be inlined (and in that case named with > unix_hacker_style). Prefer out-of-lining this method. Removing entirely. https://codereview.chromium.org/2481873002/diff/20001/google_apis/gcm/engine/... google_apis/gcm/engine/connection_event_tracker.h:52: // Write any previous connection information to the login request. On 2016/12/02 15:19:13, Peter Beverloo wrote: > nit: > ... information to the |*login_request|. > > and rename the argument to login_request Done. https://codereview.chromium.org/2481873002/diff/20001/google_apis/gcm/engine/... google_apis/gcm/engine/connection_event_tracker.h:65: }; On 2016/12/02 15:19:13, Peter Beverloo wrote: > DISALLOW_COPY_AND_ASSIGN Done. https://codereview.chromium.org/2481873002/diff/20001/google_apis/gcm/engine/... File google_apis/gcm/engine/connection_event_tracker_unittest.cc (right): https://codereview.chromium.org/2481873002/diff/20001/google_apis/gcm/engine/... google_apis/gcm/engine/connection_event_tracker_unittest.cc:11: namespace gcm { On 2016/12/02 15:19:13, Peter Beverloo wrote: > micro nit: wrap in an inner anonymous namespace Done. https://codereview.chromium.org/2481873002/diff/20001/google_apis/gcm/engine/... google_apis/gcm/engine/connection_event_tracker_unittest.cc:15: ConnectionEventTrackerTest() {} On 2016/12/02 15:19:13, Peter Beverloo wrote: > nit: I think you can just delete the constructor. Since you're in a .cc, the > compiler will do the right thing. Done. https://codereview.chromium.org/2481873002/diff/20001/google_apis/gcm/engine/... File google_apis/gcm/engine/connection_factory_impl.cc (right): https://codereview.chromium.org/2481873002/diff/20001/google_apis/gcm/engine/... google_apis/gcm/engine/connection_factory_impl.cc:102: waiting_for_network_online_ = net::NetworkChangeNotifier::IsOffline(); On 2016/12/01 18:51:49, Nicolas Zea wrote: > You probably need to notify the event tracker that network type changed here to > net::NetworkChangeNotifier::GetConnectionType() Ah! That's exactly the method I needed, and somehow I overlooked. Rather than storing/updating the network type, I'm just going to query it when we build the event. https://codereview.chromium.org/2481873002/diff/20001/google_apis/gcm/engine/... google_apis/gcm/engine/connection_factory_impl.cc:210: event_tracker_.EndConnectionAttempt(); On 2016/12/02 15:19:13, Peter Beverloo wrote: > 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. 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. https://codereview.chromium.org/2481873002/diff/20001/google_apis/gcm/engine/... google_apis/gcm/engine/connection_factory_impl.cc:267: // Let the connection tracker know about the network type. On 2016/12/02 15:19:13, Peter Beverloo wrote: > nit: I'd drop the comment. It's already clear from the call. Removed. https://codereview.chromium.org/2481873002/diff/20001/google_apis/gcm/engine/... google_apis/gcm/engine/connection_factory_impl.cc:339: event_tracker_.WriteToLoginRequest(&login_request); On 2016/12/02 15:19:13, Peter Beverloo wrote: > 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. Nice catch! It looks like this isn't caught by the tests because they use an empty BuildLoginRequestCallback method. https://codereview.chromium.org/2481873002/diff/20001/google_apis/gcm/engine/... google_apis/gcm/engine/connection_factory_impl.cc:438: event_tracker_.ConnectionAttemptSucceeded(); On 2016/12/02 15:19:13, Peter Beverloo wrote: > It seems to me that, logically, this would be a better fit in OnConnectDone(). > (Around line 406, supposedly.) 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. https://codereview.chromium.org/2481873002/diff/20001/google_apis/gcm/engine/... File google_apis/gcm/engine/connection_factory_impl.h (right): https://codereview.chromium.org/2481873002/diff/20001/google_apis/gcm/engine/... google_apis/gcm/engine/connection_factory_impl.h:116: ConnectionEventTracker event_tracker_; On 2016/12/02 15:19:13, Peter Beverloo wrote: > On 2016/12/01 18:51:49, Nicolas Zea wrote: > > nit: protected members are kind of discouraged. I think it would be cleaner to > > just inject this in at construction time if you want access to it in a test. > > +1 (our C++ style guide outright forbids this except for testing) Moved it and added a ForTesting accessor.
lgtm https://codereview.chromium.org/2481873002/diff/20001/google_apis/gcm/engine/... File google_apis/gcm/engine/connection_factory_impl.cc (right): https://codereview.chromium.org/2481873002/diff/20001/google_apis/gcm/engine/... google_apis/gcm/engine/connection_factory_impl.cc:210: event_tracker_.EndConnectionAttempt(); On 2016/12/05 15:39:01, harkness wrote: > On 2016/12/02 15:19:13, Peter Beverloo wrote: > > 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. > > 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. 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. https://codereview.chromium.org/2481873002/diff/20001/google_apis/gcm/engine/... google_apis/gcm/engine/connection_factory_impl.cc:438: event_tracker_.ConnectionAttemptSucceeded(); On 2016/12/05 15:39:01, harkness wrote: > On 2016/12/02 15:19:13, Peter Beverloo wrote: > > It seems to me that, logically, this would be a better fit in OnConnectDone(). > > (Around line 406, supposedly.) > > 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. Acknowledged. https://codereview.chromium.org/2481873002/diff/40001/google_apis/gcm/engine/... File google_apis/gcm/engine/connection_event_tracker.cc (right): https://codereview.chromium.org/2481873002/diff/40001/google_apis/gcm/engine/... google_apis/gcm/engine/connection_event_tracker.cc:8: #include "google_apis/gcm/protocol/mcs.pb.h" nit: unused (already included in the header) https://codereview.chromium.org/2481873002/diff/40001/google_apis/gcm/engine/... google_apis/gcm/engine/connection_event_tracker.cc:29: net::NetworkChangeNotifier::GetConnectionType()); two thoughts: (1) maybe add a static_cast<int> for clarity? it looks like the implicit cast is working fine, but it may be good to be explicit about it. (2) add a comment saying that the values should remain consistent since we store them on the server. acts as a bit of a safe-guard in case someone comes by and refactors the enum. https://codereview.chromium.org/2481873002/diff/40001/google_apis/gcm/engine/... google_apis/gcm/engine/connection_event_tracker.cc:32: void ConnectionEventTracker::EndConnectionAttempt() { nit: DCHECK(current_event_.has_type()); (The type should *always* have been set at this point, right?) https://codereview.chromium.org/2481873002/diff/40001/google_apis/gcm/engine/... File google_apis/gcm/engine/connection_event_tracker.h (right): https://codereview.chromium.org/2481873002/diff/40001/google_apis/gcm/engine/... google_apis/gcm/engine/connection_event_tracker.h:14: #include "net/base/network_change_notifier.h" nit: unused https://codereview.chromium.org/2481873002/diff/40001/google_apis/gcm/engine/... google_apis/gcm/engine/connection_event_tracker.h:17: class LoginRequest; nit: unused (you include mcs.pb.h) https://codereview.chromium.org/2481873002/diff/40001/google_apis/gcm/engine/... File google_apis/gcm/engine/connection_factory_impl.h (right): https://codereview.chromium.org/2481873002/diff/40001/google_apis/gcm/engine/... google_apis/gcm/engine/connection_factory_impl.h:82: ConnectionEventTracker* GetEventTrackerForTesting() { nit: prefer making this private w/ a friends declaration https://codereview.chromium.org/2481873002/diff/40001/google_apis/gcm/engine/... google_apis/gcm/engine/connection_factory_impl.h:123: // Implementation of Connect(..). If not in backoff, uses |login_request_| nit: |login_request_| does not exist. Mind updating the comment? https://codereview.chromium.org/2481873002/diff/40001/google_apis/gcm/engine/... google_apis/gcm/engine/connection_factory_impl.h:142: // whether they suceeded, and their duration. nit: s/suceeded/succeeded/ https://codereview.chromium.org/2481873002/diff/40001/google_apis/gcm/engine/... File google_apis/gcm/engine/connection_factory_impl_unittest.cc (right): https://codereview.chromium.org/2481873002/diff/40001/google_apis/gcm/engine/... google_apis/gcm/engine/connection_factory_impl_unittest.cc:118: GetClientEventsForTest() { nit: I'd drop "ForTest" since you're in a test. Up to you. https://codereview.chromium.org/2481873002/diff/40001/google_apis/gcm/engine/... google_apis/gcm/engine/connection_factory_impl_unittest.cc:380: const auto& client_events = factory()->GetClientEventsForTest(); nit (and on line 395, 603 and 617): GetClientEventsForTest returns a value, not a reference. Drop the "&". (It makes a copy.) https://codereview.chromium.org/2481873002/diff/40001/google_apis/gcm/engine/... google_apis/gcm/engine/connection_factory_impl_unittest.cc:383: for (const auto client_event : client_events) { nit (and on line 606): No need to copy the individual protobufs, so we can change this to "const auto&" to avoid that.
LGTM modulo Peter's comments/maybe adding back the DCHECK mentioned below https://codereview.chromium.org/2481873002/diff/40001/google_apis/gcm/engine/... File google_apis/gcm/engine/connection_event_tracker.cc (right): https://codereview.chromium.org/2481873002/diff/40001/google_apis/gcm/engine/... google_apis/gcm/engine/connection_event_tracker.cc:25: // TODO(harkness): Can we dcheck here that there is not an in progress I think you should be able to (the connection factory only has one open socket at a time, via socket_handle_).
https://codereview.chromium.org/2481873002/diff/40001/google_apis/gcm/engine/... File google_apis/gcm/engine/connection_event_tracker.cc (right): https://codereview.chromium.org/2481873002/diff/40001/google_apis/gcm/engine/... google_apis/gcm/engine/connection_event_tracker.cc:8: #include "google_apis/gcm/protocol/mcs.pb.h" On 2016/12/05 16:29:34, Peter Beverloo wrote: > nit: unused (already included in the header) Done. https://codereview.chromium.org/2481873002/diff/40001/google_apis/gcm/engine/... google_apis/gcm/engine/connection_event_tracker.cc:25: // TODO(harkness): Can we dcheck here that there is not an in progress On 2016/12/05 18:36:15, Nicolas Zea wrote: > I think you should be able to (the connection factory only has one open socket > at a time, via socket_handle_). I'll experiment with that in a follow-up patch, and send you an e-mail with the comments in connection_factory_impl.cc that make me wonder if it's valid. https://codereview.chromium.org/2481873002/diff/40001/google_apis/gcm/engine/... google_apis/gcm/engine/connection_event_tracker.cc:29: net::NetworkChangeNotifier::GetConnectionType()); On 2016/12/05 16:29:34, Peter Beverloo wrote: > two thoughts: > (1) maybe add a static_cast<int> for clarity? it looks like the implicit cast > is working fine, but it may be good to be explicit about it. > (2) add a comment saying that the values should remain consistent since we > store them on the server. acts as a bit of a safe-guard in case someone comes by > and refactors the enum. Done. https://codereview.chromium.org/2481873002/diff/40001/google_apis/gcm/engine/... google_apis/gcm/engine/connection_event_tracker.cc:32: void ConnectionEventTracker::EndConnectionAttempt() { On 2016/12/05 16:29:34, Peter Beverloo wrote: > nit: > DCHECK(current_event_.has_type()); > > (The type should *always* have been set at this point, right?) It should have been set, however, the test code does a bunch of explicit manipulation that doesn't respect the correct ordering of things. I fixed the first test case where it was broken once I added the DCHECK, but there were more. If you think it's worthwhile I can add the DCHECK and see about fixing the test cases in a follow-up. https://codereview.chromium.org/2481873002/diff/40001/google_apis/gcm/engine/... File google_apis/gcm/engine/connection_event_tracker.h (right): https://codereview.chromium.org/2481873002/diff/40001/google_apis/gcm/engine/... google_apis/gcm/engine/connection_event_tracker.h:14: #include "net/base/network_change_notifier.h" On 2016/12/05 16:29:34, Peter Beverloo wrote: > nit: unused Moved to the cc file, where we still need it. https://codereview.chromium.org/2481873002/diff/40001/google_apis/gcm/engine/... google_apis/gcm/engine/connection_event_tracker.h:17: class LoginRequest; On 2016/12/05 16:29:34, Peter Beverloo wrote: > nit: unused (you include mcs.pb.h) Done. https://codereview.chromium.org/2481873002/diff/40001/google_apis/gcm/engine/... File google_apis/gcm/engine/connection_factory_impl.h (right): https://codereview.chromium.org/2481873002/diff/40001/google_apis/gcm/engine/... google_apis/gcm/engine/connection_factory_impl.h:82: ConnectionEventTracker* GetEventTrackerForTesting() { On 2016/12/05 16:29:35, Peter Beverloo wrote: > nit: prefer making this private w/ a friends declaration Done. https://codereview.chromium.org/2481873002/diff/40001/google_apis/gcm/engine/... google_apis/gcm/engine/connection_factory_impl.h:123: // Implementation of Connect(..). If not in backoff, uses |login_request_| On 2016/12/05 16:29:35, Peter Beverloo wrote: > nit: |login_request_| does not exist. Mind updating the comment? Done. https://codereview.chromium.org/2481873002/diff/40001/google_apis/gcm/engine/... google_apis/gcm/engine/connection_factory_impl.h:142: // whether they suceeded, and their duration. On 2016/12/05 16:29:35, Peter Beverloo wrote: > nit: s/suceeded/succeeded/ Done. https://codereview.chromium.org/2481873002/diff/40001/google_apis/gcm/engine/... File google_apis/gcm/engine/connection_factory_impl_unittest.cc (right): https://codereview.chromium.org/2481873002/diff/40001/google_apis/gcm/engine/... google_apis/gcm/engine/connection_factory_impl_unittest.cc:118: GetClientEventsForTest() { On 2016/12/05 16:29:35, Peter Beverloo wrote: > nit: I'd drop "ForTest" since you're in a test. Up to you. Done. https://codereview.chromium.org/2481873002/diff/40001/google_apis/gcm/engine/... google_apis/gcm/engine/connection_factory_impl_unittest.cc:380: const auto& client_events = factory()->GetClientEventsForTest(); On 2016/12/05 16:29:35, Peter Beverloo wrote: > nit (and on line 395, 603 and 617): GetClientEventsForTest returns a value, not > a reference. Drop the "&". > > (It makes a copy.) Done. https://codereview.chromium.org/2481873002/diff/40001/google_apis/gcm/engine/... google_apis/gcm/engine/connection_factory_impl_unittest.cc:383: for (const auto client_event : client_events) { On 2016/12/05 16:29:35, Peter Beverloo wrote: > nit (and on line 606): No need to copy the individual protobufs, so we can > change this to "const auto&" to avoid that. Done.
The CQ bit was checked by harkness@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from peter@chromium.org, zea@chromium.org Link to the patchset: https://codereview.chromium.org/2481873002/#ps60001 (title: "Integrated code review comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1481028840771740,
"parent_rev": "d71e2d4365cac3b053548f0582dbc639bf317ec4", "commit_rev":
"a150a208fe7d824e61f4496a465e344cdc74e29d"}
Message was sent while issue was closed.
Description was changed from ========== Added ClientEvent proto and structure for storing events in the factory. As part of the project to record GCM connection errors in Chrome, this CL adds the ClientEvent to the proto for the LoginRequest that goes to GCM. It also adds local structures to record previous connection attempts, although persisting the information and using it to construct the login request is in a subsequent CL. Tests were also updated to check that events were being recorded corectly in old test cases, and a new test case was added to check wrapping on the storage. BUG=662983 ========== to ========== Added ClientEvent proto and structure for storing events in the factory. As part of the project to record GCM connection errors in Chrome, this CL adds the ClientEvent to the proto for the LoginRequest that goes to GCM. It also adds local structures to record previous connection attempts, although persisting the information and using it to construct the login request is in a subsequent CL. Tests were also updated to check that events were being recorded corectly in old test cases, and a new test case was added to check wrapping on the storage. BUG=662983 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Added ClientEvent proto and structure for storing events in the factory. As part of the project to record GCM connection errors in Chrome, this CL adds the ClientEvent to the proto for the LoginRequest that goes to GCM. It also adds local structures to record previous connection attempts, although persisting the information and using it to construct the login request is in a subsequent CL. Tests were also updated to check that events were being recorded corectly in old test cases, and a new test case was added to check wrapping on the storage. BUG=662983 ========== to ========== Added ClientEvent proto and structure for storing events in the factory. As part of the project to record GCM connection errors in Chrome, this CL adds the ClientEvent to the proto for the LoginRequest that goes to GCM. It also adds local structures to record previous connection attempts, although persisting the information and using it to construct the login request is in a subsequent CL. Tests were also updated to check that events were being recorded corectly in old test cases, and a new test case was added to check wrapping on the storage. BUG=662983 Committed: https://crrev.com/1a49d60bd22ef9ccb97619ba3441bbd89f4b0c3e Cr-Commit-Position: refs/heads/master@{#436584} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/1a49d60bd22ef9ccb97619ba3441bbd89f4b0c3e Cr-Commit-Position: refs/heads/master@{#436584} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
