From ff412edde1370a77b5109b1ca899ab63c94a6639 Mon Sep 17 00:00:00 2001 From: hai007 Date: Thu, 4 May 2023 13:39:39 -0700 Subject: [PATCH] Skip the analytics recording on the closed connection without strategy sessions PiperOrigin-RevId: 529502206 --- connections/implementation/analytics/BUILD | 2 + .../analytics/analytics_recorder.cc | 30 ++-- .../analytics/analytics_recorder_test.cc | 130 ++++++++++++------ 3 files changed, 105 insertions(+), 57 deletions(-) diff --git a/connections/implementation/analytics/BUILD b/connections/implementation/analytics/BUILD index cf86a737..b27f01c3 100644 --- a/connections/implementation/analytics/BUILD +++ b/connections/implementation/analytics/BUILD @@ -36,6 +36,7 @@ cc_library( "//internal/platform:types", "//internal/proto/analytics:connections_log_cc_proto", "//proto:connections_enums_cc_proto", + "@com_google_absl//absl/algorithm:container", "@com_google_absl//absl/container:btree", "@com_google_absl//absl/container:flat_hash_map", "@com_google_absl//absl/time", @@ -58,6 +59,7 @@ cc_test( "//internal/platform:types", "//internal/platform/implementation/g3", # build_cleaner: keep "//internal/proto/analytics:connections_log_cc_proto", + "//net/proto2/contrib/parse_proto:parse_text_proto", "//proto:connections_enums_cc_proto", "@com_github_protobuf_matchers//protobuf-matchers", "@com_google_absl//absl/time", diff --git a/connections/implementation/analytics/analytics_recorder.cc b/connections/implementation/analytics/analytics_recorder.cc index e948cbfe..e2bc8d57 100644 --- a/connections/implementation/analytics/analytics_recorder.cc +++ b/connections/implementation/analytics/analytics_recorder.cc @@ -22,6 +22,7 @@ #include #include +#include "absl/algorithm/container.h" #include "absl/time/time.h" #include "internal/analytics/event_logger.h" #include "internal/platform/logging.h" @@ -445,6 +446,14 @@ void AnalyticsRecorder::OnConnectionClosed(const std::string &endpoint_id, if (!CanRecordAnalyticsLocked("OnConnectionClosed")) { return; } + + if (current_strategy_session_ == nullptr) { + NEARBY_LOGS(VERBOSE) + << "AnalyticsRecorder CanRecordAnalytics Unexpected call " << __func__ + << " since current_strategy_session_ is required."; + return; + } + auto it = active_connections_.find(endpoint_id); if (it == active_connections_.end()) { return; @@ -457,24 +466,11 @@ void AnalyticsRecorder::OnConnectionClosed(const std::string &endpoint_id, // re-established with a new ConnectionRequest. auto pair = active_connections_.extract(it); std::unique_ptr &logical_connection = pair.mapped(); - logical_connection->GetEstablisedConnections(); - // TODO(b/245553737): the recent change in protobuf may broken the class of - // RepeatedFieldPtr. Our app will crash after sending file. The app also - // crashes even only print the size of mutable_established_connection. we - // need to reccover the code when protobuf fixes the issue. - - // auto pair = active_connections_.extract(it); - // std::unique_ptr &logical_connection = - // pair.mapped(); - - // std::vector connections = - // logical_connection->GetEstablisedConnections(); - // auto established_connections = - // current_strategy_session_->mutable_established_connection(); - // for (auto &connection : connections) { - // established_connections->Add(std::move(connection)); - // } + absl::c_copy( + logical_connection->GetEstablisedConnections(), + RepeatedFieldBackInserter( + current_strategy_session_->mutable_established_connection())); } } diff --git a/connections/implementation/analytics/analytics_recorder_test.cc b/connections/implementation/analytics/analytics_recorder_test.cc index 4c9f0c65..ecfefb37 100644 --- a/connections/implementation/analytics/analytics_recorder_test.cc +++ b/connections/implementation/analytics/analytics_recorder_test.cc @@ -17,6 +17,7 @@ #include #include +#include "net/proto2/contrib/parse_proto/parse_text_proto.h" #include "google/protobuf/message_lite.h" #include "gmock/gmock.h" #include "protobuf-matchers/protocol-buffer-matchers.h" @@ -65,6 +66,7 @@ using ::location::nearby::proto::connections::WIFI_LAN; using ::location::nearby::proto::connections::WIFI_LAN_MEDIUM_ERROR; using ::location::nearby::proto::connections::WIFI_LAN_SOCKET_CREATION; using ::nearby::analytics::EventLogger; +using ::proto2::contrib::parse_proto::ParseTextProtoOrDie; using ::testing::Contains; using ::protobuf_matchers::EqualsProto; using ::testing::Not; @@ -770,46 +772,44 @@ TEST(AnalyticsRecorderTest, OutgoingPayloadUpgraded) { analytics_recorder.LogSession(); ASSERT_TRUE(client_session_done_latch.Await(kDefaultTimeout).result()); - // TODO(b/245553737): recover the codes. - // EXPECT_THAT(event_logger.GetLoggedClientSession(), - // Partially(EqualsProto(R"pb( - // strategy_session < - // strategy: P2P_STAR - // role: ADVERTISER - // advertising_phase < - // medium: BLE - // medium: BLUETOOTH - // advertising_metadata < - // supports_extended_ble_advertisements: false - // connected_ap_frequency: 0 - // supports_nfc_technology: false - // > - // > - // established_connection < - // medium: BLUETOOTH - // sent_payload < - // type: FILE - // total_size_bytes: 50 - // num_bytes_transferred: 20 - // num_chunks: 2 - // status: MOVED_TO_NEW_MEDIUM - // > - // disconnection_reason: UPGRADED - // connection_token: "connection_token" - // > - // established_connection < - // medium: WIFI_LAN - // sent_payload < - // type: FILE - // total_size_bytes: 50 - // num_bytes_transferred: 30 - // num_chunks: 3 - // status: SUCCESS - // > - // disconnection_reason: LOCAL_DISCONNECTION - // connection_token: "connection_token" - // > - // >)pb"))); + EXPECT_THAT(event_logger.GetLoggedClientSession(), Partially(EqualsProto(R"pb( + strategy_session < + strategy: P2P_STAR + role: ADVERTISER + advertising_phase < + medium: BLE + medium: BLUETOOTH + advertising_metadata < + supports_extended_ble_advertisements: false + connected_ap_frequency: 0 + supports_nfc_technology: false + > + > + established_connection < + medium: BLUETOOTH + sent_payload < + type: FILE + total_size_bytes: 50 + num_bytes_transferred: 20 + num_chunks: 2 + status: MOVED_TO_NEW_MEDIUM + > + disconnection_reason: UPGRADED + connection_token: "connection_token" + > + established_connection < + medium: WIFI_LAN + sent_payload < + type: FILE + total_size_bytes: 50 + num_bytes_transferred: 30 + num_chunks: 3 + status: SUCCESS + > + disconnection_reason: LOCAL_DISCONNECTION + connection_token: "connection_token" + > + >)pb"))); } TEST(AnalyticsRecorderTest, UpgradeAttemptWorks) { @@ -1738,6 +1738,56 @@ TEST(AnalyticsRecorderTest, >)pb")))); } +TEST(AnalyticsRecorderOnConnectionClosedTest, + NotAddNewConnectionWithoutCallingOnStartAdvertising) { + std::string endpoint_id = "endpoint_id"; + + CountDownLatch client_session_done_latch(1); + FakeEventLogger event_logger(client_session_done_latch); + AnalyticsRecorder analytics_recorder(&event_logger); + + // via OnStartAdvertising, current_strategy_session_ is set in + // UpdateStrategySessionLocked. + analytics_recorder.OnStartAdvertising(connections::Strategy::kP2pStar, + /*mediums=*/{BLE, BLUETOOTH}); + analytics_recorder.OnStopAdvertising(); + + // LogSession + analytics_recorder.LogSession(); + ASSERT_TRUE(client_session_done_latch.Await(kDefaultTimeout).result()); + + ConnectionsLog::ClientSession strategy_session_proto = + ParseTextProtoOrDie(R"pb( + strategy_session < + strategy: P2P_STAR + role: ADVERTISER + advertising_phase < + medium: BLE + medium: BLUETOOTH + advertising_metadata < + supports_extended_ble_advertisements: false + connected_ap_frequency: 0 + supports_nfc_technology: false + > + > + >)pb"); + + EXPECT_THAT(event_logger.GetLoggedClientSession(), + Partially(EqualsProto(strategy_session_proto))); + + // Without calling OnStartAdvertising won't create new + // current_strategy_session_. + analytics_recorder.OnConnectionEstablished(endpoint_id, BLUETOOTH, + /*connection_token=*/""); + analytics_recorder.OnConnectionClosed(endpoint_id, BLUETOOTH, UPGRADED); + + analytics_recorder.LogSession(); + + // The proto won't change. + EXPECT_THAT(event_logger.GetLoggedClientSession(), + Partially(EqualsProto(strategy_session_proto))); +} + } // namespace } // namespace analytics } // namespace nearby