[Sampling profiler] Support user-specified metadata key

Provides API support for specifying an optional user-defined key along
with the metadata name. This can be used to distinguish samples based
on additional state beyond just the name -- e.g. execution in service
of different tabs within the same renderer process.

Uploading of the key state with the samples will be implemented in a
following CL.

Bug: 976864
Change-Id: Ib924be48a816e65e83976c34e27efea19da61925
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1891112Reviewed-by: Charlie Andrews <charliea@chromium.org>
Commit-Queue: Mike Wittman <wittman@chromium.org>
Cr-Commit-Position: refs/heads/master@{#711265}

base/profiler/metadata_recorder.cc

@@ -20,7 +20,9 @@ MetadataRecorder::MetadataRecorder() {
 
 MetadataRecorder::~MetadataRecorder() = default;
 
-void MetadataRecorder::Set(uint64_t name_hash, int64_t value) {
+void MetadataRecorder::Set(uint64_t name_hash,
+                           Optional<int64_t> key,
+                           int64_t value) {
   base::AutoLock lock(write_lock_);
 
   // Acquiring the |write_lock_| ensures that:
@@ -32,7 +34,7 @@ void MetadataRecorder::Set(uint64_t name_hash, int64_t value) {
   size_t item_slots_used = item_slots_used_.load(std::memory_order_relaxed);
   for (size_t i = 0; i < item_slots_used; ++i) {
     auto& item = items_[i];
-    if (item.name_hash == name_hash) {
+    if (item.name_hash == name_hash && item.key == key) {
       item.value.store(value, std::memory_order_relaxed);
 
       const bool was_active =
@@ -64,18 +66,19 @@ void MetadataRecorder::Set(uint64_t name_hash, int64_t value) {
   // is ready.
   auto& item = items_[item_slots_used];
   item.name_hash = name_hash;
+  item.key = key;
   item.value.store(value, std::memory_order_relaxed);
   item.is_active.store(true, std::memory_order_release);
   item_slots_used_.fetch_add(1, std::memory_order_release);
 }
 
-void MetadataRecorder::Remove(uint64_t name_hash) {
+void MetadataRecorder::Remove(uint64_t name_hash, Optional<int64_t> key) {
   base::AutoLock lock(write_lock_);
 
   size_t item_slots_used = item_slots_used_.load(std::memory_order_relaxed);
   for (size_t i = 0; i < item_slots_used; ++i) {
     auto& item = items_[i];
-    if (item.name_hash == name_hash) {
+    if (item.name_hash == name_hash && item.key == key) {
       // A removed item will occupy its slot until that slot is reclaimed.
       const bool was_active =
           item.is_active.exchange(false, std::memory_order_relaxed);
@@ -132,7 +135,7 @@ size_t MetadataRecorder::GetItems(
     // that field is always set last, we ignore half-created items.
     if (item.is_active.load(std::memory_order_acquire)) {
       (*items)[write_index++] = ProfileBuilder::MetadataItem{
-          item.name_hash, item.value.load(std::memory_order_relaxed)};
+          item.name_hash, item.key, item.value.load(std::memory_order_relaxed)};
     }
   }
 

base/profiler/metadata_recorder.h

@@ -9,6 +9,7 @@
 #include <atomic>
 #include <utility>
 
+#include "base/optional.h"
 #include "base/profiler/profile_builder.h"
 #include "base/synchronization/lock.h"
 #include "base/thread_annotations.h"
@@ -127,14 +128,14 @@ class BASE_EXPORT MetadataRecorder {
   MetadataRecorder(const MetadataRecorder&) = delete;
   MetadataRecorder& operator=(const MetadataRecorder&) = delete;
 
-  // Sets a name hash/value pair, overwriting any previous value set for that
-  // name hash.
-  void Set(uint64_t name_hash, int64_t value);
+  // Sets a value for a (|name_hash|, |key|) pair, overwriting any value
+  // previously set for the pair. Nullopt keys are treated as just another key
+  // state for the purpose of associating values.
+  void Set(uint64_t name_hash, Optional<int64_t> key, int64_t value);
 
-  // Removes the item with the specified name hash.
-  //
-  // If such an item does not exist, this has no effect.
-  void Remove(uint64_t name_hash);
+  // Removes the item with the specified name hash and optional key. Has no
+  // effect if such an item does not exist.
+  void Remove(uint64_t name_hash, Optional<int64_t> key);
 
   // Creates a MetadataProvider object for the recorder, which acquires the
   // necessary exclusive read lock and provides access to the recorder's items
@@ -205,12 +206,15 @@ class BASE_EXPORT MetadataRecorder {
     // is marked as active.
     std::atomic<bool> is_active{false};
 
-    // Doesn't need atomicity or memory order constraints because no reader will
-    // attempt to read it mid-write. Specifically, readers wait until
-    // |is_active| is true to read |name_hash|. Because |is_active| is always
-    // stored with a memory_order_release fence, we're guaranteed that
-    // |name_hash| will be finished writing before |is_active| is set to true.
+    // Neither name_hash or key require atomicity or memory order constraints
+    // because no reader will attempt to read them mid-write. Specifically,
+    // readers wait until |is_active| is true to read them. Because |is_active|
+    // is always stored with a memory_order_release fence, we're guaranteed that
+    // |name_hash| and |key| will be finished writing before |is_active| is set
+    // to true.
     uint64_t name_hash;
+    Optional<int64_t> key;
+
     // Requires atomic reads and writes to avoid word tearing when updating an
     // existing item unsynchronized. Does not require acquire/release semantics
     // because we rely on the |is_active| acquire/release semantics to ensure

base/profiler/metadata_recorder_unittest.cc

@@ -27,43 +27,46 @@ TEST(MetadataRecorderTest, GetItems_Empty) {
 
   size_t item_count = recorder.CreateMetadataProvider()->GetItems(&items);
 
-  ASSERT_EQ(0u, item_count);
+  EXPECT_EQ(0u, item_count);
 }
 
 TEST(MetadataRecorderTest, Set_NewNameHash) {
   MetadataRecorder recorder;
 
-  recorder.Set(10, 20);
+  recorder.Set(10, nullopt, 20);
 
   base::ProfileBuilder::MetadataItemArray items;
   size_t item_count;
   {
     item_count = recorder.CreateMetadataProvider()->GetItems(&items);
     ASSERT_EQ(1u, item_count);
-    ASSERT_EQ(10u, items[0].name_hash);
-    ASSERT_EQ(20, items[0].value);
+    EXPECT_EQ(10u, items[0].name_hash);
+    EXPECT_FALSE(items[0].key.has_value());
+    EXPECT_EQ(20, items[0].value);
   }
 
-  recorder.Set(20, 30);
+  recorder.Set(20, nullopt, 30);
 
   {
     item_count = recorder.CreateMetadataProvider()->GetItems(&items);
     ASSERT_EQ(2u, item_count);
-    ASSERT_EQ(20u, items[1].name_hash);
-    ASSERT_EQ(30, items[1].value);
+    EXPECT_EQ(20u, items[1].name_hash);
+    EXPECT_FALSE(items[1].key.has_value());
+    EXPECT_EQ(30, items[1].value);
   }
 }
 
 TEST(MetadataRecorderTest, Set_ExistingNameNash) {
   MetadataRecorder recorder;
-  recorder.Set(10, 20);
-  recorder.Set(10, 30);
+  recorder.Set(10, nullopt, 20);
+  recorder.Set(10, nullopt, 30);
 
   base::ProfileBuilder::MetadataItemArray items;
   size_t item_count = recorder.CreateMetadataProvider()->GetItems(&items);
   ASSERT_EQ(1u, item_count);
-  ASSERT_EQ(10u, items[0].name_hash);
-  ASSERT_EQ(30, items[0].value);
+  EXPECT_EQ(10u, items[0].name_hash);
+  EXPECT_FALSE(items[0].key.has_value());
+  EXPECT_EQ(30, items[0].value);
 }
 
 TEST(MetadataRecorderTest, Set_ReAddRemovedNameNash) {
@@ -71,59 +74,112 @@ TEST(MetadataRecorderTest, Set_ReAddRemovedNameNash) {
   base::ProfileBuilder::MetadataItemArray items;
   std::vector<base::ProfileBuilder::MetadataItem> expected;
   for (size_t i = 0; i < items.size(); ++i) {
-    expected.push_back(base::ProfileBuilder::MetadataItem{i, 0});
-    recorder.Set(i, 0);
+    expected.push_back(base::ProfileBuilder::MetadataItem{i, nullopt, 0});
+    recorder.Set(i, nullopt, 0);
   }
 
   // By removing an item from a full recorder, re-setting the same item, and
   // verifying that the item is returned, we can verify that the recorder is
   // reusing the inactive slot for the same name hash instead of trying (and
   // failing) to allocate a new slot.
-  recorder.Remove(3);
-  recorder.Set(3, 0);
+  recorder.Remove(3, nullopt);
+  recorder.Set(3, nullopt, 0);
 
   size_t item_count = recorder.CreateMetadataProvider()->GetItems(&items);
   EXPECT_EQ(items.size(), item_count);
-  ASSERT_THAT(expected, ::testing::UnorderedElementsAreArray(items));
+  EXPECT_THAT(expected, ::testing::UnorderedElementsAreArray(items));
 }
 
 TEST(MetadataRecorderTest, Set_AddPastMaxCount) {
   MetadataRecorder recorder;
   base::ProfileBuilder::MetadataItemArray items;
   for (size_t i = 0; i < items.size(); ++i) {
-    recorder.Set(i, 0);
+    recorder.Set(i, nullopt, 0);
   }
 
   // This should fail silently.
-  recorder.Set(items.size(), 0);
+  recorder.Set(items.size(), nullopt, 0);
+}
+
+TEST(MetadataRecorderTest, Set_NulloptKeyIsIndependentOfNonNulloptKey) {
+  MetadataRecorder recorder;
+
+  recorder.Set(10, 100, 20);
+
+  base::ProfileBuilder::MetadataItemArray items;
+  size_t item_count;
+  {
+    item_count = recorder.CreateMetadataProvider()->GetItems(&items);
+    ASSERT_EQ(1u, item_count);
+    EXPECT_EQ(10u, items[0].name_hash);
+    ASSERT_TRUE(items[0].key.has_value());
+    EXPECT_EQ(100, *items[0].key);
+    EXPECT_EQ(20, items[0].value);
+  }
+
+  recorder.Set(10, nullopt, 30);
+
+  {
+    item_count = recorder.CreateMetadataProvider()->GetItems(&items);
+    ASSERT_EQ(2u, item_count);
+
+    EXPECT_EQ(10u, items[0].name_hash);
+    ASSERT_TRUE(items[0].key.has_value());
+    EXPECT_EQ(100, *items[0].key);
+    EXPECT_EQ(20, items[0].value);
+
+    EXPECT_EQ(10u, items[1].name_hash);
+    EXPECT_FALSE(items[1].key.has_value());
+    EXPECT_EQ(30, items[1].value);
+  }
 }
 
 TEST(MetadataRecorderTest, Remove) {
   MetadataRecorder recorder;
-  recorder.Set(10, 20);
-  recorder.Set(30, 40);
-  recorder.Set(50, 60);
-  recorder.Remove(30);
+  recorder.Set(10, nullopt, 20);
+  recorder.Set(30, nullopt, 40);
+  recorder.Set(50, nullopt, 60);
+  recorder.Remove(30, nullopt);
 
   base::ProfileBuilder::MetadataItemArray items;
   size_t item_count = recorder.CreateMetadataProvider()->GetItems(&items);
   ASSERT_EQ(2u, item_count);
-  ASSERT_EQ(10u, items[0].name_hash);
-  ASSERT_EQ(20, items[0].value);
-  ASSERT_EQ(50u, items[1].name_hash);
-  ASSERT_EQ(60, items[1].value);
+  EXPECT_EQ(10u, items[0].name_hash);
+  EXPECT_FALSE(items[0].key.has_value());
+  EXPECT_EQ(20, items[0].value);
+  EXPECT_EQ(50u, items[1].name_hash);
+  EXPECT_FALSE(items[1].key.has_value());
+  EXPECT_EQ(60, items[1].value);
 }
 
 TEST(MetadataRecorderTest, Remove_DoesntExist) {
   MetadataRecorder recorder;
-  recorder.Set(10, 20);
-  recorder.Remove(20);
+  recorder.Set(10, nullopt, 20);
+  recorder.Remove(20, nullopt);
+
+  base::ProfileBuilder::MetadataItemArray items;
+  size_t item_count = recorder.CreateMetadataProvider()->GetItems(&items);
+  ASSERT_EQ(1u, item_count);
+  EXPECT_EQ(10u, items[0].name_hash);
+  EXPECT_FALSE(items[0].key.has_value());
+  EXPECT_EQ(20, items[0].value);
+}
+
+TEST(MetadataRecorderTest, Remove_NulloptKeyIsIndependentOfNonNulloptKey) {
+  MetadataRecorder recorder;
+
+  recorder.Set(10, 100, 20);
+  recorder.Set(10, nullopt, 30);
+
+  recorder.Remove(10, nullopt);
 
   base::ProfileBuilder::MetadataItemArray items;
   size_t item_count = recorder.CreateMetadataProvider()->GetItems(&items);
   ASSERT_EQ(1u, item_count);
-  ASSERT_EQ(10u, items[0].name_hash);
-  ASSERT_EQ(20, items[0].value);
+  EXPECT_EQ(10u, items[0].name_hash);
+  ASSERT_TRUE(items[0].key.has_value());
+  EXPECT_EQ(100, *items[0].key);
+  EXPECT_EQ(20, items[0].value);
 }
 
 TEST(MetadataRecorderTest, ReclaimInactiveSlots) {
@@ -132,22 +188,23 @@ TEST(MetadataRecorderTest, ReclaimInactiveSlots) {
   std::set<base::ProfileBuilder::MetadataItem> items_set;
   // Fill up the metadata map.
   for (size_t i = 0; i < base::ProfileBuilder::MAX_METADATA_COUNT; ++i) {
-    recorder.Set(i, i);
-    items_set.insert(base::ProfileBuilder::MetadataItem{i, i});
+    recorder.Set(i, nullopt, i);
+    items_set.insert(base::ProfileBuilder::MetadataItem{i, nullopt, i});
   }
 
   // Remove every fourth entry to fragment the data.
   size_t entries_removed = 0;
   for (size_t i = 3; i < base::ProfileBuilder::MAX_METADATA_COUNT; i += 4) {
-    recorder.Remove(i);
+    recorder.Remove(i, nullopt);
     ++entries_removed;
-    items_set.erase(base::ProfileBuilder::MetadataItem{i, i});
+    items_set.erase(base::ProfileBuilder::MetadataItem{i, nullopt, i});
   }
 
   // Ensure that the inactive slots are reclaimed to make room for more entries.
   for (size_t i = 1; i <= entries_removed; ++i) {
-    recorder.Set(i * 100, i * 100);
-    items_set.insert(base::ProfileBuilder::MetadataItem{i * 100, i * 100});
+    recorder.Set(i * 100, nullopt, i * 100);
+    items_set.insert(
+        base::ProfileBuilder::MetadataItem{i * 100, nullopt, i * 100});
   }
 
   base::ProfileBuilder::MetadataItemArray items_arr;
@@ -156,8 +213,8 @@ TEST(MetadataRecorderTest, ReclaimInactiveSlots) {
   base::ProfileBuilder::MetadataItemArray recorder_items;
   size_t recorder_item_count =
       recorder.CreateMetadataProvider()->GetItems(&recorder_items);
-  ASSERT_EQ(recorder_item_count, base::ProfileBuilder::MAX_METADATA_COUNT);
-  ASSERT_THAT(recorder_items, ::testing::UnorderedElementsAreArray(items_arr));
+  EXPECT_EQ(recorder_item_count, base::ProfileBuilder::MAX_METADATA_COUNT);
+  EXPECT_THAT(recorder_items, ::testing::UnorderedElementsAreArray(items_arr));
 }
 
 TEST(MetadataRecorderTest, MetadataSlotsUsedUmaHistogram) {
@@ -165,7 +222,7 @@ TEST(MetadataRecorderTest, MetadataSlotsUsedUmaHistogram) {
   base::HistogramTester histogram_tester;
 
   for (size_t i = 0; i < base::ProfileBuilder::MAX_METADATA_COUNT; ++i) {
-    recorder.Set(i * 10, i * 100);
+    recorder.Set(i * 10, nullopt, i * 100);
   }
 
   EXPECT_THAT(

base/profiler/profile_builder.cc

@@ -4,4 +4,20 @@
 
 #include "base/profiler/profile_builder.h"
 
-const size_t base::ProfileBuilder::MAX_METADATA_COUNT;
+namespace base {
+
+const size_t ProfileBuilder::MAX_METADATA_COUNT;
+
+ProfileBuilder::MetadataItem::MetadataItem(uint64_t name_hash,
+                                           Optional<int64_t> key,
+                                           int64_t value)
+    : name_hash(name_hash), key(key), value(value) {}
+
+ProfileBuilder::MetadataItem::MetadataItem() : name_hash(0), value(0) {}
+
+ProfileBuilder::MetadataItem::MetadataItem(const MetadataItem& other) = default;
+
+ProfileBuilder::MetadataItem& ProfileBuilder::MetadataItem::MetadataItem::
+operator=(const MetadataItem& other) = default;
+
+}  // namespace base

base/profiler/profile_builder.h

@@ -8,6 +8,7 @@
 #include <memory>
 
 #include "base/base_export.h"
+#include "base/optional.h"
 #include "base/profiler/frame.h"
 #include "base/sampling_heap_profiler/module_cache.h"
 #include "base/time/time.h"
@@ -26,9 +27,17 @@ class BASE_EXPORT ProfileBuilder {
   // up modules from addresses.
   virtual ModuleCache* GetModuleCache() = 0;
 
-  struct MetadataItem {
+  struct BASE_EXPORT MetadataItem {
+    MetadataItem(uint64_t name_hash, Optional<int64_t> key, int64_t value);
+    MetadataItem();
+
+    MetadataItem(const MetadataItem& other);
+    MetadataItem& operator=(const MetadataItem& other);
+
     // The hash of the metadata name, as produced by base::HashMetricName().
     uint64_t name_hash;
+    // The key if specified when setting the item.
+    Optional<int64_t> key;
     // The value of the metadata item.
     int64_t value;
   };

base/profiler/sample_metadata.cc

@@ -12,19 +12,34 @@ namespace base {
 ScopedSampleMetadata::ScopedSampleMetadata(base::StringPiece name,
                                            int64_t value)
     : name_hash_(HashMetricName(name)) {
-  GetSampleMetadataRecorder()->Set(name_hash_, value);
+  GetSampleMetadataRecorder()->Set(name_hash_, nullopt, value);
+}
+
+ScopedSampleMetadata::ScopedSampleMetadata(base::StringPiece name,
+                                           int64_t key,
+                                           int64_t value)
+    : name_hash_(HashMetricName(name)), key_(key) {
+  GetSampleMetadataRecorder()->Set(name_hash_, key, value);
 }
 
 ScopedSampleMetadata::~ScopedSampleMetadata() {
-  GetSampleMetadataRecorder()->Remove(name_hash_);
+  GetSampleMetadataRecorder()->Remove(name_hash_, key_);
 }
 
 void SetSampleMetadata(base::StringPiece name, int64_t value) {
-  GetSampleMetadataRecorder()->Set(base::HashMetricName(name), value);
+  GetSampleMetadataRecorder()->Set(base::HashMetricName(name), nullopt, value);
+}
+
+void SetSampleMetadata(base::StringPiece name, int64_t key, int64_t value) {
+  GetSampleMetadataRecorder()->Set(base::HashMetricName(name), key, value);
 }
 
 void RemoveSampleMetadata(base::StringPiece name) {
-  GetSampleMetadataRecorder()->Remove(base::HashMetricName(name));
+  GetSampleMetadataRecorder()->Remove(base::HashMetricName(name), nullopt);
+}
+
+void RemoveSampleMetadata(base::StringPiece name, int64_t key) {
+  GetSampleMetadataRecorder()->Remove(base::HashMetricName(name), key);
 }
 
 base::MetadataRecorder* GetSampleMetadataRecorder() {

base/profiler/sample_metadata.h

@@ -5,6 +5,7 @@
 #ifndef BASE_PROFILER_SAMPLE_METADATA_H_
 #define BASE_PROFILER_SAMPLE_METADATA_H_
 
+#include "base/optional.h"
 #include "base/profiler/metadata_recorder.h"
 #include "base/strings/string_piece.h"
 
@@ -52,7 +53,18 @@ namespace base {
 
 class BASE_EXPORT ScopedSampleMetadata {
  public:
+  // Set the metadata value associated with |name|.
   ScopedSampleMetadata(base::StringPiece name, int64_t value);
+
+  // Set the metadata value associated with the pair (|name|, |key|). This
+  // constructor allows the metadata to be associated with an additional
+  // user-defined key. One might supply a key based on the frame id, for
+  // example, to distinguish execution in service of scrolling between different
+  // frames. Prefer the previous constructor if no user-defined metadata is
+  // required. Note: values specified for a name and key are stored separately
+  // from values specified with only a name.
+  ScopedSampleMetadata(base::StringPiece name, int64_t key, int64_t value);
+
   ScopedSampleMetadata(const ScopedSampleMetadata&) = delete;
   ~ScopedSampleMetadata();
 
@@ -60,18 +72,39 @@ class BASE_EXPORT ScopedSampleMetadata {
 
  private:
   const uint64_t name_hash_;
+  Optional<int64_t> key_;
 };
 
-// Sets a name hash/value pair in the process global stack sampling profiler
-// metadata, overwriting any previous value set for that name hash.
+// Set the metadata value associated with |name| in the process-global stack
+// sampling profiler metadata, overwriting any previous value set for that
+// |name|.
 BASE_EXPORT void SetSampleMetadata(base::StringPiece name, int64_t value);
 
-// Removes the metadata item with the specified name hash from the process
-// global stack sampling profiler metadata.
+// Set the metadata value associated with the pair (|name|, |key|) in the
+// process-global stack sampling profiler metadata, overwriting any previous
+// value set for that (|name|, |key|) pair. This constructor allows the metadata
+// to be associated with an additional user-defined key. One might supply a key
+// based on the frame id, for example, to distinguish execution in service of
+// scrolling between different frames. Prefer the previous function if no
+// user-defined metadata is required. Note: values specified for a name and key
+// are stored separately from values specified with only a name.
+BASE_EXPORT void SetSampleMetadata(base::StringPiece name,
+                                   int64_t key,
+                                   int64_t value);
+
+// Removes the metadata item with the specified name from the process-global
+// stack sampling profiler metadata.
 //
 // If such an item doesn't exist, this has no effect.
 BASE_EXPORT void RemoveSampleMetadata(base::StringPiece name);
 
+// Removes the metadata item with the specified (|name|, |key|) pair from the
+// process-global stack sampling profiler metadata. This function does not alter
+// values set with the name |name| but no key.
+//
+// If such an item doesn't exist, this has no effect.
+BASE_EXPORT void RemoveSampleMetadata(base::StringPiece name, int64_t key);
+
 // Returns the process-global metadata recorder instance used for tracking
 // sampling profiler metadata.
 //

base/profiler/sample_metadata_unittest.cc

@@ -11,28 +11,78 @@ namespace base {
 
 TEST(SampleMetadataTest, ScopedSampleMetadata) {
   base::ProfileBuilder::MetadataItemArray items;
-  {
-    auto get_items = GetSampleMetadataRecorder()->CreateMetadataProvider();
-    ASSERT_EQ(0u, get_items->GetItems(&items));
-  }
+  ASSERT_EQ(0u, GetSampleMetadataRecorder()->CreateMetadataProvider()->GetItems(
+                    &items));
 
   {
     ScopedSampleMetadata m("myname", 100);
 
-    {
-      ASSERT_EQ(1u,
-                GetSampleMetadataRecorder()->CreateMetadataProvider()->GetItems(
-                    &items));
-      EXPECT_EQ(base::HashMetricName("myname"), items[0].name_hash);
-      EXPECT_EQ(100, items[0].value);
-    }
+    ASSERT_EQ(1u,
+              GetSampleMetadataRecorder()->CreateMetadataProvider()->GetItems(
+                  &items));
+    EXPECT_EQ(base::HashMetricName("myname"), items[0].name_hash);
+    EXPECT_FALSE(items[0].key.has_value());
+    EXPECT_EQ(100, items[0].value);
   }
 
+  ASSERT_EQ(0u, GetSampleMetadataRecorder()->CreateMetadataProvider()->GetItems(
+                    &items));
+}
+
+TEST(SampleMetadataTest, ScopedSampleMetadataWithKey) {
+  base::ProfileBuilder::MetadataItemArray items;
+  ASSERT_EQ(0u, GetSampleMetadataRecorder()->CreateMetadataProvider()->GetItems(
+                    &items));
+
   {
-    ASSERT_EQ(0u,
+    ScopedSampleMetadata m("myname", 10, 100);
+
+    ASSERT_EQ(1u,
               GetSampleMetadataRecorder()->CreateMetadataProvider()->GetItems(
                   &items));
+    EXPECT_EQ(base::HashMetricName("myname"), items[0].name_hash);
+    ASSERT_TRUE(items[0].key.has_value());
+    EXPECT_EQ(10, *items[0].key);
+    EXPECT_EQ(100, items[0].value);
   }
+
+  ASSERT_EQ(0u, GetSampleMetadataRecorder()->CreateMetadataProvider()->GetItems(
+                    &items));
+}
+
+TEST(SampleMetadataTest, SampleMetadata) {
+  base::ProfileBuilder::MetadataItemArray items;
+  ASSERT_EQ(0u, GetSampleMetadataRecorder()->CreateMetadataProvider()->GetItems(
+                    &items));
+
+  SetSampleMetadata("myname", 100);
+  ASSERT_EQ(1u, GetSampleMetadataRecorder()->CreateMetadataProvider()->GetItems(
+                    &items));
+  EXPECT_EQ(base::HashMetricName("myname"), items[0].name_hash);
+  EXPECT_FALSE(items[0].key.has_value());
+  EXPECT_EQ(100, items[0].value);
+
+  RemoveSampleMetadata("myname");
+  ASSERT_EQ(0u, GetSampleMetadataRecorder()->CreateMetadataProvider()->GetItems(
+                    &items));
+}
+
+TEST(SampleMetadataTest, SampleMetadataWithKey) {
+  base::ProfileBuilder::MetadataItemArray items;
+  ASSERT_EQ(0u, GetSampleMetadataRecorder()->CreateMetadataProvider()->GetItems(
+                    &items));
+
+  SetSampleMetadata("myname", 10, 100);
+  ASSERT_EQ(1u, GetSampleMetadataRecorder()->CreateMetadataProvider()->GetItems(
+                    &items));
+  EXPECT_EQ(base::HashMetricName("myname"), items[0].name_hash);
+  ASSERT_TRUE(items[0].key.has_value());
+  EXPECT_EQ(10, *items[0].key);
+  EXPECT_EQ(100, items[0].value);
+
+  RemoveSampleMetadata("myname", 10);
+  ASSERT_EQ(0u, GetSampleMetadataRecorder()->CreateMetadataProvider()->GetItems(
+                    &items));
 }
 
 }  // namespace base

components/metrics/call_stack_profile_builder_unittest.cc

@@ -463,7 +463,7 @@ TEST(CallStackProfileBuilderTest, MetadataRecorder_RepeatItem) {
   TestModule module;
   base::Frame frame = {0x10, &module};
 
-  metadata_recorder.Set(100, 10);
+  metadata_recorder.Set(100, base::nullopt, 10);
   {
     auto get_items = metadata_recorder.CreateMetadataProvider();
     profile_builder->RecordMetadata(get_items.get());
@@ -505,13 +505,13 @@ TEST(CallStackProfileBuilderTest, MetadataRecorder_ModifiedItem) {
   TestModule module;
   base::Frame frame = {0x10, &module};
 
-  metadata_recorder.Set(100, 10);
+  metadata_recorder.Set(100, base::nullopt, 10);
   {
     auto get_items = metadata_recorder.CreateMetadataProvider();
     profile_builder->RecordMetadata(get_items.get());
   }
   profile_builder->OnSampleCompleted({frame});
-  metadata_recorder.Set(100, 11);
+  metadata_recorder.Set(100, base::nullopt, 11);
   {
     auto get_items = metadata_recorder.CreateMetadataProvider();
     profile_builder->RecordMetadata(get_items.get());
@@ -551,15 +551,15 @@ TEST(CallStackProfileBuilderTest, MetadataRecorder_NewItem) {
   TestModule module;
   base::Frame frame = {0x10, &module};
 
-  metadata_recorder.Set(100, 10);
+  metadata_recorder.Set(100, base::nullopt, 10);
   {
     auto get_items = metadata_recorder.CreateMetadataProvider();
     profile_builder->RecordMetadata(get_items.get());
   }
   profile_builder->OnSampleCompleted({frame});
 
-  metadata_recorder.Set(100, 11);
-  metadata_recorder.Set(200, 20);
+  metadata_recorder.Set(100, base::nullopt, 11);
+  metadata_recorder.Set(200, base::nullopt, 20);
   {
     auto get_items = metadata_recorder.CreateMetadataProvider();
     profile_builder->RecordMetadata(get_items.get());
@@ -603,13 +603,13 @@ TEST(CallStackProfileBuilderTest, MetadataRecorder_RemovedItem) {
   TestModule module;
   base::Frame frame = {0x10, &module};
 
-  metadata_recorder.Set(100, 10);
+  metadata_recorder.Set(100, base::nullopt, 10);
   {
     auto get_items = metadata_recorder.CreateMetadataProvider();
     profile_builder->RecordMetadata(get_items.get());
   }
   profile_builder->OnSampleCompleted({frame});
-  metadata_recorder.Remove(100);
+  metadata_recorder.Remove(100, base::nullopt);
   {
     auto get_items = metadata_recorder.CreateMetadataProvider();
     profile_builder->RecordMetadata(get_items.get());