Use process unique bits in Ukm SourceIds.

Change-Id: I11373644959fff8cf98f82912e49d8663f7f5abe
Reviewed-on: https://chromium-review.googlesource.com/1006364Reviewed-by: Robert Kaplow <rkaplow@chromium.org>
Commit-Queue: Steven Holte <holte@chromium.org>
Cr-Commit-Position: refs/heads/master@{#550808}

services/metrics/public/cpp/BUILD.gn

@@ -42,6 +42,7 @@ source_set("tests") {
 
   sources = [
     "metrics_utils_unittest.cc",
+    "ukm_source_id_unittest.cc",
   ]
 
   deps = [

services/metrics/public/cpp/ukm_source_id.cc

@@ -6,6 +6,7 @@
 
 #include "base/atomic_sequence_num.h"
 #include "base/logging.h"
+#include "base/rand_util.h"
 
 namespace ukm {
 
@@ -18,8 +19,17 @@ const int64_t kTypeMask = (INT64_C(1) << kNumTypeBits) - 1;
 }  // namespace
 
 SourceId AssignNewSourceId() {
+  // Generate some bits which are unique to this process, so we can generate
+  // IDs independently in different processes. IDs generated by this method may
+  // collide, but it should be sufficiently rare enough to not impact data
+  // quality.
+  const static int64_t process_id_bits =
+      static_cast<int64_t>(base::RandUint64()) & ~kLowBitsMask;
+  // Generate some bits which are unique within the process, using a counter.
   static base::AtomicSequenceNumber seq;
-  return ConvertToSourceId(seq.GetNext() + 1, SourceIdType::UKM);
+  SourceId local_id = ConvertToSourceId(seq.GetNext() + 1, SourceIdType::UKM);
+  // Combine the local and process bits to generate a unique ID.
+  return (local_id & kLowBitsMask) | process_id_bits;
 }
 
 SourceId ConvertToSourceId(int64_t other_id, SourceIdType id_type) {
@@ -31,16 +41,6 @@ SourceId ConvertToSourceId(int64_t other_id, SourceIdType id_type) {
   return (other_id << kNumTypeBits) | type_bits;
 }
 
-SourceId ConvertSourceIdFromInstance(int64_t instance_id, int64_t source_id) {
-  // Only convert source IDs from Create().
-  if ((kTypeMask & source_id) != static_cast<int64_t>(SourceIdType::UKM))
-    return source_id;
-
-  // Neither ID should get large enough to cause an issue, but explicitly
-  // discard down to 32 bits anyway.
-  return ((instance_id & kLowBitsMask) << 32) | (source_id & kLowBitsMask);
-}
-
 SourceIdType GetSourceIdType(SourceId source_id) {
   return static_cast<SourceIdType>(source_id & kTypeMask);
 }

services/metrics/public/cpp/ukm_source_id.h

@@ -27,10 +27,6 @@ METRICS_EXPORT SourceId AssignNewSourceId();
 METRICS_EXPORT SourceId ConvertToSourceId(int64_t other_id,
                                           SourceIdType id_type);
 
-// Convert source ids from another process.
-METRICS_EXPORT SourceId ConvertSourceIdFromInstance(int64_t instance_id,
-                                                    int64_t source_id);
-
 // Get the SourceIdType of the SourceId.
 METRICS_EXPORT SourceIdType GetSourceIdType(SourceId source_id);
 

services/metrics/public/cpp/ukm_source_id_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 "services/metrics/public/cpp/ukm_source_id.h"
+
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace ukm {
+
+TEST(UkmSourceIdTest, AssignSourceIds) {
+  const size_t numIds = 5;
+  SourceId ids[numIds] = {};
+
+  for (size_t i = 0; i < numIds; i++) {
+    ids[i] = AssignNewSourceId();
+    EXPECT_NE(kInvalidSourceId, ids[i]);
+    EXPECT_EQ(SourceIdType::UKM, GetSourceIdType(ids[i]));
+    for (size_t j = 0; j < i; j++) {
+      EXPECT_NE(ids[j], ids[i]);
+    }
+  }
+}
+
+TEST(UkmSourceIdTest, ConvertToSourceId) {
+  const size_t numIds = 5;
+  SourceId ids[numIds] = {};
+
+  for (size_t i = 0; i < numIds; i++) {
+    ids[i] = ConvertToSourceId(i, SourceIdType::NAVIGATION_ID);
+    EXPECT_NE(kInvalidSourceId, ids[i]);
+    EXPECT_EQ(SourceIdType::NAVIGATION_ID, GetSourceIdType(ids[i]));
+    for (size_t j = 0; j < i; j++) {
+      EXPECT_NE(ids[j], ids[i]);
+    }
+  }
+}
+
+}  // namespace ukm

services/metrics/ukm_recorder_interface.cc

@@ -12,9 +12,8 @@
 
 namespace metrics {
 
-UkmRecorderInterface::UkmRecorderInterface(ukm::UkmRecorder* ukm_recorder,
-                                           int64_t instance_id)
-    : ukm_recorder_(ukm_recorder), instance_id_(instance_id) {}
+UkmRecorderInterface::UkmRecorderInterface(ukm::UkmRecorder* ukm_recorder)
+    : ukm_recorder_(ukm_recorder) {}
 
 UkmRecorderInterface::~UkmRecorderInterface() = default;
 
@@ -22,22 +21,17 @@ UkmRecorderInterface::~UkmRecorderInterface() = default;
 void UkmRecorderInterface::Create(
     ukm::UkmRecorder* ukm_recorder,
     ukm::mojom::UkmRecorderInterfaceRequest request) {
-  static base::AtomicSequenceNumber seq;
-  mojo::MakeStrongBinding(
-      std::make_unique<UkmRecorderInterface>(ukm_recorder, seq.GetNext() + 1),
-      std::move(request));
+  mojo::MakeStrongBinding(std::make_unique<UkmRecorderInterface>(ukm_recorder),
+                          std::move(request));
 }
 
 void UkmRecorderInterface::AddEntry(ukm::mojom::UkmEntryPtr ukm_entry) {
-  ukm_entry->source_id =
-      ukm::ConvertSourceIdFromInstance(instance_id_, ukm_entry->source_id);
   ukm_recorder_->AddEntry(std::move(ukm_entry));
 }
 
 void UkmRecorderInterface::UpdateSourceURL(int64_t source_id,
                                            const std::string& url) {
-  ukm_recorder_->UpdateSourceURL(
-      ukm::ConvertSourceIdFromInstance(instance_id_, source_id), GURL(url));
+  ukm_recorder_->UpdateSourceURL(source_id, GURL(url));
 }
 
 }  // namespace metrics

services/metrics/ukm_recorder_interface.h

@@ -15,7 +15,7 @@ namespace metrics {
 
 class UkmRecorderInterface : public ukm::mojom::UkmRecorderInterface {
  public:
-  UkmRecorderInterface(ukm::UkmRecorder* ukm_recorder, int64_t instance_id);
+  UkmRecorderInterface(ukm::UkmRecorder* ukm_recorder);
   ~UkmRecorderInterface() override;
 
   static void Create(ukm::UkmRecorder* ukm_recorder,
@@ -27,7 +27,6 @@ class UkmRecorderInterface : public ukm::mojom::UkmRecorderInterface {
   void UpdateSourceURL(int64_t source_id, const std::string& url) override;
 
   ukm::UkmRecorder* ukm_recorder_;
-  int64_t instance_id_;
 
   DISALLOW_COPY_AND_ASSIGN(UkmRecorderInterface);
 };