diff --git a/chrome/browser/sync/test/integration/single_client_nigori_sync_test.cc b/chrome/browser/sync/test/integration/single_client_nigori_sync_test.cc index 8f2824e9f4b9c2..9b7c18932ce08e 100644 --- a/chrome/browser/sync/test/integration/single_client_nigori_sync_test.cc +++ b/chrome/browser/sync/test/integration/single_client_nigori_sync_test.cc @@ -1376,6 +1376,10 @@ IN_PROC_BROWSER_TEST_F(SingleClientNigoriWithWebApiTest, "Sync.TrustedVaultDownloadKeysStatus", /*sample=*/syncer::TrustedVaultDownloadKeysStatus::kSuccess, /*expected_bucket_count=*/1); + histogram_tester.ExpectUniqueSample( + "Sync.TrustedVaultURLFetchResponse.DownloadKeys", + /*sample=*/200, + /*expected_bucket_count=*/1); } // Regression test for crbug.com/1267391: after following key rotation the diff --git a/components/sync/driver/trusted_vault_histograms.cc b/components/sync/driver/trusted_vault_histograms.cc index 6233d94bcb1405..f34004c103fb91 100644 --- a/components/sync/driver/trusted_vault_histograms.cc +++ b/components/sync/driver/trusted_vault_histograms.cc @@ -4,10 +4,33 @@ #include "components/sync/driver/trusted_vault_histograms.h" +#include + #include "base/metrics/histogram_functions.h" +#include "base/strings/strcat.h" namespace syncer { +namespace { + +std::string GetReasonSuffix(TrustedVaultURLFetchReasonForUMA reason) { + switch (reason) { + case TrustedVaultURLFetchReasonForUMA::kUnspecified: + return std::string(); + case TrustedVaultURLFetchReasonForUMA::kRegisterDevice: + return "RegisterDevice"; + case TrustedVaultURLFetchReasonForUMA:: + kRegisterUnspecifiedAuthenticationFactor: + return "RegisterUnspecifiedAuthenticationFactor"; + case TrustedVaultURLFetchReasonForUMA::kDownloadKeys: + return "DownloadKeys"; + case TrustedVaultURLFetchReasonForUMA::kDownloadIsRecoverabilityDegraded: + return "DownloadIsRecoverabilityDegraded"; + } +} + +} // namespace + void RecordTrustedVaultDeviceRegistrationState( TrustedVaultDeviceRegistrationStateForUMA registration_state) { base::UmaHistogramEnumeration("Sync.TrustedVaultDeviceRegistrationState", @@ -16,13 +39,23 @@ void RecordTrustedVaultDeviceRegistrationState( // Records url fetch response status (combined http and net error code). // Either |http_status| or |net_error| must be non zero. -void RecordTrustedVaultURLFetchResponse(int http_response_code, int net_error) { +void RecordTrustedVaultURLFetchResponse( + int http_response_code, + int net_error, + TrustedVaultURLFetchReasonForUMA reason) { DCHECK_LE(net_error, 0); DCHECK_GE(http_response_code, 0); - base::UmaHistogramSparse( - "Sync.TrustedVaultURLFetchResponse", - http_response_code == 0 ? net_error : http_response_code); + const int value = http_response_code == 0 ? net_error : http_response_code; + const std::string suffix = GetReasonSuffix(reason); + + base::UmaHistogramSparse("Sync.TrustedVaultURLFetchResponse", value); + + if (!suffix.empty()) { + base::UmaHistogramSparse( + base::StrCat({"Sync.TrustedVaultURLFetchResponse", ".", suffix}), + value); + } } } // namespace syncer diff --git a/components/sync/driver/trusted_vault_histograms.h b/components/sync/driver/trusted_vault_histograms.h index b3b5bd6c861ce8..63c5dfda4eca8c 100644 --- a/components/sync/driver/trusted_vault_histograms.h +++ b/components/sync/driver/trusted_vault_histograms.h @@ -20,13 +20,26 @@ enum class TrustedVaultDeviceRegistrationStateForUMA { kMaxValue = kAttemptingRegistrationWithPersistentAuthError, }; +// Used to provide UMA metric breakdowns. +enum class TrustedVaultURLFetchReasonForUMA { + kUnspecified, + kRegisterDevice, + kRegisterUnspecifiedAuthenticationFactor, + kDownloadKeys, + kDownloadIsRecoverabilityDegraded, +}; + void RecordTrustedVaultDeviceRegistrationState( TrustedVaultDeviceRegistrationStateForUMA registration_state); // Records url fetch response status (combined http and net error code). If // |http_response_code| is non-zero, it will be recorded, otherwise |net_error| // will be recorded. -void RecordTrustedVaultURLFetchResponse(int http_response_code, int net_error); +void RecordTrustedVaultURLFetchResponse( + int http_response_code, + int net_error, + TrustedVaultURLFetchReasonForUMA reason = + TrustedVaultURLFetchReasonForUMA::kUnspecified); } // namespace syncer diff --git a/components/sync/trusted_vault/trusted_vault_connection_impl.cc b/components/sync/trusted_vault/trusted_vault_connection_impl.cc index 8f440830f4e913..ccb05cd24f5577 100644 --- a/components/sync/trusted_vault/trusted_vault_connection_impl.cc +++ b/components/sync/trusted_vault/trusted_vault_connection_impl.cc @@ -241,6 +241,21 @@ void ProcessDownloadIsRecoverabilityDegradedResponse( std::move(callback).Run(status); } +TrustedVaultURLFetchReasonForUMA +GetURLFetchReasonForUMAForJoinSecurityDomainsRequest( + AuthenticationFactorType authentication_factor_type) { + switch (authentication_factor_type) { + case AuthenticationFactorType::kPhysicalDevice: + return TrustedVaultURLFetchReasonForUMA::kRegisterDevice; + case AuthenticationFactorType::kUnspecified: + return TrustedVaultURLFetchReasonForUMA:: + kRegisterUnspecifiedAuthenticationFactor; + } + + NOTREACHED(); + return TrustedVaultURLFetchReasonForUMA::kUnspecified; +} + } // namespace TrustedVaultConnectionImpl::TrustedVaultConnectionImpl( @@ -298,8 +313,8 @@ TrustedVaultConnectionImpl::DownloadNewKeys( GURL(trusted_vault_service_url_.spec() + GetGetSecurityDomainMemberURLPathAndQuery( device_key_pair->public_key().ExportToBytes())), - /*serialized_request_proto=*/absl::nullopt, - GetOrCreateURLLoaderFactory()); + /*serialized_request_proto=*/absl::nullopt, GetOrCreateURLLoaderFactory(), + TrustedVaultURLFetchReasonForUMA::kDownloadKeys); request->FetchAccessTokenAndSendRequest( account_info.account_id, access_token_fetcher_.get(), @@ -321,8 +336,8 @@ TrustedVaultConnectionImpl::DownloadIsRecoverabilityDegraded( TrustedVaultRequest::HttpMethod::kGet, GURL(trusted_vault_service_url_.spec() + kGetSecurityDomainURLPathAndQuery), - /*serialized_request_proto=*/absl::nullopt, - GetOrCreateURLLoaderFactory()); + /*serialized_request_proto=*/absl::nullopt, GetOrCreateURLLoaderFactory(), + TrustedVaultURLFetchReasonForUMA::kDownloadIsRecoverabilityDegraded); request->FetchAccessTokenAndSendRequest( account_info.account_id, access_token_fetcher_.get(), @@ -350,7 +365,9 @@ TrustedVaultConnectionImpl::SendJoinSecurityDomainsRequest( authentication_factor_public_key, authentication_factor_type, authentication_factor_type_hint) .SerializeAsString(), - GetOrCreateURLLoaderFactory()); + GetOrCreateURLLoaderFactory(), + GetURLFetchReasonForUMAForJoinSecurityDomainsRequest( + authentication_factor_type)); request->FetchAccessTokenAndSendRequest( account_info.account_id, access_token_fetcher_.get(), diff --git a/components/sync/trusted_vault/trusted_vault_request.cc b/components/sync/trusted_vault/trusted_vault_request.cc index 34da26bc06eca9..e397788a0e479c 100644 --- a/components/sync/trusted_vault/trusted_vault_request.cc +++ b/components/sync/trusted_vault/trusted_vault_request.cc @@ -76,11 +76,13 @@ TrustedVaultRequest::TrustedVaultRequest( HttpMethod http_method, const GURL& request_url, const absl::optional& serialized_request_proto, - scoped_refptr url_loader_factory) + scoped_refptr url_loader_factory, + TrustedVaultURLFetchReasonForUMA reason_for_uma) : http_method_(http_method), request_url_(request_url), serialized_request_proto_(serialized_request_proto), - url_loader_factory_(std::move(url_loader_factory)) { + url_loader_factory_(std::move(url_loader_factory)), + reason_for_uma_(reason_for_uma) { DCHECK(url_loader_factory_); DCHECK(http_method == HttpMethod::kPost || !serialized_request_proto.has_value()); @@ -129,7 +131,8 @@ void TrustedVaultRequest::OnURLLoadComplete( } RecordTrustedVaultURLFetchResponse(/*http_response_code=*/http_response_code, - /*net_error=*/url_loader_->NetError()); + /*net_error=*/url_loader_->NetError(), + reason_for_uma_); std::string response_content = response_body ? *response_body : std::string(); if (http_response_code == net::HTTP_BAD_REQUEST) { diff --git a/components/sync/trusted_vault/trusted_vault_request.h b/components/sync/trusted_vault/trusted_vault_request.h index e66f407636d3f2..ebf56a21432a75 100644 --- a/components/sync/trusted_vault/trusted_vault_request.h +++ b/components/sync/trusted_vault/trusted_vault_request.h @@ -10,6 +10,7 @@ #include "base/callback.h" #include "base/memory/weak_ptr.h" +#include "components/sync/driver/trusted_vault_histograms.h" #include "components/sync/trusted_vault/trusted_vault_connection.h" #include "third_party/abseil-cpp/absl/types/optional.h" #include "url/gurl.h" @@ -62,7 +63,8 @@ class TrustedVaultRequest : public TrustedVaultConnection::Request { HttpMethod http_method, const GURL& request_url, const absl::optional& serialized_request_proto, - scoped_refptr url_loader_factory); + scoped_refptr url_loader_factory, + TrustedVaultURLFetchReasonForUMA reason_for_uma); TrustedVaultRequest(const TrustedVaultRequest& other) = delete; TrustedVaultRequest& operator=(const TrustedVaultRequest& other) = delete; ~TrustedVaultRequest() override; @@ -93,6 +95,7 @@ class TrustedVaultRequest : public TrustedVaultConnection::Request { const GURL request_url_; const absl::optional serialized_request_proto_; const scoped_refptr url_loader_factory_; + const TrustedVaultURLFetchReasonForUMA reason_for_uma_; CompletionCallback completion_callback_; diff --git a/components/sync/trusted_vault/trusted_vault_request_unittest.cc b/components/sync/trusted_vault/trusted_vault_request_unittest.cc index 51f9209de4392f..080b6c77bcc5f1 100644 --- a/components/sync/trusted_vault/trusted_vault_request_unittest.cc +++ b/components/sync/trusted_vault/trusted_vault_request_unittest.cc @@ -92,7 +92,8 @@ class TrustedVaultRequestTest : public testing::Test { auto request = std::make_unique( http_method, GURL(kRequestUrl), request_body, - shared_url_loader_factory_); + shared_url_loader_factory_, + TrustedVaultURLFetchReasonForUMA::kUnspecified); request->FetchAccessTokenAndSendRequest(account_id, &access_token_fetcher, std::move(completion_callback)); return request; diff --git a/ios/web_view/internal/sync/cwv_trusted_vault_utils.mm b/ios/web_view/internal/sync/cwv_trusted_vault_utils.mm index 818b8aba680c69..e352feafd1fe04 100644 --- a/ios/web_view/internal/sync/cwv_trusted_vault_utils.mm +++ b/ios/web_view/internal/sync/cwv_trusted_vault_utils.mm @@ -44,7 +44,9 @@ + (void)logTrustedVaultDidUpdateState:(CWVTrustedVaultState)state { } + (void)logTrustedVaultDidReceiveHTTPStatusCode:(NSInteger)statusCode { - syncer::RecordTrustedVaultURLFetchResponse(statusCode, /*net_error=*/0); + syncer::RecordTrustedVaultURLFetchResponse( + statusCode, /*net_error=*/0, + syncer::TrustedVaultURLFetchReasonForUMA::kUnspecified); } + (void)logTrustedVaultDidFailKeyDistribution:(NSError*)error { diff --git a/tools/metrics/histograms/metadata/sync/histograms.xml b/tools/metrics/histograms/metadata/sync/histograms.xml index 2e78b6286f2719..87ae5f5bc7a344 100644 --- a/tools/metrics/histograms/metadata/sync/histograms.xml +++ b/tools/metrics/histograms/metadata/sync/histograms.xml @@ -1274,7 +1274,7 @@ chromium-metrics-reviews@google.com. - mmoskvitin@google.com mastiz@chromium.org @@ -1284,6 +1284,15 @@ chromium-metrics-reviews@google.com. Trusted Vault server (aka Security Domain Service). Note that requests that timed out are not covered by this histogram. + + + + + + +