From 0057b219d28ea28f1d6157ef51fc21e8cc755fa8 Mon Sep 17 00:00:00 2001 From: Edwin Wu Date: Wed, 19 Feb 2025 19:14:22 -0800 Subject: [PATCH] In windows, discovery phase and outgoing attempt won't be recorded due to pointer is reset to null. PiperOrigin-RevId: 728907296 --- .../analytics/analytics_recorder.cc | 60 ++++++++----------- .../analytics/analytics_recorder.h | 7 +-- .../implementation/base_pcp_handler.cc | 3 + 3 files changed, 30 insertions(+), 40 deletions(-) diff --git a/connections/implementation/analytics/analytics_recorder.cc b/connections/implementation/analytics/analytics_recorder.cc index 15d8ea48..ca68d609 100644 --- a/connections/implementation/analytics/analytics_recorder.cc +++ b/connections/implementation/analytics/analytics_recorder.cc @@ -176,8 +176,6 @@ AnalyticsRecorder::AnalyticsRecorder(EventLogger *event_logger, AnalyticsRecorder::~AnalyticsRecorder() { serial_executor_.Shutdown(); - MutexLock lock(&mutex_); - ResetClientSessionLoggingResoucesLocked(); } bool AnalyticsRecorder::IsSessionLogged() { @@ -185,27 +183,6 @@ bool AnalyticsRecorder::IsSessionLogged() { return session_was_logged_; } -void AnalyticsRecorder::ResetClientSessionLoggingResoucesLocked() { - NEARBY_LOGS(INFO) << "Reset AnalyticsRecorder ctor event_logger_=" - << event_logger_; - - incoming_connection_requests_.clear(); - outgoing_connection_requests_.clear(); - active_connections_.clear(); - bandwidth_upgrade_attempts_.clear(); - - client_session_ = nullptr; - session_was_logged_ = true; - start_client_session_was_logged_ = false; - current_strategy_ = - connections::Strategy::kNone; // Need to reset since the same strategy - // should be logged separately for - // different client sessions. - current_strategy_session_ = nullptr; - current_advertising_phase_ = nullptr; - current_discovery_phase_ = nullptr; -} - int AnalyticsRecorder::GetLatestUpdateIndexLocked( const std::vector &list) { int latest_update_index = 0; @@ -358,7 +335,6 @@ void AnalyticsRecorder::OnStoppedIncomingConnectionListening() { return; } RecordAdvertisingPhaseDurationAndReasonLocked(/* on_stop= */ false); - // RecordAdvertisingPhaseDurationLocked(); } void AnalyticsRecorder::OnEndpointFound(Medium medium) { @@ -947,6 +923,7 @@ void AnalyticsRecorder::LogSession() { } LogClientSessionLocked(); LogEvent(STOP_CLIENT_SESSION); + start_client_session_was_logged_ = false; session_was_logged_ = true; } @@ -1049,21 +1026,24 @@ bool AnalyticsRecorder::CanRecordAnalyticsLocked( return true; } +// TODO: b/391339677 - Investigate why we need to reset the resources. And +// verify in b/238375695 to see if we still meet the issue after removing the +// Reset function. void AnalyticsRecorder::LogClientSessionLocked() { + ConnectionsLog connections_log; + connections_log.set_event_type(CLIENT_SESSION); + connections_log.set_allocated_client_session(client_session_.release()); + connections_log.set_version(kVersion); + client_session_ = nullptr; + serial_executor_.Execute( "analytics-recorder", - [this, client_session = std::move(client_session_)]() mutable { - ConnectionsLog connections_log; - connections_log.set_event_type(CLIENT_SESSION); - connections_log.set_allocated_client_session(client_session.release()); - connections_log.set_version(kVersion); - + [this, connections_log = std::move(connections_log)]() mutable { NEARBY_VLOG(1) << "AnalyticsRecorder LogClientSession connections_log=" << connections_log.DebugString(); // NOLINT event_logger_->Log(connections_log); }); - ResetClientSessionLoggingResoucesLocked(); } void AnalyticsRecorder::LogEvent(EventType event_type) { @@ -1144,8 +1124,13 @@ void AnalyticsRecorder::FinishAdvertisingPhaseLocked() { UpdateAdvertiserConnectionRequestLocked(connection_request.get()); } RecordAdvertisingPhaseDurationAndReasonLocked(/* on_stop= */ false); - *current_strategy_session_->add_advertising_phase() = - *std::move(current_advertising_phase_); + if (current_strategy_session_ != nullptr) { + *current_strategy_session_->add_advertising_phase() = + *std::move(current_advertising_phase_); + } else { + NEARBY_LOGS(INFO) << "Unable to record advertising phase due to null " + "current_strategy_session_"; + } } incoming_connection_requests_.clear(); } @@ -1181,8 +1166,13 @@ void AnalyticsRecorder::FinishDiscoveryPhaseLocked() { UpdateDiscovererConnectionRequestLocked(connection_request.get()); } RecordDiscoveryPhaseDurationAndReasonLocked(/* on_stop=*/false); - *current_strategy_session_->add_discovery_phase() = - *std::move(current_discovery_phase_); + if (current_strategy_session_ != nullptr) { + *current_strategy_session_->add_discovery_phase() = + *std::move(current_discovery_phase_); + } else { + NEARBY_LOGS(INFO) << "Unable to record discovery phase due to null " + "current_strategy_session_"; + } } outgoing_connection_requests_.clear(); } diff --git a/connections/implementation/analytics/analytics_recorder.h b/connections/implementation/analytics/analytics_recorder.h index 64bd9b7e..3f7070e8 100644 --- a/connections/implementation/analytics/analytics_recorder.h +++ b/connections/implementation/analytics/analytics_recorder.h @@ -448,11 +448,6 @@ class AnalyticsRecorder { bool erase_item = true) ABSL_SHARED_LOCKS_REQUIRED(mutex_); void FinishStrategySessionLocked() ABSL_EXCLUSIVE_LOCKS_REQUIRED(mutex_); - // Reset the client cession's logging resources (e.g. current_strategy_, - // current_advertising_phase_, current_discovery_phase_, etc) - void ResetClientSessionLoggingResoucesLocked() - ABSL_EXCLUSIVE_LOCKS_REQUIRED(mutex_); - int GetLatestUpdateIndexLocked( const std::vector &list) @@ -467,6 +462,8 @@ class AnalyticsRecorder { // that outlives the one constructed. ::nearby::analytics::EventLogger *event_logger_; + // TODO: b/391339677 - Removing this thread to prevent threading issue when + // logging. SingleThreadExecutor serial_executor_; // Protects all sub-protos reading and writing in ConnectionLog. Mutex mutex_; diff --git a/connections/implementation/base_pcp_handler.cc b/connections/implementation/base_pcp_handler.cc index 6e44fd86..efa1168f 100644 --- a/connections/implementation/base_pcp_handler.cc +++ b/connections/implementation/base_pcp_handler.cc @@ -883,6 +883,9 @@ Status BasePcpHandler::RequestConnection( "to endpoint_id=" << endpoint_id; + client->OnRequestConnection(GetStrategy(), endpoint_id, + connection_options); + ConnectionInfo connection_info = FillConnectionInfo(client, info, connection_options);