From e4a3da97d1c5d79ebec61a27ab465e73165a38fd Mon Sep 17 00:00:00 2001 From: edwinwu Date: Fri, 10 Sep 2021 17:41:56 -0700 Subject: [PATCH] analytics: 3p NC: Implement AnalyticsRecorder::OnErrorCode PiperOrigin-RevId: 396041619 --- cpp/analytics/BUILD | 3 + cpp/analytics/analytics_recorder.cc | 64 ++++++++++++++ cpp/analytics/analytics_recorder.h | 6 +- cpp/analytics/analytics_recorder_test.cc | 102 +++++++++++++++++++++++ cpp/core/internal/BUILD | 1 + cpp/core/internal/client_proxy.cc | 5 ++ cpp/core/internal/client_proxy.h | 2 + cpp/platform/base/error_code_recorder.h | 2 +- 8 files changed, 183 insertions(+), 2 deletions(-) diff --git a/cpp/analytics/BUILD b/cpp/analytics/BUILD index 32161d70..fefcae6b 100644 --- a/cpp/analytics/BUILD +++ b/cpp/analytics/BUILD @@ -32,10 +32,12 @@ cc_library( "//core:core_types", "//core:event_logger", "//platform/base", + "//platform/base:error_code_recorder", "//platform/public:logging", "//platform/public:types", "//proto:connections_enums_portable_proto", "//proto/analytics:connections_log_cc_proto", + "//third_party/nearby_connections/proto/errorcode:error_code_enums_portable_proto", ], ) @@ -50,6 +52,7 @@ cc_test( ":analytics", "//testing/base/public:gunit_main", "//absl/time", + "//platform/base:error_code_recorder", "//platform/impl/g3", # build_cleaner: keep "//platform/public:logging", "//platform/public:types", diff --git a/cpp/analytics/analytics_recorder.cc b/cpp/analytics/analytics_recorder.cc index 10602bac..ae562070 100644 --- a/cpp/analytics/analytics_recorder.cc +++ b/cpp/analytics/analytics_recorder.cc @@ -44,6 +44,7 @@ using ::location::nearby::proto::connections::ConnectionsStrategy; using ::location::nearby::proto::connections::DisconnectionReason; using ::location::nearby::proto::connections::DISCOVERER; using ::location::nearby::proto::connections::EventType; +using ::location::nearby::proto::connections::ERROR_CODE; using ::location::nearby::proto::connections::FILE; using ::location::nearby::proto::connections::IGNORED; using ::location::nearby::proto::connections::INCOMING; @@ -478,6 +479,69 @@ void AnalyticsRecorder::OnBandwidthUpgradeSuccess( UPGRADE_SUCCESS); } +void AnalyticsRecorder::OnErrorCode(const ErrorCodeParams& params) { + MutexLock lock(&mutex_); + if (!CanRecordAnalyticsLocked("OnErrorCode")) { + return; + } + auto error_code = absl::make_unique(); + error_code->set_medium(params.medium); + error_code->set_event(params.event); + error_code->set_connection_token(params.connection_token); + error_code->set_description(params.description); + + if (params.is_common_error) { + error_code->set_common_error(params.common_error); + } else { + switch (params.event) { + case errorcode::proto::START_ADVERTISING: + error_code->set_start_advertising_error(params.start_advertising_error); + break; + case errorcode::proto::STOP_ADVERTISING: + error_code->set_stop_advertising_error(params.stop_advertising_error); + break; + case errorcode::proto::START_LISTENING_INCOMING_CONNECTION: + error_code->set_start_listening_incoming_connection_error( + params.start_listening_incoming_connection_error); + break; + case errorcode::proto::STOP_LISTENING_INCOMING_CONNECTION: + error_code->set_stop_listening_incoming_connection_error( + params.stop_listening_incoming_connection_error); + break; + case errorcode::proto::START_DISCOVERING: + error_code->set_start_discovering_error(params.start_discovering_error); + break; + case errorcode::proto::STOP_DISCOVERING: + error_code->set_stop_discovering_error(params.stop_discovering_error); + break; + case errorcode::proto::CONNECT: + error_code->set_connect_error(params.connect_error); + break; + case errorcode::proto::DISCONNECT: + error_code->set_disconnect_error(params.disconnect_error); + break; + case errorcode::proto::UNKNOWN_EVENT: + default: + error_code->set_common_error(params.common_error); + break; + } + } + + serial_executor_.Execute( + "analytics-recorder", [this, error_code = error_code.release()]() { + ConnectionsLog connections_log; + connections_log.set_event_type(ERROR_CODE); + connections_log.set_version(kVersion); + connections_log.set_allocated_error_code(error_code); + + NEARBY_LOGS(VERBOSE) + << "AnalyticsRecorder LogErrorCode connections_log=" + << connections_log.DebugString(); + + event_logger_->Log(connections_log); + }); +} + void AnalyticsRecorder::LogSession() { MutexLock lock(&mutex_); if (!CanRecordAnalyticsLocked("LogSession")) { diff --git a/cpp/analytics/analytics_recorder.h b/cpp/analytics/analytics_recorder.h index 8f8a6a16..b2416f54 100644 --- a/cpp/analytics/analytics_recorder.h +++ b/cpp/analytics/analytics_recorder.h @@ -22,6 +22,7 @@ #include "core/event_logger.h" #include "core/payload.h" #include "core/strategy.h" +#include "platform/base/error_code_params.h" #include "platform/public/mutex.h" #include "platform/public/single_thread_executor.h" #include "proto/analytics/connections_log.proto.h" @@ -138,6 +139,9 @@ class AnalyticsRecorder { void OnBandwidthUpgradeSuccess(const std::string &endpoint_id) ABSL_LOCKS_EXCLUDED(mutex_); + // Error Code + void OnErrorCode(const ErrorCodeParams ¶ms); + // Invokes event_logger_.Log() at the end of life of client. Log action is // called in a separate thread to allow synchronous potentially lengthy // execution. @@ -303,7 +307,7 @@ class AnalyticsRecorder { // ClientSession std::unique_ptr client_session_ = - std::make_unique(); + absl::make_unique(); absl::Time started_client_session_time_; bool session_was_logged_ ABSL_GUARDED_BY(mutex_) = false; diff --git a/cpp/analytics/analytics_recorder_test.cc b/cpp/analytics/analytics_recorder_test.cc index 70969da2..5690655e 100644 --- a/cpp/analytics/analytics_recorder_test.cc +++ b/cpp/analytics/analytics_recorder_test.cc @@ -20,6 +20,8 @@ #include "gmock/gmock.h" #include "gtest/gtest.h" #include "absl/time/time.h" +#include "platform/base/error_code_params.h" +#include "platform/base/error_code_recorder.h" #include "platform/public/count_down_latch.h" #include "platform/public/logging.h" #include "proto/analytics/connections_log.proto.h" @@ -31,9 +33,18 @@ namespace analytics { namespace { using ::location::nearby::analytics::proto::ConnectionsLog; +using ::location::nearby::errorcode::proto::DISCONNECT; +using ::location::nearby::errorcode::proto::DISCONNECT_NETWORK_FAILED; +using ::location::nearby::errorcode::proto::INVALID_PARAMETER; +using ::location::nearby::errorcode::proto::NULL_BLUETOOTH_DEVICE_NAME; +using ::location::nearby::errorcode::proto::START_DISCOVERING; +using ::location::nearby::errorcode::proto::START_EXTENDED_DISCOVERING_FAILED; +using ::location::nearby::errorcode::proto:: + TACHYON_SEND_MESSAGE_STATUS_EXCEPTION; using ::location::nearby::proto::connections::BLE; using ::location::nearby::proto::connections::BLUETOOTH; using ::location::nearby::proto::connections::CLIENT_SESSION; +using ::location::nearby::proto::connections::ERROR_CODE; using ::location::nearby::proto::connections::EventType; using ::location::nearby::proto::connections::INCOMING; using ::location::nearby::proto::connections::INITIAL; @@ -46,6 +57,7 @@ using ::location::nearby::proto::connections::STOP_CLIENT_SESSION; using ::location::nearby::proto::connections::STOP_STRATEGY_SESSION; using ::location::nearby::proto::connections::SUCCESS; using ::location::nearby::proto::connections::UPGRADED; +using ::location::nearby::proto::connections::WEB_RTC; 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; @@ -68,6 +80,9 @@ class FakeEventLogger : public EventLogger { logged_client_session_count_++; logged_client_session_ = connections_log.client_session(); } + if (event_type == ERROR_CODE) { + error_code_ = connections_log.error_code(); + } if (event_type == STOP_CLIENT_SESSION) { client_session_done_latch_.CountDown(); } @@ -81,12 +96,15 @@ class FakeEventLogger : public EventLogger { return logged_client_session_; } + const ConnectionsLog::ErrorCode& GetErrorCode() { return error_code_; } + std::vector GetLoggedEventTypes() { return logged_event_types_; } private: int logged_client_session_count_ = 0; CountDownLatch& client_session_done_latch_; ConnectionsLog::ClientSession logged_client_session_; + ConnectionsLog::ErrorCode error_code_; std::vector logged_event_types_; }; @@ -662,6 +680,90 @@ TEST(AnalyticsRecorderTest, UpgradeAttemptWorks) { >)pb"))); } +TEST(AnalyticsRecorderTest, SetErrorCodeFieldsCorrectly) { + connections::Strategy strategy = connections::Strategy::kP2pStar; + std::vector mediums = {WEB_RTC}; + std::string connection_token = "connection_token"; + + CountDownLatch client_session_done_latch(1); + FakeEventLogger event_logger(client_session_done_latch); + AnalyticsRecorder analytics_recorder(&event_logger); + analytics_recorder.OnStartDiscovery(strategy, mediums); + + ErrorCodeParams error_code_params = ErrorCodeRecorder::BuildErrorCodeParams( + WEB_RTC, DISCONNECT, DISCONNECT_NETWORK_FAILED, + TACHYON_SEND_MESSAGE_STATUS_EXCEPTION, "", connection_token); + analytics_recorder.OnErrorCode(error_code_params); + + analytics_recorder.LogSession(); + ASSERT_TRUE(client_session_done_latch.Await(kDefaultTimeout).result()); + + EXPECT_THAT(event_logger.GetErrorCode(), Partially(EqualsProto(R"pb( + medium: WEB_RTC + event: DISCONNECT + description: TACHYON_SEND_MESSAGE_STATUS_EXCEPTION + disconnect_error: DISCONNECT_NETWORK_FAILED + connection_token: "connection_token" + )pb"))); +} + +TEST(AnalyticsRecorderTest, SetErrorCodeFieldsCorrectlyForUnknownDescription) { + connections::Strategy strategy = connections::Strategy::kP2pStar; + std::vector mediums = {BLUETOOTH}; + std::string connection_token = "connection_token"; + + CountDownLatch client_session_done_latch(1); + FakeEventLogger event_logger(client_session_done_latch); + AnalyticsRecorder analytics_recorder(&event_logger); + analytics_recorder.OnStartDiscovery(strategy, mediums); + + ErrorCodeParams error_code_params; + // Skip setting error_code_params.description + error_code_params.medium = BLUETOOTH; + error_code_params.event = START_DISCOVERING; + error_code_params.start_discovering_error = START_EXTENDED_DISCOVERING_FAILED; + error_code_params.connection_token = connection_token; + analytics_recorder.OnErrorCode(error_code_params); + + analytics_recorder.LogSession(); + ASSERT_TRUE(client_session_done_latch.Await(kDefaultTimeout).result()); + + EXPECT_THAT(event_logger.GetErrorCode(), Partially(EqualsProto(R"pb( + medium: BLUETOOTH + event: START_DISCOVERING + description: UNKNOWN + start_discovering_error: START_EXTENDED_DISCOVERING_FAILED + connection_token: "connection_token" + )pb"))); +} + +TEST(AnalyticsRecorderTest, SetErrorCodeFieldsCorrectlyForCommonError) { + connections::Strategy strategy = connections::Strategy::kP2pStar; + std::vector mediums = {BLUETOOTH}; + std::string connection_token = "connection_token"; + + CountDownLatch client_session_done_latch(1); + FakeEventLogger event_logger(client_session_done_latch); + AnalyticsRecorder analytics_recorder(&event_logger); + analytics_recorder.OnStartDiscovery(strategy, mediums); + + ErrorCodeParams error_code_params = ErrorCodeRecorder::BuildErrorCodeParams( + BLUETOOTH, START_DISCOVERING, INVALID_PARAMETER, + NULL_BLUETOOTH_DEVICE_NAME, "", connection_token); + analytics_recorder.OnErrorCode(error_code_params); + + analytics_recorder.LogSession(); + ASSERT_TRUE(client_session_done_latch.Await(kDefaultTimeout).result()); + + EXPECT_THAT(event_logger.GetErrorCode(), Partially(EqualsProto(R"pb( + medium: BLUETOOTH + event: START_DISCOVERING + description: NULL_BLUETOOTH_DEVICE_NAME + common_error: INVALID_PARAMETER + connection_token: "connection_token" + )pb"))); +} + } // namespace } // namespace analytics } // namespace nearby diff --git a/cpp/core/internal/BUILD b/cpp/core/internal/BUILD index c5e42de9..ccdb8f15 100644 --- a/cpp/core/internal/BUILD +++ b/cpp/core/internal/BUILD @@ -107,6 +107,7 @@ cc_library( "//platform/api:comm", "//platform/base", "//platform/base:cancellation_flag", + "//platform/base:error_code_recorder", "//platform/base:util", "//platform/public:comm", "//platform/public:logging", diff --git a/cpp/core/internal/client_proxy.cc b/cpp/core/internal/client_proxy.cc index c2fb61eb..031832cb 100644 --- a/cpp/core/internal/client_proxy.cc +++ b/cpp/core/internal/client_proxy.cc @@ -24,6 +24,7 @@ #include "absl/container/flat_hash_set.h" #include "absl/strings/escaping.h" #include "absl/strings/str_cat.h" +#include "platform/base/error_code_recorder.h" #include "platform/base/feature_flags.h" #include "platform/base/prng.h" #include "platform/public/logging.h" @@ -48,6 +49,10 @@ ClientProxy::ClientProxy(analytics::EventLogger* event_logger) NEARBY_LOGS(INFO) << "ClientProxy ctor event_logger=" << event_logger; analytics_recorder_ = std::make_unique(event_logger); + error_code_recorder_ = std::make_unique( + [this](const ErrorCodeParams& params) { + analytics_recorder_->OnErrorCode(params); + }); } ClientProxy::~ClientProxy() { Reset(); } diff --git a/cpp/core/internal/client_proxy.h b/cpp/core/internal/client_proxy.h index 539b5e35..f0ceaa33 100644 --- a/cpp/core/internal/client_proxy.h +++ b/cpp/core/internal/client_proxy.h @@ -27,6 +27,7 @@ #include "core/strategy.h" #include "platform/base/byte_array.h" #include "platform/base/cancellation_flag.h" +#include "platform/base/error_code_recorder.h" #include "platform/base/prng.h" #include "platform/public/cancelable_alarm.h" #include "platform/public/mutex.h" @@ -312,6 +313,7 @@ class ClientProxy final { // An analytics logger with |EventLogger| provided by client, which is default // nullptr as no-op. std::unique_ptr analytics_recorder_; + std::unique_ptr error_code_recorder_; }; } // namespace connections diff --git a/cpp/platform/base/error_code_recorder.h b/cpp/platform/base/error_code_recorder.h index 7cf10053..bd1c4b84 100644 --- a/cpp/platform/base/error_code_recorder.h +++ b/cpp/platform/base/error_code_recorder.h @@ -64,7 +64,6 @@ class ErrorCodeRecorder { location::nearby::errorcode::proto::Description description, const std::string& pii_message, const std::string& connection_token); - private: // An auxiliary funciton for LogError() to assemble the ErrorCodeParams // struct. // @@ -75,6 +74,7 @@ class ErrorCodeRecorder { location::nearby::errorcode::proto::Description description, const std::string& pii_message, const std::string& connection_token); + private: // A listener to call back to AnlayticsRecorder.OnErrorCode() by building // error_code_params. static ErrorCodeListener listener_;