Trust Tokens: Split the metrics recording helper classes' initialization

This is the first of a series of two CLs adding support for slicing
Trust Tokens operation outcome metrics by whether an operation is
platform-provided (https://bit.ly/platform-provided-trust-tokens).

The overall implementation approach is to add a new delegate to
TrustTokenRequestIssuanceHelper that it notifies once it discovers a
particular operation will be platform-provided; this delegate will be
implemented by TrustTokenOperationMetricsRecorder, which will update the
names of the metrics it writes to depending on whether the operation at
hand is platform-provided.

This change refactors refactors OperationTimingRequestHelperWrapper to
take a metrics recorder as an input instead of default-initializing one
as a member. This allows the child CL, crrev.com/c/2603584, to pass a
handle to a MetricsRecorder to TrustTokenRequestIssuanceHelpers it
creates before wrapping the RequestIssuanceHelpers in their
OperationTimingRequestHelperWrappers.

(Before this change, there would be a cyclic dependency: the
OperationTimingRequestHelperWrapper and the RequestIssuanceHelper would
both need handles to each other. Refactoring MetricsRecorder to be
initialized separately from OperationTimingRequestHelperWrapper allows
getting around this without needing a kludge like adding an
IssuanceHelper::SetDelegate(Delegate*) method.)

The child CL will update TrustTokenRequestIssuanceHelper with the
new delegate interface and add the new metrics.

Bug: 1163019
Change-Id: I2fc40399e48c6b4722a1147633eac678ee960200
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2606141
Auto-Submit: David Van Cleve <davidvc@chromium.org>
Commit-Queue: Matt Menke <mmenke@chromium.org>
Reviewed-by: Matt Menke <mmenke@chromium.org>
Reviewed-by: Charlie Harrison <csharrison@chromium.org>
Cr-Commit-Position: refs/heads/master@{#841901}

services/network/trust_tokens/operation_timing_request_helper_wrapper.cc

@@ -7,9 +7,9 @@
 namespace network {
 
 OperationTimingRequestHelperWrapper::OperationTimingRequestHelperWrapper(
-    mojom::TrustTokenOperationType type,
+    std::unique_ptr<TrustTokenOperationMetricsRecorder> metrics_recorder,
     std::unique_ptr<TrustTokenRequestHelper> helper)
-    : type_(type), helper_(std::move(helper)) {}
+    : recorder_(std::move(metrics_recorder)), helper_(std::move(helper)) {}
 
 OperationTimingRequestHelperWrapper::~OperationTimingRequestHelperWrapper() =
     default;
@@ -17,7 +17,7 @@ OperationTimingRequestHelperWrapper::~OperationTimingRequestHelperWrapper() =
 void OperationTimingRequestHelperWrapper::Begin(
     net::URLRequest* request,
     base::OnceCallback<void(mojom::TrustTokenOperationStatus)> done) {
-  recorder_.BeginBegin(type_);
+  recorder_->BeginBegin();
   helper_->Begin(
       request, base::BindOnce(&OperationTimingRequestHelperWrapper::FinishBegin,
                               weak_factory_.GetWeakPtr(), std::move(done)));
@@ -26,7 +26,7 @@ void OperationTimingRequestHelperWrapper::Begin(
 void OperationTimingRequestHelperWrapper::Finalize(
     mojom::URLResponseHead* response,
     base::OnceCallback<void(mojom::TrustTokenOperationStatus)> done) {
-  recorder_.BeginFinalize();
+  recorder_->BeginFinalize();
   helper_->Finalize(
       response,
       base::BindOnce(&OperationTimingRequestHelperWrapper::FinishFinalize,
@@ -36,14 +36,14 @@ void OperationTimingRequestHelperWrapper::Finalize(
 void OperationTimingRequestHelperWrapper::FinishBegin(
     base::OnceCallback<void(mojom::TrustTokenOperationStatus)> done,
     mojom::TrustTokenOperationStatus status) {
-  recorder_.FinishBegin(status);
+  recorder_->FinishBegin(status);
   std::move(done).Run(status);
 }
 
 void OperationTimingRequestHelperWrapper::FinishFinalize(
     base::OnceCallback<void(mojom::TrustTokenOperationStatus)> done,
     mojom::TrustTokenOperationStatus status) {
-  recorder_.FinishFinalize(status);
+  recorder_->FinishFinalize(status);
   std::move(done).Run(status);
 }
 

services/network/trust_tokens/operation_timing_request_helper_wrapper.h

@@ -10,6 +10,7 @@
 #include "base/memory/weak_ptr.h"
 #include "services/network/trust_tokens/trust_token_operation_metrics_recorder.h"
 #include "services/network/trust_tokens/trust_token_request_helper.h"
+#include "services/network/trust_tokens/trust_token_request_issuance_helper.h"
 
 namespace network {
 
@@ -19,7 +20,7 @@ namespace network {
 class OperationTimingRequestHelperWrapper : public TrustTokenRequestHelper {
  public:
   explicit OperationTimingRequestHelperWrapper(
-      mojom::TrustTokenOperationType type,
+      std::unique_ptr<TrustTokenOperationMetricsRecorder> metrics_recorder,
       std::unique_ptr<TrustTokenRequestHelper> helper);
   ~OperationTimingRequestHelperWrapper() override;
 
@@ -46,8 +47,7 @@ class OperationTimingRequestHelperWrapper : public TrustTokenRequestHelper {
       base::OnceCallback<void(mojom::TrustTokenOperationStatus)> done,
       mojom::TrustTokenOperationStatus status);
 
-  mojom::TrustTokenOperationType type_;
-  TrustTokenOperationMetricsRecorder recorder_;
+  std::unique_ptr<TrustTokenOperationMetricsRecorder> recorder_;
   std::unique_ptr<TrustTokenRequestHelper> helper_;
 
   base::WeakPtrFactory<OperationTimingRequestHelperWrapper> weak_factory_{this};

services/network/trust_tokens/trust_token_operation_metrics_recorder.cc

@@ -31,6 +31,8 @@ base::StringPiece StatusToSuccessOrFailure(
   switch (status) {
     case mojom::TrustTokenOperationStatus::kOk:
     case mojom::TrustTokenOperationStatus::kAlreadyExists:
+    case mojom::TrustTokenOperationStatus::
+        kOperationSuccessfullyFulfilledLocally:
       return "Success";
     default:
       return "Failure";
@@ -54,9 +56,13 @@ const char kHistogramPartsSeparator[] = ".";
 
 }  // namespace
 
-void TrustTokenOperationMetricsRecorder::BeginBegin(
-    mojom::TrustTokenOperationType type) {
-  type_ = type;
+TrustTokenOperationMetricsRecorder::TrustTokenOperationMetricsRecorder(
+    mojom::TrustTokenOperationType type)
+    : type_(type) {}
+TrustTokenOperationMetricsRecorder::~TrustTokenOperationMetricsRecorder() =
+    default;
+
+void TrustTokenOperationMetricsRecorder::BeginBegin() {
   begin_start_ = base::TimeTicks::Now();
 }
 

services/network/trust_tokens/trust_token_operation_metrics_recorder.h

@@ -7,6 +7,7 @@
 
 #include "base/time/time.h"
 #include "services/network/public/mojom/trust_tokens.mojom.h"
+#include "services/network/trust_tokens/trust_token_request_issuance_helper.h"
 
 namespace network {
 
@@ -29,15 +30,16 @@ extern const char kTrustTokenBeginTimeHistogramNameBase[];
 // operation.
 class TrustTokenOperationMetricsRecorder final {
  public:
-  TrustTokenOperationMetricsRecorder() = default;
-  ~TrustTokenOperationMetricsRecorder() = default;
+  explicit TrustTokenOperationMetricsRecorder(
+      mojom::TrustTokenOperationType type);
+  ~TrustTokenOperationMetricsRecorder();
 
   TrustTokenOperationMetricsRecorder(
       const TrustTokenOperationMetricsRecorder&) = delete;
   TrustTokenOperationMetricsRecorder& operator=(
       const TrustTokenOperationMetricsRecorder&) = delete;
 
-  void BeginBegin(mojom::TrustTokenOperationType type);
+  void BeginBegin();
   void FinishBegin(mojom::TrustTokenOperationStatus status);
 
   void BeginFinalize();

services/network/trust_tokens/trust_token_operation_metrics_recorder_unittest.cc

@@ -15,10 +15,11 @@ namespace network {
 TEST(TrustTokenOperationMetricsRecorder, Success) {
   base::test::TaskEnvironment env(
       base::test::TaskEnvironment::TimeSource::MOCK_TIME);
-  TrustTokenOperationMetricsRecorder recorder;
+  TrustTokenOperationMetricsRecorder recorder(
+      mojom::TrustTokenOperationType::kIssuance);
   base::HistogramTester histograms;
 
-  recorder.BeginBegin(mojom::TrustTokenOperationType::kIssuance);
+  recorder.BeginBegin();
   env.FastForwardBy(base::TimeDelta::FromSeconds(1));
   recorder.FinishBegin(mojom::TrustTokenOperationStatus::kOk);
 
@@ -59,10 +60,11 @@ TEST(TrustTokenOperationMetricsRecorder, Success) {
 TEST(TrustTokenOperationMetricsRecorder, BeginFailure) {
   base::test::TaskEnvironment env(
       base::test::TaskEnvironment::TimeSource::MOCK_TIME);
-  TrustTokenOperationMetricsRecorder recorder;
+  TrustTokenOperationMetricsRecorder recorder(
+      mojom::TrustTokenOperationType::kRedemption);
   base::HistogramTester histograms;
 
-  recorder.BeginBegin(mojom::TrustTokenOperationType::kRedemption);
+  recorder.BeginBegin();
   env.FastForwardBy(base::TimeDelta::FromSeconds(1));
   recorder.FinishBegin(mojom::TrustTokenOperationStatus::kUnknownError);
 
@@ -77,10 +79,11 @@ TEST(TrustTokenOperationMetricsRecorder, BeginFailure) {
 TEST(TrustTokenOperationMetricsRecorder, FinalizeFailure) {
   base::test::TaskEnvironment env(
       base::test::TaskEnvironment::TimeSource::MOCK_TIME);
-  TrustTokenOperationMetricsRecorder recorder;
+  TrustTokenOperationMetricsRecorder recorder(
+      mojom::TrustTokenOperationType::kSigning);
   base::HistogramTester histograms;
 
-  recorder.BeginBegin(mojom::TrustTokenOperationType::kSigning);
+  recorder.BeginBegin();
   env.FastForwardBy(base::TimeDelta::FromSeconds(1));
   recorder.FinishBegin(mojom::TrustTokenOperationStatus::kOk);
 

services/network/trust_tokens/trust_token_request_helper_factory.cc

@@ -25,8 +25,10 @@
 #include "services/network/trust_tokens/local_trust_token_operation_delegate.h"
 #include "services/network/trust_tokens/local_trust_token_operation_delegate_impl.h"
 #include "services/network/trust_tokens/operating_system_matching.h"
+#include "services/network/trust_tokens/operation_timing_request_helper_wrapper.h"
 #include "services/network/trust_tokens/suitable_trust_token_origin.h"
 #include "services/network/trust_tokens/trust_token_key_commitment_controller.h"
+#include "services/network/trust_tokens/trust_token_operation_metrics_recorder.h"
 #include "services/network/trust_tokens/trust_token_parameterization.h"
 #include "services/network/trust_tokens/trust_token_request_canonicalizer.h"
 #include "services/network/trust_tokens/trust_token_request_redemption_helper.h"
@@ -140,31 +142,36 @@ void TrustTokenRequestHelperFactory::ConstructHelperUsingStore(
     TrustTokenStore* store) {
   DCHECK(params);
 
+  auto metrics_recorder =
+      std::make_unique<TrustTokenOperationMetricsRecorder>(params->type);
+
   switch (params->type) {
     case mojom::TrustTokenOperationType::kIssuance: {
       LogOutcome(net_log, params->type,
                  Outcome::kSuccessfullyCreatedAnIssuanceHelper);
-      std::move(done).Run(std::unique_ptr<TrustTokenRequestHelper>(
-          new TrustTokenRequestIssuanceHelper(
-              std::move(top_frame_origin), store, key_commitment_getter_,
-              std::make_unique<BoringsslTrustTokenIssuanceCryptographer>(),
-              std::make_unique<LocalTrustTokenOperationDelegateImpl>(
-                  context_client_provider_),
-              base::BindRepeating(&IsCurrentOperatingSystem),
-              std::move(net_log))));
+      auto helper = std::make_unique<TrustTokenRequestIssuanceHelper>(
+          std::move(top_frame_origin), store, key_commitment_getter_,
+          std::make_unique<BoringsslTrustTokenIssuanceCryptographer>(),
+          std::make_unique<LocalTrustTokenOperationDelegateImpl>(
+              context_client_provider_),
+          base::BindRepeating(&IsCurrentOperatingSystem), std::move(net_log));
+      std::move(done).Run(TrustTokenStatusOrRequestHelper(
+          std::make_unique<OperationTimingRequestHelperWrapper>(
+              std::move(metrics_recorder), std::move(helper))));
       return;
     }
 
     case mojom::TrustTokenOperationType::kRedemption: {
       LogOutcome(net_log, params->type,
                  Outcome::kSuccessfullyCreatedARedemptionHelper);
-      std::move(done).Run(std::unique_ptr<TrustTokenRequestHelper>(
-          new TrustTokenRequestRedemptionHelper(
-              std::move(top_frame_origin), params->refresh_policy, store,
-              key_commitment_getter_,
-              std::make_unique<Ed25519KeyPairGenerator>(),
-              std::make_unique<BoringsslTrustTokenRedemptionCryptographer>(),
-              std::move(net_log))));
+      auto helper = std::make_unique<TrustTokenRequestRedemptionHelper>(
+          std::move(top_frame_origin), params->refresh_policy, store,
+          key_commitment_getter_, std::make_unique<Ed25519KeyPairGenerator>(),
+          std::make_unique<BoringsslTrustTokenRedemptionCryptographer>(),
+          std::move(net_log));
+      std::move(done).Run(TrustTokenStatusOrRequestHelper(
+          std::make_unique<OperationTimingRequestHelperWrapper>(
+              std::move(metrics_recorder), std::move(helper))));
       return;
     }
 
@@ -199,12 +206,14 @@ void TrustTokenRequestHelperFactory::ConstructHelperUsingStore(
 
       LogOutcome(net_log, params->type,
                  Outcome::kSuccessfullyCreatedASigningHelper);
-      std::move(done).Run(std::unique_ptr<TrustTokenRequestHelper>(
-          new TrustTokenRequestSigningHelper(
-              store, std::move(signing_params),
-              std::make_unique<Ed25519TrustTokenRequestSigner>(),
-              std::make_unique<TrustTokenRequestCanonicalizer>(),
-              std::move(net_log))));
+      auto helper = std::make_unique<TrustTokenRequestSigningHelper>(
+          store, std::move(signing_params),
+          std::make_unique<Ed25519TrustTokenRequestSigner>(),
+          std::make_unique<TrustTokenRequestCanonicalizer>(),
+          std::move(net_log));
+      std::move(done).Run(TrustTokenStatusOrRequestHelper(
+          std::make_unique<OperationTimingRequestHelperWrapper>(
+              std::move(metrics_recorder), std::move(helper))));
       return;
     }
   }

services/network/url_loader.cc

@@ -70,7 +70,6 @@
 #include "services/network/resource_scheduler/resource_scheduler_client.h"
 #include "services/network/sec_header_helpers.h"
 #include "services/network/throttling/scoped_throttling_token.h"
-#include "services/network/trust_tokens/operation_timing_request_helper_wrapper.h"
 #include "services/network/trust_tokens/trust_token_request_helper.h"
 #include "url/origin.h"
 
@@ -879,8 +878,7 @@ void URLLoader::OnDoneConstructingTrustTokenHelper(
     return;
   }
 
-  trust_token_helper_ = std::make_unique<OperationTimingRequestHelperWrapper>(
-      type, status_or_helper.TakeOrCrash());
+  trust_token_helper_ = status_or_helper.TakeOrCrash();
   trust_token_helper_->Begin(
       url_request_.get(),
       base::BindOnce(&URLLoader::OnDoneBeginningTrustTokenOperation,