Skip to content

Commit

Permalink
[QuickStart] Turn QuickStartMetrics into a class
Browse files Browse the repository at this point in the history
Let's convert the quick_start_metrics namespace into a QuickStartMetrics
class. This will allow us to add state to these logging functions and
remove all of the timers in the calling code.

Test: Trivial change well-covered by existing tests

Bug: b/307817810
Change-Id: Idf3380a678c47836024bb442fed0b49272bab339
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4976188
Reviewed-by: Michael Hansen <hansenmichael@google.com>
Reviewed-by: Renato Silva <rrsilva@google.com>
Commit-Queue: Curt Clemens <cclem@google.com>
Cr-Commit-Position: refs/heads/main@{#1216392}
  • Loading branch information
Curt Clemens authored and Chromium LUCI CQ committed Oct 27, 2023
1 parent dc70f60 commit 2be3601
Show file tree
Hide file tree
Showing 17 changed files with 442 additions and 410 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -234,7 +234,7 @@ void Connection::OnRequestAccountTransferAssertionResponse(
assertion_info.authenticator_data = fido_response->auth_data;
assertion_info.signature = fido_response->signature;

quick_start_metrics::RecordGaiaTransferResult(
QuickStartMetrics::RecordGaiaTransferResult(
/*succeeded=*/true, /*failure_reason=*/absl::nullopt);

std::move(callback).Run(assertion_info);
Expand Down Expand Up @@ -293,8 +293,8 @@ void Connection::SendBytesAndReadResponse(std::vector<uint8_t>&& bytes,
QuickStartResponseType response_type,
ConnectionResponseCallback callback,
base::TimeDelta timeout) {
quick_start_metrics::RecordMessageSent(
quick_start_metrics::MapResponseToMessageType(response_type));
QuickStartMetrics::RecordMessageSent(
QuickStartMetrics::MapResponseToMessageType(response_type));
nearby_connection_->Write(std::move(bytes));
nearby_connection_->Read(base::BindOnce(
&Connection::OnResponseReceived, response_weak_ptr_factory_.GetWeakPtr(),
Expand Down Expand Up @@ -325,10 +325,10 @@ void Connection::OnHandshakeResponse(
absl::optional<std::vector<uint8_t>> response_bytes) {
if (!response_bytes) {
QS_LOG(ERROR) << "Failed to read handshake response from NearbyConnection";
quick_start_metrics::RecordHandshakeResult(
QuickStartMetrics::RecordHandshakeResult(
/*success=*/false, /*duration=*/handshake_elapsed_timer_->Elapsed(),
/*error_code=*/
quick_start_metrics::HandshakeErrorCode::kFailedToReadResponse);
QuickStartMetrics::HandshakeErrorCode::kFailedToReadResponse);
handshake_elapsed_timer_.reset();
std::move(callback).Run(/*success=*/false);
return;
Expand All @@ -338,11 +338,11 @@ void Connection::OnHandshakeResponse(
session_context_.shared_secret());
bool success = status == handshake::VerifyHandshakeMessageStatus::kSuccess;
if (success) {
quick_start_metrics::RecordHandshakeResult(
QuickStartMetrics::RecordHandshakeResult(
/*success=*/true, /*duration=*/handshake_elapsed_timer_->Elapsed(),
/*error_code=*/absl::nullopt);
} else {
quick_start_metrics::RecordHandshakeResult(
QuickStartMetrics::RecordHandshakeResult(
/*success=*/false, /*duration=*/handshake_elapsed_timer_->Elapsed(),
/*error_code=*/
handshake::MapHandshakeStatusToErrorCode(status));
Expand Down Expand Up @@ -469,12 +469,12 @@ void Connection::OnResponseTimeout(QuickStartResponseType response_type) {
QS_LOG(ERROR) << "Timed out waiting for " << response_type
<< " response from source device.";
Close(TargetDeviceConnectionBroker::ConnectionClosedReason::kResponseTimeout);
quick_start_metrics::RecordMessageReceived(
/*desired_message_type=*/quick_start_metrics::MapResponseToMessageType(
QuickStartMetrics::RecordMessageReceived(
/*desired_message_type=*/QuickStartMetrics::MapResponseToMessageType(
response_type),
/*succeeded=*/false,
/*listen_duration=*/kDefaultRoundTripTimeout,
quick_start_metrics::MessageReceivedErrorCode::kTimeOut);
QuickStartMetrics::MessageReceivedErrorCode::kTimeOut);
message_elapsed_timer_.reset();
if (response_type == QuickStartResponseType::kHandshake &&
handshake_elapsed_timer_) {
Expand All @@ -493,15 +493,15 @@ void Connection::OnResponseReceived(
<< " response from source device";

if (!response_bytes.has_value()) {
quick_start_metrics::RecordMessageReceived(
/*desired_message_type=*/quick_start_metrics::MapResponseToMessageType(
QuickStartMetrics::RecordMessageReceived(
/*desired_message_type=*/QuickStartMetrics::MapResponseToMessageType(
response_type),
/*succeeded=*/false,
/*listen_duration=*/message_elapsed_timer_->Elapsed(),
quick_start_metrics::MessageReceivedErrorCode::kDeserializationFailure);
QuickStartMetrics::MessageReceivedErrorCode::kDeserializationFailure);
} else {
quick_start_metrics::RecordMessageReceived(
/*desired_message_type=*/quick_start_metrics::MapResponseToMessageType(
QuickStartMetrics::RecordMessageReceived(
/*desired_message_type=*/QuickStartMetrics::MapResponseToMessageType(
response_type),
/*succeeded=*/true,
/*listen_duration=*/message_elapsed_timer_->Elapsed(), absl::nullopt);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -176,9 +176,9 @@ class ConnectionTest : public testing::Test {
absl::nullopt);
TestMessageMetrics(
/*succeeded=*/false, /*message_type=*/
quick_start_metrics::MapResponseToMessageType(response_type),
QuickStartMetrics::MapResponseToMessageType(response_type),
/*error_code=*/
quick_start_metrics::MessageReceivedErrorCode::kDeserializationFailure);
QuickStartMetrics::MessageReceivedErrorCode::kDeserializationFailure);
}

void OnHandshakeResponse(base::OnceCallback<void(bool)> callback) {
Expand All @@ -195,15 +195,14 @@ class ConnectionTest : public testing::Test {

void TestMessageMetrics(
bool should_succeed,
quick_start_metrics::MessageType message_type,
absl::optional<quick_start_metrics::MessageReceivedErrorCode>
error_code) {
QuickStartMetrics::MessageType message_type,
absl::optional<QuickStartMetrics::MessageReceivedErrorCode> error_code) {
histogram_tester_.ExpectBucketCount("QuickStart.MessageSent.MessageType",
message_type, 1);
histogram_tester_.ExpectBucketCount(
"QuickStart.MessageReceived.DesiredMessageType", message_type, 1);
switch (message_type) {
case quick_start_metrics::MessageType::kWifiCredentials:
case QuickStartMetrics::MessageType::kWifiCredentials:
histogram_tester_.ExpectBucketCount(
"QuickStart.MessageReceived.WifiCredentials.Succeeded",
should_succeed, 1);
Expand All @@ -215,7 +214,7 @@ class ConnectionTest : public testing::Test {
error_code.value(), 1);
}
break;
case quick_start_metrics::MessageType::kBootstrapConfigurations:
case QuickStartMetrics::MessageType::kBootstrapConfigurations:
histogram_tester_.ExpectBucketCount(
"QuickStart.MessageReceived.BootstrapConfigurations.Succeeded",
should_succeed, 1);
Expand All @@ -228,7 +227,7 @@ class ConnectionTest : public testing::Test {
error_code.value(), 1);
}
break;
case quick_start_metrics::MessageType::kHandshake:
case QuickStartMetrics::MessageType::kHandshake:
histogram_tester_.ExpectBucketCount(
"QuickStart.MessageReceived.Handshake.Succeeded", should_succeed,
1);
Expand All @@ -240,7 +239,7 @@ class ConnectionTest : public testing::Test {
error_code.value(), 1);
}
break;
case quick_start_metrics::MessageType::kNotifySourceOfUpdate:
case QuickStartMetrics::MessageType::kNotifySourceOfUpdate:
histogram_tester_.ExpectBucketCount(
"QuickStart.MessageReceived.NotifySourceOfUpdate.Succeeded",
should_succeed, 1);
Expand All @@ -253,7 +252,7 @@ class ConnectionTest : public testing::Test {
error_code.value(), 1);
}
break;
case quick_start_metrics::MessageType::kGetInfo:
case QuickStartMetrics::MessageType::kGetInfo:
histogram_tester_.ExpectBucketCount(
"QuickStart.MessageReceived.GetInfo.Succeeded", should_succeed, 1);
histogram_tester_.ExpectTotalCount(
Expand All @@ -264,7 +263,7 @@ class ConnectionTest : public testing::Test {
error_code.value(), 1);
}
break;
case quick_start_metrics::MessageType::kAssertion:
case QuickStartMetrics::MessageType::kAssertion:
histogram_tester_.ExpectBucketCount(
"QuickStart.MessageReceived.Assertion.Succeeded", should_succeed,
1);
Expand All @@ -281,7 +280,7 @@ class ConnectionTest : public testing::Test {

void TestHandshakeMetrics(
bool should_succeed,
absl::optional<quick_start_metrics::HandshakeErrorCode> error_code) {
absl::optional<QuickStartMetrics::HandshakeErrorCode> error_code) {
if (!should_succeed) {
histogram_tester_.ExpectBucketCount(
"QuickStart.HandshakeResult.ErrorCode", error_code.value(), 1);
Expand Down Expand Up @@ -368,7 +367,7 @@ TEST_F(ConnectionTest, RequestWifiCredentials) {
EXPECT_TRUE(credentials.value().is_hidden);
TestMessageMetrics(
/*should_succeed=*/true,
/*message_type=*/quick_start_metrics::MessageType::kWifiCredentials,
/*message_type=*/QuickStartMetrics::MessageType::kWifiCredentials,
/*error_code=*/absl::nullopt);
}

Expand Down Expand Up @@ -416,7 +415,7 @@ TEST_F(ConnectionTest, RequestAccountInfo) {
ASSERT_TRUE(future.Wait());

TestMessageMetrics(/*should_succeed=*/true, /*message_type=*/
quick_start_metrics::MessageType::kBootstrapConfigurations,
QuickStartMetrics::MessageType::kBootstrapConfigurations,
/*error_code=*/absl::nullopt);
}

Expand Down Expand Up @@ -450,7 +449,7 @@ TEST_F(ConnectionTest, RequestAccountTransferAssertion) {
fake_nearby_connection_->AppendReadableData(kTestBytes);
TestMessageMetrics(
/*should_succeed=*/true,
/*message_type=*/quick_start_metrics::MessageType::kGetInfo,
/*message_type=*/QuickStartMetrics::MessageType::kGetInfo,
/*error_code=*/absl::nullopt);

// OnFidoGetInfoResponse should trigger a write of FIDO GetAssertion
Expand Down Expand Up @@ -499,7 +498,7 @@ TEST_F(ConnectionTest, RequestAccountTransferAssertion) {
EXPECT_FALSE(fake_nearby_connection_->IsClosed());
TestMessageMetrics(
/*should_succeed=*/true,
/*message_type=*/quick_start_metrics::MessageType::kAssertion,
/*message_type=*/QuickStartMetrics::MessageType::kAssertion,
/*error_code=*/absl::nullopt);

// Wait for callback to finish and verify response
Expand Down Expand Up @@ -582,7 +581,7 @@ TEST_F(ConnectionTest, NotifySourceOfUpdate_Success) {
EXPECT_TRUE(future.Get());
TestMessageMetrics(
/*should_succeed=*/true,
/*message_type=*/quick_start_metrics::MessageType::kNotifySourceOfUpdate,
/*message_type=*/QuickStartMetrics::MessageType::kNotifySourceOfUpdate,
/*error_code=*/absl::nullopt);
}

Expand All @@ -598,7 +597,7 @@ TEST_F(ConnectionTest, NotifySourceOfUpdate_FalseAckReceivedValue) {
EXPECT_FALSE(future.Get());
TestMessageMetrics(
/*should_succeed=*/true,
/*message_type=*/quick_start_metrics::MessageType::kNotifySourceOfUpdate,
/*message_type=*/QuickStartMetrics::MessageType::kNotifySourceOfUpdate,
/*error_code=*/absl::nullopt);
}

Expand Down Expand Up @@ -638,8 +637,8 @@ TEST_F(ConnectionTest, NotifySourceOfUpdate_ResponseTimeout) {
EXPECT_EQ(connection_->GetState(), Connection::State::kClosed);
TestMessageMetrics(
/*should_succeed=*/false,
/*message_type=*/quick_start_metrics::MessageType::kNotifySourceOfUpdate,
/*error_code=*/quick_start_metrics::MessageReceivedErrorCode::kTimeOut);
/*message_type=*/QuickStartMetrics::MessageType::kNotifySourceOfUpdate,
/*error_code=*/QuickStartMetrics::MessageReceivedErrorCode::kTimeOut);
}

TEST_F(ConnectionTest, SendBytesAndReadResponse_TimedOut) {
Expand Down Expand Up @@ -736,7 +735,7 @@ TEST_F(ConnectionTest, InitiateHandshake) {
EXPECT_TRUE(future.Get());
TestMessageMetrics(
/*should_succeed=*/true,
/*message_type=*/quick_start_metrics::MessageType::kHandshake,
/*message_type=*/QuickStartMetrics::MessageType::kHandshake,
/*error_code=*/absl::nullopt);
TestHandshakeMetrics(/*should_succeed=*/true, /*error_code=*/absl::nullopt);
}
Expand All @@ -752,7 +751,7 @@ TEST_F(ConnectionTest, InitiateHandshake_BadResponse) {
fake_nearby_connection_->AppendReadableData(written_payload);
EXPECT_FALSE(future.Get());
TestHandshakeMetrics(/*should_succeed=*/false,
/*error_code=*/quick_start_metrics::HandshakeErrorCode::
/*error_code=*/QuickStartMetrics::HandshakeErrorCode::
kUnexpectedAuthPayloadRole);
}

Expand All @@ -761,7 +760,7 @@ TEST_F(ConnectionTest, EmptyHandshakeResponse) {
connection_->InitiateHandshake(kAuthToken, future.GetCallback());
OnHandshakeResponse(future.GetCallback());
TestHandshakeMetrics(/*should_succeed=*/false,
/*error_code=*/quick_start_metrics::HandshakeErrorCode::
/*error_code=*/QuickStartMetrics::HandshakeErrorCode::
kFailedToReadResponse);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,44 +22,43 @@ constexpr uint8_t kFastPairModelIdChromebase[] = {0xe9, 0x31, 0x6c};
constexpr uint8_t kFastPairModelIdChromebox[] = {0xda, 0xde, 0x43};
constexpr uint16_t kCompanyId = 0x00e0;

quick_start_metrics::FastPairAdvertisingErrorCode
QuickStartMetrics::FastPairAdvertisingErrorCode
MapBluetoothAdvertisementErrorCode(
device::BluetoothAdvertisement::ErrorCode error_code) {
switch (error_code) {
case device::BluetoothAdvertisement::ErrorCode::ERROR_UNSUPPORTED_PLATFORM:
return quick_start_metrics::FastPairAdvertisingErrorCode::
return QuickStartMetrics::FastPairAdvertisingErrorCode::
kUnsupportedPlatform;
case device::BluetoothAdvertisement::ErrorCode::
ERROR_ADVERTISEMENT_ALREADY_EXISTS:
return quick_start_metrics::FastPairAdvertisingErrorCode::
return QuickStartMetrics::FastPairAdvertisingErrorCode::
kAdvertisementAlreadyExists;
case device::BluetoothAdvertisement::ErrorCode::
ERROR_ADVERTISEMENT_DOES_NOT_EXIST:
return quick_start_metrics::FastPairAdvertisingErrorCode::
return QuickStartMetrics::FastPairAdvertisingErrorCode::
kAdvertisementDoesNotExist;
case device::BluetoothAdvertisement::ErrorCode::
ERROR_ADVERTISEMENT_INVALID_LENGTH:
return quick_start_metrics::FastPairAdvertisingErrorCode::
return QuickStartMetrics::FastPairAdvertisingErrorCode::
kAdvertisementInvalidLength;
case device::BluetoothAdvertisement::ErrorCode::
ERROR_STARTING_ADVERTISEMENT:
return quick_start_metrics::FastPairAdvertisingErrorCode::
return QuickStartMetrics::FastPairAdvertisingErrorCode::
kStartingAdvertisement;
case device::BluetoothAdvertisement::ErrorCode::ERROR_RESET_ADVERTISING:
return quick_start_metrics::FastPairAdvertisingErrorCode::
kResetAdvertising;
return QuickStartMetrics::FastPairAdvertisingErrorCode::kResetAdvertising;
case device::BluetoothAdvertisement::ErrorCode::ERROR_ADAPTER_POWERED_OFF:
return quick_start_metrics::FastPairAdvertisingErrorCode::
return QuickStartMetrics::FastPairAdvertisingErrorCode::
kAdapterPoweredOff;
case device::BluetoothAdvertisement::ErrorCode::
ERROR_INVALID_ADVERTISEMENT_INTERVAL:
return quick_start_metrics::FastPairAdvertisingErrorCode::
return QuickStartMetrics::FastPairAdvertisingErrorCode::
kInvalidAdvertisementInterval;
case device::BluetoothAdvertisement::ErrorCode::
INVALID_ADVERTISEMENT_ERROR_CODE:
[[fallthrough]];
default:
return quick_start_metrics::FastPairAdvertisingErrorCode::
return QuickStartMetrics::FastPairAdvertisingErrorCode::
kInvalidAdvertisementErrorCode;
}
}
Expand Down Expand Up @@ -127,9 +126,9 @@ void FastPairAdvertiser::StartAdvertising(
DCHECK(!advertisement_);
advertising_timer_ = std::make_unique<base::ElapsedTimer>();
advertising_method_ = use_pin_authentication
? quick_start_metrics::AdvertisingMethod::kPin
: quick_start_metrics::AdvertisingMethod::kQrCode;
quick_start_metrics::RecordFastPairAdvertisementStarted(advertising_method_);
? QuickStartMetrics::AdvertisingMethod::kPin
: QuickStartMetrics::AdvertisingMethod::kQrCode;
QuickStartMetrics::RecordFastPairAdvertisementStarted(advertising_method_);
RegisterAdvertisement(std::move(callback), std::move(error_callback),
advertising_id);
}
Expand Down Expand Up @@ -189,9 +188,9 @@ void FastPairAdvertiser::OnRegisterAdvertisementError(
base::OnceClosure error_callback,
device::BluetoothAdvertisement::ErrorCode error_code) {
LOG(ERROR) << __func__ << " failed with error code = " << error_code;
quick_start_metrics::FastPairAdvertisingErrorCode uma_error_code_enum =
QuickStartMetrics::FastPairAdvertisingErrorCode uma_error_code_enum =
MapBluetoothAdvertisementErrorCode(error_code);
quick_start_metrics::RecordFastPairAdvertisementEnded(
QuickStartMetrics::RecordFastPairAdvertisementEnded(
/*advertising_method*/ advertising_method_, /*succeeded=*/false,
/*duration=*/advertising_timer_->Elapsed(),
/*error_code=*/uma_error_code_enum);
Expand All @@ -212,7 +211,7 @@ void FastPairAdvertiser::UnregisterAdvertisement(base::OnceClosure callback) {

void FastPairAdvertiser::OnUnregisterAdvertisement() {
advertisement_.reset();
quick_start_metrics::RecordFastPairAdvertisementEnded(
QuickStartMetrics::RecordFastPairAdvertisementEnded(
/*advertising_method*/ advertising_method_, /*succeeded=*/true,
/*duration=*/advertising_timer_->Elapsed(),
/*error_code=*/absl::nullopt);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,7 @@ class FastPairAdvertiser : public device::BluetoothAdvertisement::Observer {
// Timer to keep track of advertising duration.
std::unique_ptr<base::ElapsedTimer> advertising_timer_;
// Used for metrics to record advertising method.
quick_start_metrics::AdvertisingMethod advertising_method_;
QuickStartMetrics::AdvertisingMethod advertising_method_;
base::WeakPtrFactory<FastPairAdvertiser> weak_ptr_factory_{this};
};

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -430,10 +430,10 @@ TEST_F(FastPairAdvertiserTest, TestStartAdvertising_DeleteInErrorCallback) {
expected_failure_count_++;
histograms_.ExpectBucketCount(
"QuickStart.FastPairAdvertisementStarted.AdvertisingMethod",
quick_start_metrics::AdvertisingMethod::kQrCode, expected_failure_count_);
QuickStartMetrics::AdvertisingMethod::kQrCode, expected_failure_count_);
histograms_.ExpectBucketCount(
"QuickStart.FastPairAdvertisementEnded.AdvertisingMethod",
quick_start_metrics::AdvertisingMethod::kQrCode, expected_failure_count_);
QuickStartMetrics::AdvertisingMethod::kQrCode, expected_failure_count_);
histograms_.ExpectBucketCount(
"QuickStart.FastPairAdvertisementEnded.Succeeded", false,
expected_failure_count_);
Expand Down

0 comments on commit 2be3601

Please sign in to comment.