[sync] Break down Sync.TrustedVaultURLFetchResponse per reason

This breakdown allows a more fine-grained interpretation of error buckets, as the distribution may be significantly different for each request type/reason. Change-Id: I655fe6fab6e0b82f68cdd65e368d2e4d831f3b52 Bug: 1334939 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3721035 Reviewed-by: Maksim Moskvitin <[email protected]> Reviewed-by: Rushan Suleymanov <[email protected]> Commit-Queue: Mikel Astiz <[email protected]> Cr-Commit-Position: refs/heads/main@{#1017598}
hadissulejmani · Jun 24, 2022 · 55255b4 · 55255b4
1 parent 13a47b1
commit 55255b4
Show file tree

Hide file tree

Showing 9 changed files with 102 additions and 17 deletions.
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
@@ -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
@@ -4,10 +4,33 @@
 
 #include "components/sync/driver/trusted_vault_histograms.h"
 
+#include <string>
+
 #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
@@ -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
@@ -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
@@ -76,11 +76,13 @@ TrustedVaultRequest::TrustedVaultRequest(
     HttpMethod http_method,
     const GURL& request_url,
     const absl::optional<std::string>& serialized_request_proto,
-    scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory)
+    scoped_refptr<network::SharedURLLoaderFactory> 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
@@ -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<std::string>& serialized_request_proto,
-      scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory);
+      scoped_refptr<network::SharedURLLoaderFactory> 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<std::string> serialized_request_proto_;
   const scoped_refptr<network::SharedURLLoaderFactory> 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
@@ -92,7 +92,8 @@ class TrustedVaultRequestTest : public testing::Test {
 
     auto request = std::make_unique<TrustedVaultRequest>(
         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
@@ -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
@@ -1274,7 +1274,7 @@ [email protected].
   </summary>
 </histogram>
 
-<histogram name="Sync.TrustedVaultURLFetchResponse"
+<histogram name="Sync.TrustedVaultURLFetchResponse{Reason}"
     enum="CombinedHttpResponseAndNetErrorCode" expires_after="2022-11-13">
   <owner>[email protected]</owner>
   <owner>[email protected]</owner>
@@ -1284,6 +1284,15 @@ [email protected].
     Trusted Vault server (aka Security Domain Service). Note that requests that
     timed out are not covered by this histogram.
   </summary>
+  <token key="Reason">
+    <variant name="" summary="Any reason"/>
+    <variant name=".DownloadIsRecoverabilityDegraded"
+        summary="Downloading of the recoverability-degraded state"/>
+    <variant name=".DownloadKeys" summary="Downloading of keys"/>
+    <variant name=".RegisterDevice" summary="Local device registration"/>
+    <variant name=".RegisterUnspecifiedAuthenticationFactor"
+        summary="Registration of an authentication factor of unspecified type"/>
+  </token>
 </histogram>
 
 <histogram name="Sync.TypedURLDatabaseError" enum="SyncTypedUrlDatabaseError"