Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Nearby Share SDK version ID to Connections logs #3086

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion connections/core.h
Original file line number Diff line number Diff line change
Expand Up @@ -52,8 +52,9 @@ class Core {
explicit Core(ServiceControllerRouter* router);
// Client needs to call this constructor if analytics logger is needed.
Core(::nearby::analytics::EventLogger* event_logger,
absl::string_view nearby_share_version_id,
ServiceControllerRouter* router)
: client_(event_logger), router_(router) {}
: client_(event_logger, nearby_share_version_id), router_(router) {}
~Core();
Core(Core&&);
Core& operator=(Core&&);
Expand Down
17 changes: 15 additions & 2 deletions connections/implementation/analytics/analytics_recorder.cc
Original file line number Diff line number Diff line change
Expand Up @@ -103,8 +103,10 @@ using ::nearby::analytics::EventLogger;
using SafeDisconnectionResult = ::location::nearby::analytics::proto::
ConnectionsLog::EstablishedConnection::SafeDisconnectionResult;

AnalyticsRecorder::AnalyticsRecorder(EventLogger *event_logger)
: event_logger_(event_logger) {
AnalyticsRecorder::AnalyticsRecorder(EventLogger *event_logger,
absl::string_view nearby_share_version_id)
: event_logger_(event_logger),
nearby_share_version_id_(nearby_share_version_id) {
NEARBY_LOGS(INFO) << "Start AnalyticsRecorder ctor event_logger_="
<< event_logger_;
LogStartSession();
Expand Down Expand Up @@ -710,6 +712,10 @@ void AnalyticsRecorder::OnErrorCode(const ErrorCodeParams &params) {
ConnectionsLog connections_log;
connections_log.set_event_type(ERROR_CODE);
connections_log.set_version(kVersion);
if (!nearby_share_version_id_.empty()) {
connections_log.set_nearby_sharing_sdk_version(
nearby_share_version_id_);
}
connections_log.set_allocated_error_code(error_code);

NEARBY_VLOG(1) << "AnalyticsRecorder LogErrorCode connections_log="
Expand Down Expand Up @@ -798,6 +804,10 @@ void AnalyticsRecorder::LogClientSessionLocked() {
connections_log.set_event_type(CLIENT_SESSION);
connections_log.set_allocated_client_session(client_session.release());
connections_log.set_version(kVersion);
if (!nearby_share_version_id_.empty()) {
connections_log.set_nearby_sharing_sdk_version(
nearby_share_version_id_);
}

NEARBY_VLOG(1) << "AnalyticsRecorder LogClientSession connections_log="
<< connections_log.DebugString(); // NOLINT
Expand All @@ -812,6 +822,9 @@ void AnalyticsRecorder::LogEvent(EventType event_type) {
ConnectionsLog connections_log;
connections_log.set_event_type(event_type);
connections_log.set_version(kVersion);
if (!nearby_share_version_id_.empty()) {
connections_log.set_nearby_sharing_sdk_version(nearby_share_version_id_);
}

NEARBY_VLOG(1) << "AnalyticsRecorder LogEvent connections_log="
<< connections_log.DebugString(); // NOLINT
Expand Down
4 changes: 3 additions & 1 deletion connections/implementation/analytics/analytics_recorder.h
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,8 @@ namespace analytics {

class AnalyticsRecorder {
public:
explicit AnalyticsRecorder(::nearby::analytics::EventLogger *event_logger);
explicit AnalyticsRecorder(::nearby::analytics::EventLogger *event_logger,
absl::string_view nearby_share_version_id);
virtual ~AnalyticsRecorder();

// TODO(edwinwu): Implement to pass real values for AdvertisingMetadata and
Expand Down Expand Up @@ -359,6 +360,7 @@ class AnalyticsRecorder {
// Not owned by AnalyticsRecorder. Pointer must refer to a valid object
// that outlives the one constructed.
::nearby::analytics::EventLogger *event_logger_;
const std::string nearby_share_version_id_;

SingleThreadExecutor serial_executor_;
// Protects all sub-protos reading and writing in ConnectionLog.
Expand Down
62 changes: 32 additions & 30 deletions connections/implementation/analytics/analytics_recorder_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,8 @@ using ::testing::Not;
using ::testing::proto::Partially;

constexpr absl::Duration kDefaultTimeout = absl::Milliseconds(1000);
constexpr absl::string_view kNearbyShareVersionId = {
"major.minor.build.revision"};

class FakeEventLogger : public MockEventLogger {
public:
Expand Down Expand Up @@ -142,7 +144,7 @@ class FakeEventLogger : public MockEventLogger {
TEST(AnalyticsRecorderTest, SessionOnlyLoggedOnceWorks) {
CountDownLatch client_session_done_latch(1);
FakeEventLogger event_logger(client_session_done_latch);
AnalyticsRecorder analytics_recorder(&event_logger);
AnalyticsRecorder analytics_recorder(&event_logger, kNearbyShareVersionId);

analytics_recorder.LogSession();
analytics_recorder.LogSession();
Expand All @@ -159,7 +161,7 @@ TEST(AnalyticsRecorderTest, SetFieldsCorrectlyForNestedAdvertisingCalls) {

CountDownLatch client_session_done_latch(1);
FakeEventLogger event_logger(client_session_done_latch);
AnalyticsRecorder analytics_recorder(&event_logger);
AnalyticsRecorder analytics_recorder(&event_logger, kNearbyShareVersionId);

analytics_recorder.OnStartAdvertising(strategy, /*mediums=*/{BLE, BLUETOOTH});
analytics_recorder.OnStopAdvertising();
Expand Down Expand Up @@ -201,7 +203,7 @@ TEST(AnalyticsRecorderTest, SetFieldsCorrectlyForNestedDiscoveryCalls) {

CountDownLatch client_session_done_latch(1);
FakeEventLogger event_logger(client_session_done_latch);
AnalyticsRecorder analytics_recorder(&event_logger);
AnalyticsRecorder analytics_recorder(&event_logger, kNearbyShareVersionId);

analytics_recorder.OnStartDiscovery(
strategy, /*mediums=*/{BLE, BLUETOOTH},
Expand Down Expand Up @@ -255,7 +257,7 @@ TEST(AnalyticsRecorderTest,

CountDownLatch client_session_done_latch(1);
FakeEventLogger event_logger(client_session_done_latch);
AnalyticsRecorder analytics_recorder(&event_logger);
AnalyticsRecorder analytics_recorder(&event_logger, kNearbyShareVersionId);

analytics_recorder.OnStartAdvertising(strategy, mediums);
analytics_recorder.OnStartDiscovery(strategy, mediums);
Expand Down Expand Up @@ -351,7 +353,7 @@ TEST(AnalyticsRecorderTest, AdvertiserConnectionRequestsWorks) {

CountDownLatch client_session_done_latch(1);
FakeEventLogger event_logger(client_session_done_latch);
AnalyticsRecorder analytics_recorder(&event_logger);
AnalyticsRecorder analytics_recorder(&event_logger, kNearbyShareVersionId);

analytics_recorder.OnStartAdvertising(connections::Strategy::kP2pStar,
/*mediums=*/{BLE, BLUETOOTH});
Expand Down Expand Up @@ -418,7 +420,7 @@ TEST(AnalyticsRecorderTest, DiscoveryConnectionRequestsWorks) {

CountDownLatch client_session_done_latch(1);
FakeEventLogger event_logger(client_session_done_latch);
AnalyticsRecorder analytics_recorder(&event_logger);
AnalyticsRecorder analytics_recorder(&event_logger, kNearbyShareVersionId);

analytics_recorder.OnStartDiscovery(connections::Strategy::kP2pStar,
/*mediums=*/{BLE, BLUETOOTH});
Expand Down Expand Up @@ -486,7 +488,7 @@ TEST(AnalyticsRecorderTest,

CountDownLatch client_session_done_latch(1);
FakeEventLogger event_logger(client_session_done_latch);
AnalyticsRecorder analytics_recorder(&event_logger);
AnalyticsRecorder analytics_recorder(&event_logger, kNearbyShareVersionId);

analytics_recorder.OnStartAdvertising(connections::Strategy::kP2pStar,
/*mediums=*/{BLE, BLUETOOTH});
Expand Down Expand Up @@ -544,7 +546,7 @@ TEST(AnalyticsRecorderTest,

CountDownLatch client_session_done_latch(1);
FakeEventLogger event_logger(client_session_done_latch);
AnalyticsRecorder analytics_recorder(&event_logger);
AnalyticsRecorder analytics_recorder(&event_logger, kNearbyShareVersionId);

analytics_recorder.OnStartDiscovery(connections::Strategy::kP2pStar,
/*mediums=*/{BLE, BLUETOOTH});
Expand Down Expand Up @@ -598,7 +600,7 @@ TEST(AnalyticsRecorderTest,
TEST(AnalyticsRecorderTest, SuccessfulIncomingConnectionAttempt) {
CountDownLatch client_session_done_latch(1);
FakeEventLogger event_logger(client_session_done_latch);
AnalyticsRecorder analytics_recorder(&event_logger);
AnalyticsRecorder analytics_recorder(&event_logger, kNearbyShareVersionId);

analytics_recorder.OnStartAdvertising(connections::Strategy::kP2pStar,
/*mediums=*/{BLE, BLUETOOTH});
Expand Down Expand Up @@ -656,7 +658,7 @@ TEST(AnalyticsRecorderTest,

CountDownLatch client_session_done_latch(1);
FakeEventLogger event_logger(client_session_done_latch);
AnalyticsRecorder analytics_recorder(&event_logger);
AnalyticsRecorder analytics_recorder(&event_logger, kNearbyShareVersionId);

auto connections_attempt_metadata_params =
analytics_recorder.BuildConnectionAttemptMetadataParams(
Expand Down Expand Up @@ -728,7 +730,7 @@ TEST(AnalyticsRecorderTest, UnfinishedEstablishedConnectionsAddedAsUnfinished) {

CountDownLatch client_session_done_latch(1);
FakeEventLogger event_logger(client_session_done_latch);
AnalyticsRecorder analytics_recorder(&event_logger);
AnalyticsRecorder analytics_recorder(&event_logger, kNearbyShareVersionId);

analytics_recorder.OnStartAdvertising(connections::Strategy::kP2pStar,
/*mediums=*/{BLE, BLUETOOTH});
Expand Down Expand Up @@ -780,7 +782,7 @@ TEST(AnalyticsRecorderTest, OutgoingPayloadUpgraded) {

CountDownLatch client_session_done_latch(1);
FakeEventLogger event_logger(client_session_done_latch);
AnalyticsRecorder analytics_recorder(&event_logger);
AnalyticsRecorder analytics_recorder(&event_logger, kNearbyShareVersionId);

analytics_recorder.OnStartAdvertising(connections::Strategy::kP2pStar,
/*mediums=*/{BLE, BLUETOOTH});
Expand Down Expand Up @@ -859,7 +861,7 @@ TEST(AnalyticsRecorderTest, UpgradeAttemptWorks) {

CountDownLatch client_session_done_latch(1);
FakeEventLogger event_logger(client_session_done_latch);
AnalyticsRecorder analytics_recorder(&event_logger);
AnalyticsRecorder analytics_recorder(&event_logger, kNearbyShareVersionId);

analytics_recorder.OnStartAdvertising(connections::Strategy::kP2pStar,
/*mediums=*/{BLE, BLUETOOTH});
Expand Down Expand Up @@ -933,7 +935,7 @@ TEST(AnalyticsRecorderTest, StartListeningForIncomingConnectionsWorks) {

CountDownLatch client_session_done_latch(1);
FakeEventLogger event_logger(client_session_done_latch);
AnalyticsRecorder analytics_recorder(&event_logger);
AnalyticsRecorder analytics_recorder(&event_logger, kNearbyShareVersionId);

analytics_recorder.OnStartedIncomingConnectionListening(
connections::Strategy::kP2pStar);
Expand Down Expand Up @@ -983,7 +985,7 @@ TEST(AnalyticsRecorderTest, StartListeningForIncomingConnectionsWorks) {
TEST(AnalyticsRecorderTest, SetErrorCodeFieldsCorrectly) {
CountDownLatch client_session_done_latch(1);
FakeEventLogger event_logger(client_session_done_latch);
AnalyticsRecorder analytics_recorder(&event_logger);
AnalyticsRecorder analytics_recorder(&event_logger, kNearbyShareVersionId);
analytics_recorder.OnStartDiscovery(connections::Strategy::kP2pStar,
/*mediums=*/{WEB_RTC});

Expand All @@ -1010,7 +1012,7 @@ TEST(AnalyticsRecorderTest, SetErrorCodeFieldsCorrectly) {
TEST(AnalyticsRecorderTest, SetErrorCodeFieldsCorrectlyForUnknownDescription) {
CountDownLatch client_session_done_latch(1);
FakeEventLogger event_logger(client_session_done_latch);
AnalyticsRecorder analytics_recorder(&event_logger);
AnalyticsRecorder analytics_recorder(&event_logger, kNearbyShareVersionId);
analytics_recorder.OnStartDiscovery(connections::Strategy::kP2pStar,
/*mediums=*/{BLUETOOTH});

Expand Down Expand Up @@ -1040,7 +1042,7 @@ TEST(AnalyticsRecorderTest, SetErrorCodeFieldsCorrectlyForUnknownDescription) {
TEST(AnalyticsRecorderTest, SetErrorCodeFieldsCorrectlyForCommonError) {
CountDownLatch client_session_done_latch(1);
FakeEventLogger event_logger(client_session_done_latch);
AnalyticsRecorder analytics_recorder(&event_logger);
AnalyticsRecorder analytics_recorder(&event_logger, kNearbyShareVersionId);
analytics_recorder.OnStartDiscovery(connections::Strategy::kP2pStar,
/*mediums=*/{BLUETOOTH});

Expand All @@ -1067,7 +1069,7 @@ TEST(AnalyticsRecorderTest, SetErrorCodeFieldsCorrectlyForCommonError) {
TEST(AnalyticsRecorderTest, CheckIfSessionWasLogged) {
CountDownLatch client_session_done_latch(1);
FakeEventLogger event_logger(client_session_done_latch);
AnalyticsRecorder analytics_recorder(&event_logger);
AnalyticsRecorder analytics_recorder(&event_logger, kNearbyShareVersionId);

// LogSession to count down client_session_done_latch.
analytics_recorder.LogSession();
Expand All @@ -1083,7 +1085,7 @@ TEST(AnalyticsRecorderTest, ConstructAnalyticsRecorder) {
&start_client_session_done_latch);

// Call the constructor to count down the session_done_latch.
AnalyticsRecorder analytics_recorder(&event_logger);
AnalyticsRecorder analytics_recorder(&event_logger, kNearbyShareVersionId);
ASSERT_TRUE(start_client_session_done_latch.Await(kDefaultTimeout).result());

std::vector<EventType> event_types = event_logger.GetLoggedEventTypes();
Expand All @@ -1099,7 +1101,7 @@ TEST(AnalyticsRecorderTest,
&start_client_session_done_latch);

// Call the constructor to count down the start_client_session_done_latch.
AnalyticsRecorder analytics_recorder(&event_logger);
AnalyticsRecorder analytics_recorder(&event_logger, kNearbyShareVersionId);
ASSERT_TRUE(start_client_session_done_latch.Await(kDefaultTimeout).result());

// Log start client session once.
Expand Down Expand Up @@ -1128,7 +1130,7 @@ TEST(AnalyticsRecorderTest,
&start_client_session_done_latch);

// Call the constructor to count down the start_client_session_done_latch.
AnalyticsRecorder analytics_recorder(&event_logger);
AnalyticsRecorder analytics_recorder(&event_logger, kNearbyShareVersionId);
ASSERT_TRUE(start_client_session_done_latch.Await(kDefaultTimeout).result());

// Log start client session once.
Expand Down Expand Up @@ -1165,7 +1167,7 @@ TEST(AnalyticsRecorderTest,

CountDownLatch client_session_done_latch(1);
FakeEventLogger event_logger(client_session_done_latch);
AnalyticsRecorder analytics_recorder(&event_logger);
AnalyticsRecorder analytics_recorder(&event_logger, kNearbyShareVersionId);

analytics_recorder.OnStartAdvertising(connections::Strategy::kP2pStar,
/*mediums=*/{BLE, BLUETOOTH});
Expand Down Expand Up @@ -1268,7 +1270,7 @@ TEST(AnalyticsRecorderTest,

CountDownLatch client_session_done_latch(1);
FakeEventLogger event_logger(client_session_done_latch);
AnalyticsRecorder analytics_recorder(&event_logger);
AnalyticsRecorder analytics_recorder(&event_logger, kNearbyShareVersionId);

analytics_recorder.OnStartDiscovery(connections::Strategy::kP2pStar,
/*mediums=*/{BLE, BLUETOOTH});
Expand Down Expand Up @@ -1374,7 +1376,7 @@ TEST(AnalyticsRecorderTest, ClearcActiveConnectionsAfterSessionWasLogged) {

CountDownLatch client_session_done_latch(1);
FakeEventLogger event_logger(client_session_done_latch);
AnalyticsRecorder analytics_recorder(&event_logger);
AnalyticsRecorder analytics_recorder(&event_logger, kNearbyShareVersionId);

analytics_recorder.OnStartAdvertising(strategy, mediums);

Expand Down Expand Up @@ -1469,7 +1471,7 @@ TEST(AnalyticsRecorderTest,

CountDownLatch client_session_done_latch(1);
FakeEventLogger event_logger(client_session_done_latch);
AnalyticsRecorder analytics_recorder(&event_logger);
AnalyticsRecorder analytics_recorder(&event_logger, kNearbyShareVersionId);

analytics_recorder.OnStartAdvertising(connections::Strategy::kP2pStar,
/*mediums=*/{BLE, BLUETOOTH});
Expand Down Expand Up @@ -1613,7 +1615,7 @@ TEST(AnalyticsRecorderTest,

CountDownLatch client_session_done_latch(1);
FakeEventLogger event_logger(client_session_done_latch);
AnalyticsRecorder analytics_recorder(&event_logger);
AnalyticsRecorder analytics_recorder(&event_logger, kNearbyShareVersionId);

analytics_recorder.OnStartAdvertising(connections::Strategy::kP2pStar,
/*mediums=*/{BLUETOOTH});
Expand Down Expand Up @@ -1655,7 +1657,7 @@ TEST(AnalyticsRecorderTest,
NotLogSameStrategySessionProtoAfterSessionWasLogged) {
CountDownLatch client_session_done_latch(1);
FakeEventLogger event_logger(client_session_done_latch);
AnalyticsRecorder analytics_recorder(&event_logger);
AnalyticsRecorder analytics_recorder(&event_logger, kNearbyShareVersionId);

//// Via OnStartAdvertising, current_strategy_session_is set in
//// UpdateStrategySessionLocked.
Expand Down Expand Up @@ -1711,7 +1713,7 @@ TEST(AnalyticsRecorderTest,
NotLogDuplicateAdvertisingPhaseAfterSessionWasLogged) {
CountDownLatch client_session_done_latch(1);
FakeEventLogger event_logger(client_session_done_latch);
AnalyticsRecorder analytics_recorder(&event_logger);
AnalyticsRecorder analytics_recorder(&event_logger, kNearbyShareVersionId);

analytics_recorder.OnStartAdvertising(
connections::Strategy::kP2pStar,
Expand Down Expand Up @@ -1790,7 +1792,7 @@ TEST(AnalyticsRecorderTest,

CountDownLatch client_session_done_latch(1);
FakeEventLogger event_logger(client_session_done_latch);
AnalyticsRecorder analytics_recorder(&event_logger);
AnalyticsRecorder analytics_recorder(&event_logger, kNearbyShareVersionId);

analytics_recorder.OnStartDiscovery(
strategy, {BLUETOOTH}, /*is_extended_advertisement_supported=*/true,
Expand Down Expand Up @@ -1870,7 +1872,7 @@ TEST(AnalyticsRecorderOnConnectionClosedTest,

CountDownLatch client_session_done_latch(1);
FakeEventLogger event_logger(client_session_done_latch);
AnalyticsRecorder analytics_recorder(&event_logger);
AnalyticsRecorder analytics_recorder(&event_logger, kNearbyShareVersionId);

// via OnStartAdvertising, current_strategy_session_ is set in
// UpdateStrategySessionLocked.
Expand Down
7 changes: 4 additions & 3 deletions connections/implementation/client_proxy.cc
Original file line number Diff line number Diff line change
Expand Up @@ -86,11 +86,12 @@ bool IsFeatureUseStableEndpointIdEnabled() {
constexpr absl::Duration
ClientProxy::kHighPowerAdvertisementEndpointIdCacheTimeout;

ClientProxy::ClientProxy(::nearby::analytics::EventLogger* event_logger)
ClientProxy::ClientProxy(::nearby::analytics::EventLogger* event_logger,
absl::string_view nearby_share_version_id)
: client_id_(Prng().NextInt64()) {
NEARBY_LOGS(INFO) << "ClientProxy ctor event_logger=" << event_logger;
analytics_recorder_ =
std::make_unique<analytics::AnalyticsRecorder>(event_logger);
analytics_recorder_ = std::make_unique<analytics::AnalyticsRecorder>(
event_logger, nearby_share_version_id);
error_code_recorder_ = std::make_unique<ErrorCodeRecorder>(
[this](const ErrorCodeParams& params) {
analytics_recorder_->OnErrorCode(params);
Expand Down
4 changes: 2 additions & 2 deletions connections/implementation/client_proxy.h
Original file line number Diff line number Diff line change
Expand Up @@ -65,8 +65,8 @@ class ClientProxy final {
static constexpr absl::Duration
kHighPowerAdvertisementEndpointIdCacheTimeout = absl::Seconds(30);

explicit ClientProxy(
::nearby::analytics::EventLogger* event_logger = nullptr);
explicit ClientProxy(::nearby::analytics::EventLogger* event_logger = nullptr,
absl::string_view nearby_share_version_id = {});
~ClientProxy();
ClientProxy(ClientProxy&&) = default;
ClientProxy& operator=(ClientProxy&&) = default;
Expand Down
6 changes: 5 additions & 1 deletion internal/proto/analytics/connections_log.proto
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ option objc_class_prefix = "GNCP";

// Top-level log proto for Nearby Connections.
// LINT.IfChange(ConnectionsLog)
// Next Tag: 7
// Next Tag: 8
message ConnectionsLog {
// The type of this log.
optional location.nearby.proto.connections.EventType event_type = 1;
Expand Down Expand Up @@ -55,6 +55,10 @@ message ConnectionsLog {
// Reference: http://shortn/_9smZZ8CTD6, http://shortn/_Y08gVwZRKc
optional string files_migration_phase = 6;

// The version of Nearby Sharing SDK. It is passed from NearbySharing SDK.
optional string nearby_sharing_sdk_version = 7
/* type = ST_SOFTWARE_ID */;

// Encapsulates one session of a client connected to Nearby Connections API.
message ClientSession {
// Elapsed time in milliseconds between Client connect and
Expand Down
Loading
Loading