Convert DataTypeDebugInfoEmitter into util function

Please see crbug.com/1137896#c1 for context.
This CL introduces a new util function which achieves the same
UMA-recording functionality of DataTypeDebugInfoEmitter. All users of
the DataTypeDebugInfoEmitter API are migrated to the new function.

In particular, for CommitContributionImpl this means the histogram will
be recorded earlier: it used to be recorded after the server responded
(in CleanUp), whereas now it's recorded before sending the commit request
(in AddToCommitMessage). This is an acceptable solution because the commit
is not aborted between these two events (Init can't return nullptr after
this [1]). It may still slightly influence the metrics around shutdown,
since the engine is synchronous and some steps are slow.

Removing the class itself together with some clean up is left to
crrev.com/c/2470528.

[1] https://source.chromium.org/chromium/chromium/src/+/a450ab794839b63f9c2ba982f738945f90e9fee2:components/sync/engine_impl/commit.cc;l=135


Bug: 1137896
Change-Id: I9f3ba96d4aad9a434e9b326458de8c392d4946b4
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2470877
Commit-Queue: Victor Vianna <victorvianna@google.com>
Reviewed-by: Mikel Astiz <mastiz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#817103}

components/sync/BUILD.gn

@@ -106,6 +106,8 @@ static_library("rest_of_sync") {
     "engine_impl/cycle/data_type_tracker.cc",
     "engine_impl/cycle/data_type_tracker.h",
     "engine_impl/cycle/debug_info_getter.h",
+    "engine_impl/cycle/entity_change_metric_recording.cc",
+    "engine_impl/cycle/entity_change_metric_recording.h",
     "engine_impl/cycle/nudge_tracker.cc",
     "engine_impl/cycle/nudge_tracker.h",
     "engine_impl/cycle/status_controller.cc",
@@ -436,7 +438,6 @@ source_set("unit_tests") {
     "engine_impl/bookmark_update_preprocessing_unittest.cc",
     "engine_impl/commit_contribution_impl_unittest.cc",
     "engine_impl/commit_processor_unittest.cc",
-    "engine_impl/cycle/data_type_debug_info_emitter_unittest.cc",
     "engine_impl/cycle/nudge_tracker_unittest.cc",
     "engine_impl/cycle/status_controller_unittest.cc",
     "engine_impl/debug_info_event_listener_unittest.cc",

components/sync/engine_impl/commit_contribution_impl.cc

@@ -12,6 +12,7 @@
 #include "components/sync/base/time.h"
 #include "components/sync/base/unique_position.h"
 #include "components/sync/engine/commit_and_get_updates_types.h"
+#include "components/sync/engine_impl/cycle/entity_change_metric_recording.h"
 #include "components/sync/engine_impl/model_type_worker.h"
 #include "components/sync/protocol/proto_value_conversions.h"
 
@@ -27,7 +28,6 @@ CommitContributionImpl::CommitContributionImpl(
     base::OnceCallback<void(SyncCommitError)> on_full_commit_failure_callback,
     Cryptographer* cryptographer,
     PassphraseType passphrase_type,
-    DataTypeDebugInfoEmitter* debug_info_emitter,
     bool only_commit_specifics)
     : type_(type),
       on_commit_response_callback_(std::move(on_commit_response_callback)),
@@ -38,7 +38,6 @@ CommitContributionImpl::CommitContributionImpl(
       context_(context),
       commit_requests_(std::move(commit_requests)),
       cleaned_up_(false),
-      debug_info_emitter_(debug_info_emitter),
       only_commit_specifics_(only_commit_specifics) {}
 
 CommitContributionImpl::~CommitContributionImpl() {
@@ -52,7 +51,6 @@ void CommitContributionImpl::AddToCommitMessage(
 
   commit_message->mutable_entries()->Reserve(commit_message->entries_size() +
                                              commit_requests_.size());
-  CommitCounters* counters = debug_info_emitter_->GetMutableCommitCounters();
 
   for (const auto& commit_request : commit_requests_) {
     sync_pb::SyncEntity* sync_entity = commit_message->add_entries();
@@ -72,13 +70,12 @@ void CommitContributionImpl::AddToCommitMessage(
     CHECK(
         !sync_entity->specifics().password().has_client_only_encrypted_data());
 
-    // Update the relevant counter based on the type of the commit request.
     if (commit_request->entity->is_deleted()) {
-      counters->num_deletion_commits_attempted++;
+      RecordEntityChangeMetrics(type_, ModelTypeEntityChange::kLocalDeletion);
     } else if (commit_request->base_version <= 0) {
-      counters->num_creation_commits_attempted++;
+      RecordEntityChangeMetrics(type_, ModelTypeEntityChange::kLocalCreation);
     } else {
-      counters->num_update_commits_attempted++;
+      RecordEntityChangeMetrics(type_, ModelTypeEntityChange::kLocalUpdate);
     }
   }
 
@@ -190,9 +187,8 @@ void CommitContributionImpl::ProcessCommitFailure(
 }
 
 void CommitContributionImpl::CleanUp() {
+  // TODO(crbug.com/1137896): Remove this method, is has no use anymore.
   cleaned_up_ = true;
-
-  debug_info_emitter_->EmitCommitCountersUpdate();
 }
 
 size_t CommitContributionImpl::GetNumEntries() const {

components/sync/engine_impl/commit_contribution_impl.h

@@ -15,7 +15,6 @@
 #include "components/sync/base/passphrase_enums.h"
 #include "components/sync/engine/commit_and_get_updates_types.h"
 #include "components/sync/engine_impl/commit_contribution.h"
-#include "components/sync/engine_impl/cycle/data_type_debug_info_emitter.h"
 #include "components/sync/protocol/sync.pb.h"
 
 namespace syncer {
@@ -43,7 +42,6 @@ class CommitContributionImpl : public CommitContribution {
       base::OnceCallback<void(SyncCommitError)> on_full_commit_failure_callback,
       Cryptographer* cryptographer,
       PassphraseType passphrase_type,
-      DataTypeDebugInfoEmitter* debug_info_emitter,
       bool only_commit_specifics);
   ~CommitContributionImpl() override;
 
@@ -99,8 +97,6 @@ class CommitContributionImpl : public CommitContribution {
   // that CleanUp() is called before the object is destructed.
   bool cleaned_up_;
 
-  DataTypeDebugInfoEmitter* debug_info_emitter_;
-
   // Don't send any metadata to server, only specifics. This is needed for
   // commit only types to save bandwidth.
   bool only_commit_specifics_;

components/sync/engine_impl/commit_contribution_impl_unittest.cc

@@ -165,8 +165,6 @@ TEST(CommitContributionImplTest,
                      &request_data->specifics_hash);
   request_data->entity = std::move(data);
 
-  DataTypeDebugInfoEmitter debug_info_emitter(PASSWORDS);
-
   std::unique_ptr<CryptographerImpl> cryptographer =
       CryptographerImpl::FromSingleKeyForTesting("dummy");
 
@@ -177,7 +175,6 @@ TEST(CommitContributionImplTest,
       /*on_commit_response_callback=*/base::NullCallback(),
       /*on_full_commit_failure_callback=*/base::NullCallback(),
       cryptographer.get(), PassphraseType::kImplicitPassphrase,
-      &debug_info_emitter,
       /*only_commit_specifics=*/false);
 
   sync_pb::ClientToServerMessage msg;
@@ -231,8 +228,6 @@ TEST(CommitContributionImplTest,
                      &request_data->specifics_hash);
   request_data->entity = std::move(data);
 
-  DataTypeDebugInfoEmitter debug_info_emitter(PASSWORDS);
-
   std::unique_ptr<CryptographerImpl> cryptographer =
       CryptographerImpl::FromSingleKeyForTesting("dummy");
 
@@ -243,7 +238,6 @@ TEST(CommitContributionImplTest,
       /*on_commit_response_callback=*/base::NullCallback(),
       /*on_full_commit_failure_callback=*/base::NullCallback(),
       cryptographer.get(), PassphraseType::kCustomPassphrase,
-      &debug_info_emitter,
       /*only_commit_specifics=*/false);
 
   sync_pb::ClientToServerMessage msg;
@@ -276,8 +270,6 @@ TEST(CommitContributionImplTest, ShouldPropagateFailedItemsOnCommitResponse) {
   CommitRequestDataList requests_data;
   requests_data.push_back(std::move(request_data));
 
-  DataTypeDebugInfoEmitter debug_info_emitter(PASSWORDS);
-
   std::unique_ptr<CryptographerImpl> cryptographer =
       CryptographerImpl::CreateEmpty();
 
@@ -298,7 +290,6 @@ TEST(CommitContributionImplTest, ShouldPropagateFailedItemsOnCommitResponse) {
       std::move(on_commit_response_callback),
       /*on_full_commit_failure_callback=*/base::NullCallback(),
       cryptographer.get(), PassphraseType::kCustomPassphrase,
-      &debug_info_emitter,
       /*only_commit_specifics=*/false);
 
   sync_pb::ClientToServerMessage msg;
@@ -332,8 +323,6 @@ TEST(CommitContributionImplTest, ShouldPropagateFailedItemsOnCommitResponse) {
 }
 
 TEST(CommitContributionImplTest, ShouldPropagateFullCommitFailure) {
-  DataTypeDebugInfoEmitter debug_info_emitter(BOOKMARKS);
-
   base::MockOnceCallback<void(SyncCommitError commit_error)>
       on_commit_failure_callback;
   EXPECT_CALL(on_commit_failure_callback, Run(SyncCommitError::kNetworkError));
@@ -342,7 +331,7 @@ TEST(CommitContributionImplTest, ShouldPropagateFullCommitFailure) {
       BOOKMARKS, sync_pb::DataTypeContext(), CommitRequestDataList(),
       /*on_commit_response_callback=*/base::NullCallback(),
       on_commit_failure_callback.Get(), /*cryptographer=*/nullptr,
-      PassphraseType::kKeystorePassphrase, &debug_info_emitter,
+      PassphraseType::kKeystorePassphrase,
       /*only_commit_specifics=*/false);
 
   contribution.ProcessCommitFailure(SyncCommitError::kNetworkError);

components/sync/engine_impl/cycle/data_type_debug_info_emitter.cc

@@ -10,54 +10,7 @@
 
 namespace syncer {
 
-namespace {
-
-const char kModelTypeEntityChangeHistogramPrefix[] =
-    "Sync.ModelTypeEntityChange3.";
-
-// Values corrospond to a UMA histogram, do not modify, or delete any values.
-// Add new values only directly before COUNT.
-enum ModelTypeEntityChange {
-  LOCAL_DELETION = 0,
-  LOCAL_CREATION = 1,
-  LOCAL_UPDATE = 2,
-  REMOTE_DELETION = 3,
-  REMOTE_NON_INITIAL_UPDATE = 4,
-  REMOTE_INITIAL_UPDATE = 5,
-  MODEL_TYPE_ENTITY_CHANGE_COUNT = 6
-};
-
-void EmitNewChangesToUma(int count,
-                         ModelTypeEntityChange bucket,
-                         base::HistogramBase* histogram) {
-  DCHECK_GE(count, 0);
-  if (count > 0) {
-    histogram->AddCount(bucket, count);
-  }
-}
-
-// We'll add many values in this histogram. Since the name of the histogram is
-// not static here, we cannot use the (efficient) macros that use caching of the
-// histogram object. The helper functions like UmaHistogramEnumeration are not
-// efficient and thus we need re-implement the code here, caching the resulting
-// histogram object.
-base::HistogramBase* GetModelTypeEntityChangeHistogram(ModelType type) {
-  std::string type_string = ModelTypeToHistogramSuffix(type);
-  std::string full_histogram_name =
-      kModelTypeEntityChangeHistogramPrefix + type_string;
-  return base::LinearHistogram::FactoryGet(
-      /*name=*/full_histogram_name, /*minimum=*/1,
-      /*maximum=*/MODEL_TYPE_ENTITY_CHANGE_COUNT,
-      /*bucket_count=*/MODEL_TYPE_ENTITY_CHANGE_COUNT + 1,
-      /*flagz=*/base::HistogramBase::kUmaTargetedHistogramFlag);
-}
-
-}  // namespace
-
-DataTypeDebugInfoEmitter::DataTypeDebugInfoEmitter(ModelType type)
-    : histogram_(GetModelTypeEntityChangeHistogram(type)) {
-  DCHECK(histogram_);
-}
+DataTypeDebugInfoEmitter::DataTypeDebugInfoEmitter(ModelType type) {}
 
 DataTypeDebugInfoEmitter::~DataTypeDebugInfoEmitter() {}
 
@@ -70,20 +23,6 @@ CommitCounters* DataTypeDebugInfoEmitter::GetMutableCommitCounters() {
 }
 
 void DataTypeDebugInfoEmitter::EmitCommitCountersUpdate() {
-  // Emit the newly added counts to UMA.
-  EmitNewChangesToUma(
-      /*count=*/commit_counters_.num_creation_commits_attempted -
-          emitted_commit_counters_.num_creation_commits_attempted,
-      /*bucket=*/LOCAL_CREATION, histogram_);
-  EmitNewChangesToUma(
-      /*count=*/commit_counters_.num_deletion_commits_attempted -
-          emitted_commit_counters_.num_deletion_commits_attempted,
-      /*bucket=*/LOCAL_DELETION, histogram_);
-  EmitNewChangesToUma(
-      /*count=*/commit_counters_.num_update_commits_attempted -
-          emitted_commit_counters_.num_update_commits_attempted,
-      /*bucket=*/LOCAL_UPDATE, histogram_);
-
   // Mark the current state of the counters as uploaded to UMA.
   emitted_commit_counters_ = commit_counters_;
 }
@@ -97,28 +36,6 @@ UpdateCounters* DataTypeDebugInfoEmitter::GetMutableUpdateCounters() {
 }
 
 void DataTypeDebugInfoEmitter::EmitUpdateCountersUpdate() {
-  // Emit the newly added counts to UMA.
-  EmitNewChangesToUma(
-      /*count=*/update_counters_.num_initial_updates_received -
-          emitted_update_counters_.num_initial_updates_received,
-      /*bucket=*/REMOTE_INITIAL_UPDATE, histogram_);
-  EmitNewChangesToUma(
-      /*count=*/update_counters_.num_non_initial_tombstone_updates_received -
-          emitted_update_counters_.num_non_initial_tombstone_updates_received,
-      /*bucket=*/REMOTE_DELETION, histogram_);
-  // The remote_non_initial_update type is not explicitly stored, we need to
-  // compute it as a diff of (all - deletions).
-  int emitted_remote_non_initial_updates_count =
-      emitted_update_counters_.num_non_initial_updates_received -
-      emitted_update_counters_.num_non_initial_tombstone_updates_received;
-  int remote_non_initial_updates_count =
-      update_counters_.num_non_initial_updates_received -
-      update_counters_.num_non_initial_tombstone_updates_received;
-  EmitNewChangesToUma(
-      /*count=*/remote_non_initial_updates_count -
-          emitted_remote_non_initial_updates_count,
-      /*bucket=*/REMOTE_NON_INITIAL_UPDATE, histogram_);
-
   // Mark the current state of the counters as uploaded to UMA.
   emitted_update_counters_ = update_counters_;
 }

components/sync/engine_impl/cycle/data_type_debug_info_emitter.h

@@ -10,10 +10,6 @@
 #include "base/macros.h"
 #include "components/sync/base/model_type.h"
 
-namespace base {
-class HistogramBase;
-}
-
 namespace syncer {
 
 // A class to maintain counts related to sync commit requests and responses.
@@ -50,6 +46,7 @@ struct UpdateCounters {
 // is delegated to the UpdateHandler and CommitContributors. For the Stats
 // counters, the emitter will let sub class to fetch all the required
 // information on demand.
+// TODO(crbug.com/1137896): This class is unused, remove it.
 class DataTypeDebugInfoEmitter {
  public:
   explicit DataTypeDebugInfoEmitter(ModelType type);
@@ -85,10 +82,6 @@ class DataTypeDebugInfoEmitter {
   CommitCounters emitted_commit_counters_;
   UpdateCounters emitted_update_counters_;
 
-  // The histogram to record to; cached for efficiency because many histogram
-  // entries are recorded in this object during run-time.
-  base::HistogramBase* const histogram_;
-
   DISALLOW_COPY_AND_ASSIGN(DataTypeDebugInfoEmitter);
 };
 

components/sync/engine_impl/cycle/data_type_debug_info_emitter_unittest.cc

-// Copyright 2018 The Chromium Authors. All rights reserved.
-// Use of this source code is governed by a BSD-style license that can be
-// found in the LICENSE file.
-
-#include "components/sync/engine_impl/cycle/data_type_debug_info_emitter.h"
-
-#include "base/test/metrics/histogram_tester.h"
-#include "testing/gtest/include/gtest/gtest.h"
-
-namespace syncer {
-namespace {
-
-TEST(DataTypeDebugInfoEmitterTest, ShouldEmitCommitsToUMAIfChanged) {
-  DataTypeDebugInfoEmitter emitter(BOOKMARKS);
-
-  CommitCounters* counters = emitter.GetMutableCommitCounters();
-  counters->num_deletion_commits_attempted += 3;
-  counters->num_creation_commits_attempted += 2;
-  counters->num_update_commits_attempted += 1;
-
-  base::HistogramTester histogram_tester;
-  emitter.EmitCommitCountersUpdate();
-  EXPECT_EQ(
-      3, histogram_tester.GetBucketCount("Sync.ModelTypeEntityChange3.BOOKMARK",
-                                         /*LOCAL_DELETION=*/0));
-  EXPECT_EQ(
-      2, histogram_tester.GetBucketCount("Sync.ModelTypeEntityChange3.BOOKMARK",
-                                         /*LOCAL_CREATION=*/1));
-  EXPECT_EQ(1, histogram_tester.GetBucketCount(
-                   "Sync.ModelTypeEntityChange3.BOOKMARK", /*LOCAL_UPDATE=*/2));
-}
-
-TEST(DataTypeDebugInfoEmitterTest, ShouldNotEmitCommitsToUMAIfNotChanged) {
-  DataTypeDebugInfoEmitter emitter(BOOKMARKS);
-
-  base::HistogramTester histogram_tester;
-  emitter.EmitCommitCountersUpdate();
-  histogram_tester.ExpectTotalCount("Sync.ModelTypeEntityChange3.BOOKMARK", 0);
-}
-
-// Tests that at each EmitCommitCountersUpdate() call, only the changes since
-// the last call to EmitCommitCountersUpdate() are reported to UMA.
-TEST(DataTypeDebugInfoEmitterTest, ShouldEmitCommitsToUMAIncrementally) {
-  DataTypeDebugInfoEmitter emitter(BOOKMARKS);
-
-  CommitCounters* counters = emitter.GetMutableCommitCounters();
-  counters->num_deletion_commits_attempted += 3;
-  counters->num_creation_commits_attempted += 2;
-  counters->num_update_commits_attempted += 1;
-
-  // First emission - tested in the test above.
-  emitter.EmitCommitCountersUpdate();
-
-  counters = emitter.GetMutableCommitCounters();
-  counters->num_deletion_commits_attempted += 1;
-  counters->num_creation_commits_attempted += 2;
-  counters->num_update_commits_attempted += 3;
-
-  // Test the second emission that it only reports the increment in counters.
-  base::HistogramTester histogram_tester;
-  emitter.EmitCommitCountersUpdate();
-  EXPECT_EQ(
-      1, histogram_tester.GetBucketCount("Sync.ModelTypeEntityChange3.BOOKMARK",
-                                         /*LOCAL_DELETION=*/0));
-  EXPECT_EQ(
-      2, histogram_tester.GetBucketCount("Sync.ModelTypeEntityChange3.BOOKMARK",
-                                         /*LOCAL_CREATION=*/1));
-  EXPECT_EQ(3, histogram_tester.GetBucketCount(
-                   "Sync.ModelTypeEntityChange3.BOOKMARK", /*LOCAL_UPDATE=*/2));
-}
-
-TEST(DataTypeDebugInfoEmitterTest, ShouldEmitUpdatesToUMAIfChanged) {
-  DataTypeDebugInfoEmitter emitter(BOOKMARKS);
-
-  UpdateCounters* counters = emitter.GetMutableUpdateCounters();
-  counters->num_initial_updates_received += 5;
-  counters->num_non_initial_updates_received += 3;
-  counters->num_non_initial_tombstone_updates_received += 1;
-
-  base::HistogramTester histogram_tester;
-  emitter.EmitUpdateCountersUpdate();
-  EXPECT_EQ(
-      1, histogram_tester.GetBucketCount("Sync.ModelTypeEntityChange3.BOOKMARK",
-                                         /*REMOTE_DELETION=*/3));
-  EXPECT_EQ(
-      2, histogram_tester.GetBucketCount("Sync.ModelTypeEntityChange3.BOOKMARK",
-                                         /*REMOTE_NON_INITIAL_UPDATE=*/4));
-  EXPECT_EQ(
-      5, histogram_tester.GetBucketCount("Sync.ModelTypeEntityChange3.BOOKMARK",
-                                         /*REMOTE_INITIAL_UPDATE=*/5));
-}
-
-TEST(DataTypeDebugInfoEmitterTest, ShouldNotEmitUpdatesToUMAIfNotChanged) {
-  DataTypeDebugInfoEmitter emitter(BOOKMARKS);
-
-  base::HistogramTester histogram_tester;
-  emitter.EmitUpdateCountersUpdate();
-  histogram_tester.ExpectTotalCount("Sync.ModelTypeEntityChange3.BOOKMARK", 0);
-}
-
-// Tests that at each EmitUpdateCountersUpdate() call, only the changes since
-// the last call to EmitUpdateCountersUpdate() are reported to UMA.
-TEST(DataTypeDebugInfoEmitterTest, ShouldEmitUpdatesToUMAIncrementally) {
-  DataTypeDebugInfoEmitter emitter(BOOKMARKS);
-
-  UpdateCounters* counters = emitter.GetMutableUpdateCounters();
-  counters->num_initial_updates_received += 5;
-  counters->num_non_initial_updates_received += 3;
-  counters->num_non_initial_tombstone_updates_received += 1;
-
-  // First emission - tested in the test above.
-  emitter.EmitUpdateCountersUpdate();
-
-  counters = emitter.GetMutableUpdateCounters();
-  counters->num_initial_updates_received += 4;
-  counters->num_non_initial_updates_received += 3;
-  counters->num_non_initial_tombstone_updates_received += 2;
-
-  // Test the second emission that it only reports the increment in counters.
-  base::HistogramTester histogram_tester;
-  emitter.EmitUpdateCountersUpdate();
-  EXPECT_EQ(
-      2, histogram_tester.GetBucketCount("Sync.ModelTypeEntityChange3.BOOKMARK",
-                                         /*REMOTE_DELETION=*/3));
-  EXPECT_EQ(
-      1, histogram_tester.GetBucketCount("Sync.ModelTypeEntityChange3.BOOKMARK",
-                                         /*REMOTE_NON_INITIAL_UPDATE=*/4));
-  EXPECT_EQ(
-      4, histogram_tester.GetBucketCount("Sync.ModelTypeEntityChange3.BOOKMARK",
-                                         /*REMOTE_INITIAL_UPDATE=*/5));
-}
-
-}  // namespace
-}  // namespace syncer

components/sync/engine_impl/cycle/entity_change_metric_recording.cc

+// Copyright 2020 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "components/sync/engine_impl/cycle/entity_change_metric_recording.h"
+
+#include "base/metrics/histogram_functions.h"
+
+namespace syncer {
+
+namespace {
+
+const char kEntityChangeHistogramPrefix[] = "Sync.ModelTypeEntityChange3.";
+
+}  // namespace
+
+void RecordEntityChangeMetrics(ModelType type, ModelTypeEntityChange change) {
+  std::string histogram_name = std::string(kEntityChangeHistogramPrefix) +
+                               ModelTypeToHistogramSuffix(type);
+  base::UmaHistogramEnumeration(histogram_name, change);
+}
+
+std::string GetEntityChangeHistogramNameForTest(ModelType type) {
+  return std::string(kEntityChangeHistogramPrefix) +
+         ModelTypeToHistogramSuffix(type);
+}
+
+}  // namespace syncer

components/sync/engine_impl/cycle/entity_change_metric_recording.h

+// Copyright 2020 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef COMPONENTS_SYNC_ENGINE_IMPL_CYCLE_ENTITY_CHANGE_METRIC_RECORDING_H_
+#define COMPONENTS_SYNC_ENGINE_IMPL_CYCLE_ENTITY_CHANGE_METRIC_RECORDING_H_
+
+#include <string>
+
+#include "components/sync/base/model_type.h"
+
+namespace syncer {
+
+// These values are persisted to logs. Entries should not be renumbered and
+// numeric values should never be reused.
+enum class ModelTypeEntityChange {
+  kLocalDeletion = 0,
+  kLocalCreation = 1,
+  kLocalUpdate = 2,
+  kRemoteDeletion = 3,
+  kRemoteNonInitialUpdate = 4,
+  kRemoteInitialUpdate = 5,
+  kMaxValue = kRemoteInitialUpdate,
+};
+
+void RecordEntityChangeMetrics(ModelType type, ModelTypeEntityChange change);
+
+std::string GetEntityChangeHistogramNameForTest(ModelType type);
+
+}  // namespace syncer
+
+#endif  // COMPONENTS_SYNC_ENGINE_IMPL_CYCLE_ENTITY_CHANGE_METRIC_RECORDING_H_

components/sync/engine_impl/model_type_registry.cc

@@ -76,22 +76,13 @@ void ModelTypeRegistry::ConnectDataType(
   if (encrypted_types_.Has(type))
     cryptographer_copy = cryptographer_->Clone();
 
-  DataTypeDebugInfoEmitter* emitter = GetEmitter(type);
-  if (emitter == nullptr) {
-    auto new_emitter = std::make_unique<DataTypeDebugInfoEmitter>(type);
-    emitter = new_emitter.get();
-    data_type_debug_info_emitter_map_.insert(
-        std::make_pair(type, std::move(new_emitter)));
-  }
-
   bool initial_sync_done =
       activation_response->model_type_state.initial_sync_done();
   auto worker = std::make_unique<ModelTypeWorker>(
       type, activation_response->model_type_state,
       /*trigger_initial_sync=*/!initial_sync_done,
       std::move(cryptographer_copy), passphrase_type_, nudge_handler_,
-      std::move(activation_response->type_processor), emitter,
-      cancelation_signal_);
+      std::move(activation_response->type_processor), cancelation_signal_);
 
   // Save a raw pointer and add the worker to our structures.
   ModelTypeWorker* worker_ptr = worker.get();

components/sync/engine_impl/model_type_worker.cc

@@ -30,6 +30,7 @@
 #include "components/sync/engine_impl/bookmark_update_preprocessing.h"
 #include "components/sync/engine_impl/commit_contribution.h"
 #include "components/sync/engine_impl/commit_contribution_impl.h"
+#include "components/sync/engine_impl/cycle/entity_change_metric_recording.h"
 #include "components/sync/protocol/proto_memory_estimations.h"
 
 namespace syncer {
@@ -73,10 +74,8 @@ ModelTypeWorker::ModelTypeWorker(
     PassphraseType passphrase_type,
     NudgeHandler* nudge_handler,
     std::unique_ptr<ModelTypeProcessor> model_type_processor,
-    DataTypeDebugInfoEmitter* debug_info_emitter,
     CancelationSignal* cancelation_signal)
     : type_(type),
-      debug_info_emitter_(debug_info_emitter),
       model_type_state_(initial_state),
       model_type_processor_(std::move(model_type_processor)),
       cryptographer_(std::move(cryptographer)),
@@ -188,21 +187,21 @@ SyncerError ModelTypeWorker::ProcessGetUpdatesResponse(
   *model_type_state_.mutable_type_context() = mutated_context;
   *model_type_state_.mutable_progress_marker() = progress_marker;
 
-  UpdateCounters* counters = debug_info_emitter_->GetMutableUpdateCounters();
-
-  if (!from_uss_migrator) {
-    if (is_initial_sync) {
-      counters->num_initial_updates_received += applicable_updates.size();
-    } else {
-      counters->num_non_initial_updates_received += applicable_updates.size();
+  for (const sync_pb::SyncEntity* update_entity : applicable_updates) {
+    // TODO(crbug.com/1137896): The USS migrator doesn't exist anymore, remove
+    // this parameter.
+    if (!from_uss_migrator) {
+      RecordEntityChangeMetrics(
+          type_, is_initial_sync
+                     ? ModelTypeEntityChange::kRemoteInitialUpdate
+                     : ModelTypeEntityChange::kRemoteNonInitialUpdate);
     }
-  }
 
-  for (const sync_pb::SyncEntity* update_entity : applicable_updates) {
     if (update_entity->deleted()) {
       status->increment_num_tombstone_updates_downloaded_by(1);
       if (!is_initial_sync) {
-        ++counters->num_non_initial_tombstone_updates_received;
+        RecordEntityChangeMetrics(type_,
+                                  ModelTypeEntityChange::kRemoteDeletion);
       }
     }
 
@@ -227,7 +226,6 @@ SyncerError ModelTypeWorker::ProcessGetUpdatesResponse(
     }
   }
 
-  debug_info_emitter_->EmitUpdateCountersUpdate();
   return SyncerError(SyncerError::SYNCER_OK);
 }
 
@@ -425,7 +423,7 @@ std::unique_ptr<CommitContribution> ModelTypeWorker::GetContribution(
                      weak_ptr_factory_.GetWeakPtr()),
       base::BindOnce(&ModelTypeWorker::OnFullCommitFailure,
                      weak_ptr_factory_.GetWeakPtr()),
-      cryptographer_.get(), passphrase_type_, debug_info_emitter_,
+      cryptographer_.get(), passphrase_type_,
       CommitOnlyTypes().Has(GetModelType()));
 }
 

components/sync/engine_impl/model_type_worker.h

@@ -23,7 +23,6 @@
 #include "components/sync/engine/commit_queue.h"
 #include "components/sync/engine/sync_encryption_handler.h"
 #include "components/sync/engine_impl/commit_contributor.h"
-#include "components/sync/engine_impl/cycle/data_type_debug_info_emitter.h"
 #include "components/sync/engine_impl/nudge_handler.h"
 #include "components/sync/engine_impl/update_handler.h"
 #include "components/sync/nigori/cryptographer.h"
@@ -67,7 +66,6 @@ class ModelTypeWorker : public UpdateHandler,
                   PassphraseType passphrase_type,
                   NudgeHandler* nudge_handler,
                   std::unique_ptr<ModelTypeProcessor> model_type_processor,
-                  DataTypeDebugInfoEmitter* debug_info_emitter,
                   CancelationSignal* cancelation_signal);
   ~ModelTypeWorker() override;
 
@@ -210,7 +208,6 @@ class ModelTypeWorker : public UpdateHandler,
   void OnFullCommitFailure(SyncCommitError commit_error);
 
   ModelType type_;
-  DataTypeDebugInfoEmitter* debug_info_emitter_;
 
   // State that applies to the entire model type.
   sync_pb::ModelTypeState model_type_state_;

components/sync/engine_impl/model_type_worker_unittest.cc

@@ -19,7 +19,7 @@
 #include "components/sync/base/unique_position.h"
 #include "components/sync/engine/model_type_processor.h"
 #include "components/sync/engine_impl/commit_contribution.h"
-#include "components/sync/engine_impl/cycle/data_type_debug_info_emitter.h"
+#include "components/sync/engine_impl/cycle/entity_change_metric_recording.h"
 #include "components/sync/engine_impl/cycle/status_controller.h"
 #include "components/sync/nigori/cryptographer_impl.h"
 #include "components/sync/nigori/nigori.h"
@@ -90,15 +90,6 @@ sync_pb::EntitySpecifics EncryptPasswordSpecifics(
   return encrypted_specifics;
 }
 
-void VerifyCommitCount(const DataTypeDebugInfoEmitter* emitter,
-                       int expected_creation_count,
-                       int expected_deletion_count) {
-  EXPECT_EQ(expected_creation_count,
-            emitter->GetCommitCounters().num_creation_commits_attempted);
-  EXPECT_EQ(expected_deletion_count,
-            emitter->GetCommitCounters().num_deletion_commits_attempted);
-}
-
 }  // namespace
 
 // Tests the ModelTypeWorker.
@@ -147,8 +138,7 @@ class ModelTypeWorkerTest : public ::testing::Test {
         update_encryption_filter_index_(0),
         mock_type_processor_(nullptr),
         mock_server_(std::make_unique<SingleTypeMockServer>(model_type)),
-        is_processor_disconnected_(false),
-        emitter_(std::make_unique<DataTypeDebugInfoEmitter>(model_type)) {}
+        is_processor_disconnected_(false) {}
 
   ~ModelTypeWorkerTest() override {}
 
@@ -182,9 +172,8 @@ class ModelTypeWorkerTest : public ::testing::Test {
     nudge_handler()->ClearCounters();
   }
 
-  void InitializeCommitOnly() {
-    mock_server_ = std::make_unique<SingleTypeMockServer>(USER_EVENTS);
-    emitter_ = std::make_unique<DataTypeDebugInfoEmitter>(USER_EVENTS);
+  void InitializeCommitOnly(ModelType model_type) {
+    mock_server_ = std::make_unique<SingleTypeMockServer>(model_type);
 
     // Don't set progress marker, commit only types don't use them.
     ModelTypeState initial_state;
@@ -211,7 +200,7 @@ class ModelTypeWorkerTest : public ::testing::Test {
     worker_ = std::make_unique<ModelTypeWorker>(
         type, state, !state.initial_sync_done(), std::move(cryptographer_copy),
         PassphraseType::kImplicitPassphrase, &mock_nudge_handler_,
-        std::move(processor), emitter_.get(), &cancelation_signal_);
+        std::move(processor), &cancelation_signal_);
   }
 
   void InitializeCryptographer() {
@@ -467,7 +456,6 @@ class ModelTypeWorkerTest : public ::testing::Test {
   MockModelTypeProcessor* processor() { return mock_type_processor_; }
   ModelTypeWorker* worker() { return worker_.get(); }
   SingleTypeMockServer* server() { return mock_server_.get(); }
-  DataTypeDebugInfoEmitter* emitter() { return emitter_.get(); }
   MockNudgeHandler* nudge_handler() { return &mock_nudge_handler_; }
   StatusController* status_controller() { return &status_controller_; }
 
@@ -505,8 +493,6 @@ class ModelTypeWorkerTest : public ::testing::Test {
 
   bool is_processor_disconnected_;
 
-  std::unique_ptr<DataTypeDebugInfoEmitter> emitter_;
-
   StatusController status_controller_;
 };
 
@@ -519,14 +505,19 @@ class ModelTypeWorkerTest : public ::testing::Test {
 // values. It makes sense to have one or two tests that are this thorough, but
 // we shouldn't be this verbose in all tests.
 TEST_F(ModelTypeWorkerTest, SimpleCommit) {
+  base::HistogramTester histogram_tester;
   NormalInitialize();
 
   EXPECT_EQ(0, nudge_handler()->GetNumCommitNudges());
   EXPECT_EQ(nullptr, worker()->GetContribution(INT_MAX));
   EXPECT_EQ(0U, server()->GetNumCommitMessages());
   EXPECT_EQ(0U, processor()->GetNumCommitResponses());
-  VerifyCommitCount(emitter(), /*expected_creation_count=*/0,
-                    /*expected_deletion_count=*/0);
+  histogram_tester.ExpectBucketCount(
+      GetEntityChangeHistogramNameForTest(worker()->GetModelType()),
+      ModelTypeEntityChange::kLocalCreation, 0);
+  histogram_tester.ExpectBucketCount(
+      GetEntityChangeHistogramNameForTest(worker()->GetModelType()),
+      ModelTypeEntityChange::kLocalDeletion, 0);
 
   worker()->NudgeForCommit();
   EXPECT_EQ(1, nudge_handler()->GetNumCommitNudges());
@@ -551,8 +542,12 @@ TEST_F(ModelTypeWorkerTest, SimpleCommit) {
   EXPECT_FALSE(entity.deleted());
   EXPECT_EQ(kValue1, entity.specifics().preference().value());
 
-  VerifyCommitCount(emitter(), /*expected_creation_count=*/1,
-                    /*expected_deletion_count=*/0);
+  histogram_tester.ExpectBucketCount(
+      GetEntityChangeHistogramNameForTest(worker()->GetModelType()),
+      ModelTypeEntityChange::kLocalCreation, 1);
+  histogram_tester.ExpectBucketCount(
+      GetEntityChangeHistogramNameForTest(worker()->GetModelType()),
+      ModelTypeEntityChange::kLocalDeletion, 0);
 
   // Exhaustively verify the commit response returned to the model thread.
   ASSERT_EQ(0U, processor()->GetNumCommitFailures());
@@ -573,17 +568,26 @@ TEST_F(ModelTypeWorkerTest, SimpleCommit) {
 }
 
 TEST_F(ModelTypeWorkerTest, SimpleDelete) {
+  base::HistogramTester histogram_tester;
   NormalInitialize();
 
   // We can't delete an entity that was never committed.
   // Step 1 is to create and commit a new entity.
-  VerifyCommitCount(emitter(), /*expected_creation_count=*/0,
-                    /*expected_deletion_count=*/0);
+  histogram_tester.ExpectBucketCount(
+      GetEntityChangeHistogramNameForTest(worker()->GetModelType()),
+      ModelTypeEntityChange::kLocalCreation, 0);
+  histogram_tester.ExpectBucketCount(
+      GetEntityChangeHistogramNameForTest(worker()->GetModelType()),
+      ModelTypeEntityChange::kLocalDeletion, 0);
   processor()->SetCommitRequest(GenerateCommitRequest(kTag1, kValue1));
   DoSuccessfulCommit();
 
-  VerifyCommitCount(emitter(), /*expected_creation_count=*/1,
-                    /*expected_deletion_count=*/0);
+  histogram_tester.ExpectBucketCount(
+      GetEntityChangeHistogramNameForTest(worker()->GetModelType()),
+      ModelTypeEntityChange::kLocalCreation, 1);
+  histogram_tester.ExpectBucketCount(
+      GetEntityChangeHistogramNameForTest(worker()->GetModelType()),
+      ModelTypeEntityChange::kLocalDeletion, 0);
 
   ASSERT_TRUE(processor()->HasCommitResponse(kHash1));
   const CommitResponseData& initial_commit_response =
@@ -594,8 +598,12 @@ TEST_F(ModelTypeWorkerTest, SimpleDelete) {
   processor()->SetCommitRequest(GenerateDeleteRequest(kTag1));
   DoSuccessfulCommit();
 
-  VerifyCommitCount(emitter(), /*expected_creation_count=*/1,
-                    /*expected_deletion_count=*/1);
+  histogram_tester.ExpectBucketCount(
+      GetEntityChangeHistogramNameForTest(worker()->GetModelType()),
+      ModelTypeEntityChange::kLocalCreation, 1);
+  histogram_tester.ExpectBucketCount(
+      GetEntityChangeHistogramNameForTest(worker()->GetModelType()),
+      ModelTypeEntityChange::kLocalDeletion, 1);
 
   // Verify the SyncEntity sent in the commit message.
   ASSERT_EQ(2U, server()->GetNumCommitMessages());
@@ -684,9 +692,12 @@ TEST_F(ModelTypeWorkerTest, TwoNewItemsCommittedSeparately) {
 
 // Test normal update receipt code path.
 TEST_F(ModelTypeWorkerTest, ReceiveUpdates) {
+  base::HistogramTester histogram_tester;
   NormalInitialize();
 
-  EXPECT_EQ(0, emitter()->GetUpdateCounters().num_non_initial_updates_received);
+  histogram_tester.ExpectBucketCount(
+      GetEntityChangeHistogramNameForTest(worker()->GetModelType()),
+      ModelTypeEntityChange::kRemoteNonInitialUpdate, 0);
 
   const ClientTagHash tag_hash = GenerateTagHash(kTag1);
 
@@ -711,7 +722,9 @@ TEST_F(ModelTypeWorkerTest, ReceiveUpdates) {
   EXPECT_EQ(kTag1, entity.specifics.preference().name());
   EXPECT_EQ(kValue1, entity.specifics.preference().value());
 
-  EXPECT_EQ(1, emitter()->GetUpdateCounters().num_non_initial_updates_received);
+  histogram_tester.ExpectBucketCount(
+      GetEntityChangeHistogramNameForTest(worker()->GetModelType()),
+      ModelTypeEntityChange::kRemoteNonInitialUpdate, 1);
 }
 
 TEST_F(ModelTypeWorkerTest, ReceiveUpdates_NoDuplicateHash) {
@@ -1338,7 +1351,9 @@ TEST_F(ModelTypeWorkerTest, RecreateDeletedEntity) {
 }
 
 TEST_F(ModelTypeWorkerTest, CommitOnly) {
-  InitializeCommitOnly();
+  base::HistogramTester histogram_tester;
+  ModelType model_type = USER_EVENTS;
+  InitializeCommitOnly(model_type);
 
   int id = 123456789;
   EntitySpecifics specifics;
@@ -1361,8 +1376,12 @@ TEST_F(ModelTypeWorkerTest, CommitOnly) {
   EXPECT_TRUE(entity.specifics().has_user_event());
   EXPECT_EQ(id, entity.specifics().user_event().event_time_usec());
 
-  VerifyCommitCount(emitter(), /*expected_creation_count=*/1,
-                    /*expected_deletion_count=*/0);
+  histogram_tester.ExpectBucketCount(
+      GetEntityChangeHistogramNameForTest(model_type),
+      ModelTypeEntityChange::kLocalCreation, 1);
+  histogram_tester.ExpectBucketCount(
+      GetEntityChangeHistogramNameForTest(model_type),
+      ModelTypeEntityChange::kLocalDeletion, 0);
 
   ASSERT_EQ(1U, processor()->GetNumCommitResponses());
   EXPECT_EQ(1U, processor()->GetNthCommitResponse(0).size());