Allow UKM sampling experiment to enable everything.

Change-Id: I1c11dff04f9a60e21756964b16bfa77007330c68
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3268486
Reviewed-by: Robert Kaplow <[email protected]>
Commit-Queue: Brian White <[email protected]>
Cr-Commit-Position: refs/heads/main@{#940923}

chrome/browser/metrics/ukm_browsertest.cc

@@ -1444,7 +1444,7 @@ IN_PROC_BROWSER_TEST_F(UkmBrowserTest, DebugUiRenders) {
   PlatformBrowser browser = CreatePlatformBrowser(profile);
 
   ukm::UkmService* ukm_service(GetUkmService());
-  EXPECT_TRUE(ukm_service->IsSamplingEnabled());
+  EXPECT_TRUE(ukm_service->IsSamplingConfigured());
 
   // chrome://ukm
   const GURL debug_url(content::GetWebUIURLString(content::kChromeUIUkmHost));

components/ukm/debug/ukm_debug_data_extractor.cc

@@ -94,7 +94,7 @@ base::Value UkmDebugDataExtractor::GetStructuredData(
 
   ukm_data.SetKey(
       "is_sampling_enabled",
-      base::Value(static_cast<bool>(ukm_service->IsSamplingEnabled())));
+      base::Value(static_cast<bool>(ukm_service->IsSamplingConfigured())));
 
   std::map<SourceId, SourceData> source_data;
   for (const auto& kv : ukm_service->recordings_.sources) {

components/ukm/test_ukm_recorder.cc

@@ -39,7 +39,7 @@ void MergeEntry(const mojom::UkmEntry* in, mojom::UkmEntry* out) {
 TestUkmRecorder::TestUkmRecorder() {
   EnableRecording(/*extensions=*/true);
   StoreWhitelistedEntries();
-  DisableSamplingForTesting();
+  SetSamplingForTesting(1);  // 1-in-1 == unsampled
 }
 
 TestUkmRecorder::~TestUkmRecorder() {}

components/ukm/ukm_recorder_impl.cc

@@ -86,6 +86,7 @@ enum class DroppedDataReason {
   NOT_MATCHED = 8,
   EMPTY_URL = 9,
   REJECTED_BY_FILTER = 10,
+  SAMPLING_UNCONFIGURED = 11,
   NUM_DROPPED_DATA_REASONS
 };
 
@@ -247,12 +248,14 @@ void UkmRecorderImpl::DisableRecording() {
   extensions_enabled_ = false;
 }
 
-void UkmRecorderImpl::DisableSamplingForTesting() {
-  sampling_enabled_ = false;
+void UkmRecorderImpl::SetSamplingForTesting(int rate) {
+  sampling_forced_for_testing_ = true;
+  default_sampling_rate_ = rate;
+  event_sampling_rates_.clear();
 }
 
-bool UkmRecorderImpl::IsSamplingEnabled() const {
-  return sampling_enabled_ &&
+bool UkmRecorderImpl::IsSamplingConfigured() const {
+  return sampling_forced_for_testing_ ||
          base::FeatureList::IsEnabled(kUkmSamplingRateFeature);
 }
 
@@ -405,6 +408,8 @@ void UkmRecorderImpl::StoreRecordingsInReport(Report* report) {
         event_aggregate.dropped_due_to_whitelist);
     proto_aggregate->set_dropped_due_to_filter(
         event_aggregate.dropped_due_to_filter);
+    proto_aggregate->set_dropped_due_to_unconfigured(
+        event_aggregate.dropped_due_to_unconfigured);
     for (const auto& metric_and_aggregate : event_aggregate.metrics) {
       const MetricAggregate& aggregate = metric_and_aggregate.second;
       Aggregate::Metric* proto_metric = proto_aggregate->add_metrics();
@@ -434,6 +439,11 @@ void UkmRecorderImpl::StoreRecordingsInReport(Report* report) {
         proto_metric->set_dropped_due_to_filter(
             aggregate.dropped_due_to_filter);
       }
+      if (aggregate.dropped_due_to_unconfigured !=
+          event_aggregate.dropped_due_to_unconfigured) {
+        proto_metric->set_dropped_due_to_unconfigured(
+            aggregate.dropped_due_to_unconfigured);
+      }
     }
   }
   int num_serialized_sources = 0;
@@ -727,6 +737,15 @@ void UkmRecorderImpl::AddEntry(mojom::UkmEntryPtr entry) {
     aggregate.value_square_sum += value * value;
   }
 
+  if (!IsSamplingConfigured()) {
+    RecordDroppedEntry(entry->event_hash,
+                       DroppedDataReason::SAMPLING_UNCONFIGURED);
+    event_aggregate.dropped_due_to_unconfigured++;
+    for (auto& metric : entry->metrics)
+      event_aggregate.metrics[metric.first].dropped_due_to_unconfigured++;
+    return;
+  }
+
   if (ShouldRestrictToWhitelistedEntries() &&
       !base::Contains(whitelisted_entry_hashes_, entry->event_hash)) {
     RecordDroppedEntry(entry->event_hash, DroppedDataReason::NOT_WHITELISTED);
@@ -736,20 +755,18 @@ void UkmRecorderImpl::AddEntry(mojom::UkmEntryPtr entry) {
     return;
   }
 
-  if (IsSamplingEnabled()) {
-    if (default_sampling_rate_ < 0) {
-      LoadExperimentSamplingInfo();
-    }
+  if (default_sampling_rate_ < 0) {
+    LoadExperimentSamplingInfo();
+  }
 
-    bool sampled_in = IsSampledIn(entry->source_id, entry->event_hash);
+  bool sampled_in = IsSampledIn(entry->source_id, entry->event_hash);
 
-    if (!sampled_in) {
-      RecordDroppedEntry(entry->event_hash, DroppedDataReason::SAMPLED_OUT);
-      event_aggregate.dropped_due_to_sampling++;
-      for (auto& metric : entry->metrics)
-        event_aggregate.metrics[metric.first].dropped_due_to_sampling++;
-      return;
-    }
+  if (!sampled_in) {
+    RecordDroppedEntry(entry->event_hash, DroppedDataReason::SAMPLED_OUT);
+    event_aggregate.dropped_due_to_sampling++;
+    for (auto& metric : entry->metrics)
+      event_aggregate.metrics[metric.first].dropped_due_to_sampling++;
+    return;
   }
 
   if (recordings_.entries.size() >= max_entries_) {

components/ukm/ukm_recorder_impl.h

@@ -62,11 +62,11 @@ class COMPONENT_EXPORT(UKM_RECORDER) UkmRecorderImpl : public UkmRecorder {
   void EnableRecording(bool extensions);
   void DisableRecording();
 
-  // Disables sampling for testing purposes.
-  void DisableSamplingForTesting() override;
+  // Controls sampling for testing purposes. Sampling is 1-in-N (N==rate).
+  void SetSamplingForTesting(int rate) override;
 
-  // True if sampling is enabled.
-  bool IsSamplingEnabled() const;
+  // True if sampling has been configured.
+  bool IsSamplingConfigured() const;
 
   // Deletes all stored recordings.
   void Purge();
@@ -170,6 +170,7 @@ class COMPONENT_EXPORT(UKM_RECORDER) UkmRecorderImpl : public UkmRecorder {
     uint64_t dropped_due_to_sampling = 0;
     uint64_t dropped_due_to_whitelist = 0;
     uint64_t dropped_due_to_filter = 0;
+    uint64_t dropped_due_to_unconfigured = 0;
   };
 
   struct EventAggregate {
@@ -182,6 +183,7 @@ class COMPONENT_EXPORT(UKM_RECORDER) UkmRecorderImpl : public UkmRecorder {
     uint64_t dropped_due_to_sampling = 0;
     uint64_t dropped_due_to_whitelist = 0;
     uint64_t dropped_due_to_filter = 0;
+    uint64_t dropped_due_to_unconfigured = 0;
   };
 
   using MetricAggregateMap = std::map<uint64_t, MetricAggregate>;
@@ -211,8 +213,8 @@ class COMPONENT_EXPORT(UKM_RECORDER) UkmRecorderImpl : public UkmRecorder {
   // Indicates whether recording continuity has been broken since last report.
   bool recording_is_continuous_ = true;
 
-  // Indicates if sampling has been enabled.
-  bool sampling_enabled_ = true;
+  // Indicates if sampling has been forced for testing.
+  bool sampling_forced_for_testing_ = false;
 
   // A pseudo-random number used as the base for sampling choices. This
   // allows consistent "is sampled in" results for a given source and event

components/ukm/ukm_service_unittest.cc

@@ -71,7 +71,7 @@ SourceId ConvertSourceIdToWhitelistedType(SourceId id, SourceIdType type) {
 class TestRecordingHelper {
  public:
   explicit TestRecordingHelper(UkmRecorder* recorder) : recorder_(recorder) {
-    recorder_->DisableSamplingForTesting();
+    recorder_->SetSamplingForTesting(1);
   }
 
   TestRecordingHelper(const TestRecordingHelper&) = delete;

services/metrics/public/cpp/ukm_recorder.h

@@ -79,8 +79,8 @@ class METRICS_EXPORT UkmRecorder {
   // Add an entry to the UkmEntry list.
   virtual void AddEntry(mojom::UkmEntryPtr entry) = 0;
 
-  // Disables sampling for testing purposes.
-  virtual void DisableSamplingForTesting() {}
+  // Controls sampling for testing purposes. Sampling is 1-in-N (N==rate).
+  virtual void SetSamplingForTesting(int rate) {}
 
  protected:
   // Type-safe wrappers for Update<X> functions.

third_party/metrics_proto/ukm/aggregate.proto

@@ -9,7 +9,7 @@ option java_package = "org.chromium.components.metrics";
 
 package ukm;
 
-// Next tag: 9
+// Next tag: 10
 message Aggregate {
   // The id of the Source this Event is associated with. If this is zero
   // then it's an aggregate across all sources.
@@ -31,8 +31,17 @@ message Aggregate {
   optional uint64 dropped_due_to_whitelist = 7;
 
   // The total number of times this source/event was dropped due to filter.
+  // Unlike the others, records dropped for this reason are not included
+  // in the |total_count|.
+  // https://source.chromium.org/chromium/chromium/src/+/main:components/ukm/ukm_entry_filter.h
   optional uint64 dropped_due_to_filter = 8;
 
+  // The total number of times this source/event was dropped due to there
+  // being no configuration of UKM via Finch. Until a full configuration is
+  // delivered by VariationService to the client, UKM is disabled but some
+  // basic information is still collected and counted here.
+  optional uint64 dropped_due_to_unconfigured = 9;
+
   // For each Event, we have a list of possible metrics included. Metric names
   // cannot be repeated. There is no guarentee that all metrics that are
   // possible for a given event will be included in a single Aggregate.
@@ -52,6 +61,7 @@ message Aggregate {
     optional uint64 dropped_due_to_sampling = 6;
     optional uint64 dropped_due_to_whitelist = 7;
     optional uint64 dropped_due_to_filter = 8;
+    optional uint64 dropped_due_to_unconfigured = 9;
   }
   repeated Metric metrics = 6;
 }

tools/metrics/histograms/enums.xml

@@ -85700,6 +85700,7 @@ Full version information for the fingerprint enum values:
   <int value="8" label="Not matched"/>
   <int value="9" label="Empty URL"/>
   <int value="10" label="Rejected by filter"/>
+  <int value="11" label="No UKM sampling configuration received"/>
 </enum>
 
 <enum name="UkmEventNameHash">