From 9c5c3cc45f9ee2dc3051dd41b94e98228f1d8d4e Mon Sep 17 00:00:00 2001 From: Yuwei Huang Date: Thu, 18 Jan 2018 20:10:12 +0000 Subject: [PATCH] [Remoting Mobile] Rewire connection state reporting logic for telemetry This CL improves the connection state report accuracy such that: * If session is disconnected before the CONNECTED state, report a CANCEL event. Previously nothing is reported in this state. * If session is disconnected during the CONNECTED state, report a CLOSE event. * On iOS, report an AUTHENTICATION_FAILED error if the user tries to do a third party auth, which is not supported. Previously we don't report anything and when this happens. Bug: 793995 Change-Id: I9155c0efe43892149179e70f52ecee17434ff88e Reviewed-on: https://chromium-review.googlesource.com/869457 Reviewed-by: Jamie Walch Commit-Queue: Yuwei Huang Cr-Commit-Position: refs/heads/master@{#530249} --- remoting/client/chromoting_session.cc | 40 +++++++++++++++++-------- remoting/client/chromoting_session.h | 14 +++++---- remoting/ios/session/remoting_client.mm | 2 ++ 3 files changed, 39 insertions(+), 17 deletions(-) diff --git a/remoting/client/chromoting_session.cc b/remoting/client/chromoting_session.cc index 0f1dbfdcd1d05b..48948b1643674b 100644 --- a/remoting/client/chromoting_session.cc +++ b/remoting/client/chromoting_session.cc @@ -114,21 +114,37 @@ void ChromotingSession::Connect() { } void ChromotingSession::Disconnect() { + DisconnectForReason(protocol::ErrorCode::OK); +} + +void ChromotingSession::DisconnectForReason(protocol::ErrorCode error) { if (!runtime_->network_task_runner()->BelongsToCurrentThread()) { runtime_->network_task_runner()->PostTask( - FROM_HERE, base::Bind(&ChromotingSession::Disconnect, GetWeakPtr())); + FROM_HERE, base::BindOnce(&ChromotingSession::DisconnectForReason, + GetWeakPtr(), error)); return; } - stats_logging_enabled_ = false; - - // User disconnection will not trigger OnConnectionState(Closed, OK). - // Remote disconnection will trigger OnConnectionState(...) and later trigger - // Disconnect(). - if (connected_) { - logger_->LogSessionStateChange(ChromotingEvent::SessionState::CLOSED, - ChromotingEvent::ConnectionError::NONE); - connected_ = false; + EnableStatsLogging(false); + + // Do not log session state change if the connection is never started or is + // already closed. + if (session_state_ != protocol::ConnectionToHost::INITIALIZING && + session_state_ != protocol::ConnectionToHost::FAILED && + session_state_ != protocol::ConnectionToHost::CLOSED) { + ChromotingEvent::SessionState session_state_to_log; + if (error != protocol::ErrorCode::OK) { + session_state_to_log = ChromotingEvent::SessionState::CONNECTION_FAILED; + } else if (session_state_ == protocol::ConnectionToHost::CONNECTED) { + session_state_to_log = ChromotingEvent::SessionState::CLOSED; + } else { + session_state_to_log = ChromotingEvent::SessionState::CONNECTION_CANCELED; + } + logger_->LogSessionStateChange( + session_state_to_log, ClientTelemetryLogger::TranslateError(error)); + session_state_ = (error == protocol::ErrorCode::OK) + ? protocol::ConnectionToHost::CLOSED + : protocol::ConnectionToHost::FAILED; } ReleaseResources(); @@ -317,8 +333,8 @@ void ChromotingSession::OnConnectionState( // This code assumes no intermediate connection state between CONNECTED and // CLOSED/FAILED. - connected_ = state == protocol::ConnectionToHost::CONNECTED; - EnableStatsLogging(connected_); + session_state_ = state; + EnableStatsLogging(session_state_ == protocol::ConnectionToHost::CONNECTED); logger_->LogSessionStateChange(ClientTelemetryLogger::TranslateState(state), ClientTelemetryLogger::TranslateError(error)); diff --git a/remoting/client/chromoting_session.h b/remoting/client/chromoting_session.h index a498f7a9edbc0d..2fab111805ec0b 100644 --- a/remoting/client/chromoting_session.h +++ b/remoting/client/chromoting_session.h @@ -90,14 +90,18 @@ class ChromotingSession : public ClientUserInterface, // Must be called before destruction. void Disconnect(); - // Requests the android app to fetch a third-party token. + // Similar to Disconnect(), except that this method allows you to specify the + // reason to disconnect, which will be reported to telemetry. + void DisconnectForReason(protocol::ErrorCode error); + + // Requests the client to fetch a third-party token. void FetchThirdPartyToken( const std::string& host_public_key, const std::string& token_url, const std::string& scope, const protocol::ThirdPartyTokenFetchedCallback& token_fetched_callback); - // Called by the android app when the token is fetched. + // Called by the client when the token is fetched. void HandleOnThirdPartyTokenFetched(const std::string& token, const std::string& shared_secret); @@ -211,9 +215,9 @@ class ChromotingSession : public ClientUserInterface, // set of capabilities for this remoting session. std::string capabilities_; - // Indicates whether the client is connected to the host. Used on network - // thread. - bool connected_ = false; + // The current session state. Used on network thread. + protocol::ConnectionToHost::State session_state_ = + protocol::ConnectionToHost::INITIALIZING; std::unique_ptr logger_; diff --git a/remoting/ios/session/remoting_client.mm b/remoting/ios/session/remoting_client.mm index 17130394ba3b57..52bb0d7c5bb910 100644 --- a/remoting/ios/session/remoting_client.mm +++ b/remoting/ios/session/remoting_client.mm @@ -327,6 +327,8 @@ - (void)fetchThirdPartyTokenForUrl:(NSString*)tokenUrl // Not supported for iOS yet. _sessionDetails.state = SessionFailed; _sessionDetails.error = SessionErrorThirdPartyAuthNotSupported; + _session->DisconnectForReason( + remoting::protocol::ErrorCode::AUTHENTICATION_FAILED); [[NSNotificationCenter defaultCenter] postNotificationName:kHostSessionStatusChanged object:self