Allow a metric to be unlisted

doc/developer-guide/internal-libraries/Metrics.en.rst

@@ -98,6 +98,24 @@ never as a side effect of a broad query.
    renamed or removed between releases without notice. Do not build monitoring on them; use the
    published aggregate instead.
 
+Enumerating metrics
+===================
+
+``for_each`` visits every listed metric of a store, in creation order:
+
+.. code-block:: cpp
+
+    ts::Metrics::instance().for_each([](std::string_view name, ts::Metrics::MetricType type, int64_t value) {
+      // ...
+    });
+
+This is the only way to enumerate a store. There is no public iterator, and deliberately so:
+enumeration is always the whole store, so nothing can hold a cursor across changes to the store or
+name a position the walk would skip. Reach a single metric by name with ``lookup`` instead.
+
+The callback must not create a metric, which would be an attempt to grow the store from inside a
+pass over it.
+
 Derived metrics
 ===============
 
@@ -188,6 +206,42 @@ sampling point*, not the true peak. There are two ways to arrange this, with dif
 
 Which is appropriate depends on whether the consumer needs to aggregate over time downstream.
 
+Unlisting a metric
+==================
+
+A metric can be taken out of the store's listing after the fact. An unlisted metric is skipped by
+iteration, so it disappears from ``traffic_ctl metric match``, the JSONRPC record lookup and
+``stats_over_http``, without either of those consumers needing to know about it:
+
+.. code-block:: cpp
+
+    auto &m = ts::Metrics::instance();
+
+    m.unlist(id);                       // by id
+    m.unlist("proxy.process.example");  // or by name
+
+    m.relist(id);                       // put it back
+
+The slot, the name and the atomic all survive: an unlisted number that still rings. An unlisted
+metric still resolves through ``lookup``, so an exact name query, a logging field reference and
+``TSStatFindName`` all continue to work, and its value may still be read and written. Creating the
+same name again relists it and returns the same id with its accumulated value intact, so a metric
+that comes and goes with a configuration setting costs nothing to bring back.
+
+This exists because the decision to publish a name is otherwise made once, when the metric is first
+created, and can never be revisited. Any metric whose name or publication policy depends on a
+runtime changeable setting needs a way to retract a name it has already published.
+
+.. important::
+
+   Unlisting hides; it does not free. The slot and the name remain allocated against the storage
+   limit below. Unlisting does not make an unbounded naming scheme safe.
+
+.. note::
+
+   The set walked is fixed when ``for_each`` begins, so a metric created while it runs is not
+   visited.
+
 Storage limits
 ==============
 

include/tsutil/Metrics.h

@@ -93,14 +93,25 @@ class Metrics
   static constexpr int      METRIC_TYPE_BITS = 29;
   static constexpr int      METRIC_TYPE_MASK = 0x1FFF;
 
+  /// The reserved slot 0 of every store, returned when an id cannot be produced. Present under this
+  /// name in both stores, so a consumer querying both has to expect it twice.
+  static constexpr std::string_view BAD_ID_NAME{"proxy.process.api.metrics.bad_id"};
+
 private:
-  using NameAndId       = std::tuple<std::string, IdType>;
-  using LookupTable     = std::unordered_map<std::string_view, IdType>;
-  using NameStorage     = std::array<NameAndId, MAX_SIZE>;
-  using AtomicStorage   = std::array<AtomicType, MAX_SIZE>;
-  using NamesAndAtomics = std::tuple<NameStorage, AtomicStorage>;
+  using NameAndId     = std::tuple<std::string, IdType>;
+  using LookupTable   = std::unordered_map<std::string_view, IdType>;
+  using NameStorage   = std::array<NameAndId, MAX_SIZE>;
+  using AtomicStorage = std::array<AtomicType, MAX_SIZE>;
+  /// Per slot flag bits, see @c UNLISTED. A parallel array rather than a member of @c NameAndId
+  /// because an atomic member would make that tuple neither copyable nor movable, and the slot is
+  /// written there with a tuple assignment.
+  using FlagStorage     = std::array<std::atomic<uint8_t>, MAX_SIZE>;
+  using NamesAndAtomics = std::tuple<NameStorage, AtomicStorage, FlagStorage>;
   using BlobStorage     = std::array<std::unique_ptr<NamesAndAtomics>, MAX_BLOBS>;
 
+  /// The slot exists and is still resolvable by name or id, but is skipped by iteration.
+  static constexpr uint8_t UNLISTED = 0x01;
+
 public:
   Metrics(const self_type &)              = delete;
   self_type &operator=(const self_type &) = delete;
@@ -145,6 +156,57 @@ class Metrics
   {
     return _storage->lookup(id, out_name, type);
   }
+
+  /** Take @a id out of the store's listing.
+   *
+   * An unlisted metric keeps its slot, its name and its atomic. It is skipped by iteration, so it
+   * vanishes from everything that enumerates the store, but it still resolves through @c lookup and
+   * its value may still be read and written -- an unlisted number that still rings. Creating the
+   * same name again relists it and returns the same id.
+   *
+   * @return @c false if @a id does not name an allocated slot.
+   */
+  bool
+  unlist(IdType id)
+  {
+    return _storage->set_listed(id, false);
+  }
+
+  /// Put @a id back in the listing. @see unlist
+  bool
+  relist(IdType id)
+  {
+    return _storage->set_listed(id, true);
+  }
+
+  /** Whether @a id is enumerated.
+   *
+   * @return @c false for an unlisted metric, and also for an id that names no allocated slot --
+   *   neither appears in iteration.
+   */
+  bool
+  listed(IdType id) const
+  {
+    return _storage->listed(id);
+  }
+
+  /// Convenience for callers that publish by name and do not retain the id. @see unlist
+  bool
+  unlist(std::string_view name)
+  {
+    auto id = lookup(name);
+
+    return id != NOT_FOUND && unlist(id);
+  }
+
+  /// Convenience for callers that publish by name and do not retain the id. @see relist
+  bool
+  relist(std::string_view name)
+  {
+    auto id = lookup(name);
+
+    return id != NOT_FOUND && relist(id);
+  }
   AtomicType &
   operator[](IdType id)
   {
@@ -191,87 +253,21 @@ class Metrics
     return _storage->valid(id);
   }
 
-  // Static methods to encapsulate access to the atomic's
-  class iterator
-  {
-  public:
-    using iterator_category = std::input_iterator_tag;
-    using value_type        = std::tuple<std::string_view, MetricType, int64_t>;
-    using difference_type   = ptrdiff_t;
-    using pointer           = value_type *;
-    using reference         = value_type &;
-
-    iterator(const Metrics &m, IdType pos) : _metrics(m), _it(pos) {}
-
-    iterator &
-    operator++()
-    {
-      next();
-
-      return *this;
-    }
-
-    iterator
-    operator++(int)
-    {
-      iterator result = *this;
-
-      next();
-
-      return result;
-    }
-
-    value_type
-    operator*() const
-    {
-      std::string_view name;
-      MetricType       type;
-      auto             metric = _metrics.lookup(_it, &name, &type);
-
-      return std::make_tuple(name, type, metric->_value.load());
-    }
-
-    bool
-    operator==(const iterator &o) const
-    {
-      return _it == o._it && std::addressof(_metrics) == std::addressof(o._metrics);
-    }
-
-    bool
-    operator!=(const iterator &o) const
-    {
-      return _it != o._it || std::addressof(_metrics) != std::addressof(o._metrics);
-    }
-
-  private:
-    void next();
-
-    const Metrics  &_metrics;
-    Metrics::IdType _it;
-  };
-
-  iterator
-  begin() const
-  {
-    return iterator(*this, 0);
-  }
-
-  iterator
-  end() const
-  {
-    return iterator(*this, _storage->next_free_id());
-  }
-
-  iterator
-  find(const std::string_view name) const
+  /** Visit every listed metric.
+   *
+   * @a func is called as <tt>func(std::string_view name, MetricType type, int64_t value)</tt> for
+   * each listed metric, in creation order. Unlisted metrics are skipped, @see unlist.
+   *
+   * The set walked is fixed when the call begins: a metric created while it runs is not visited.
+   * Enumeration is deliberately the whole store and nothing less. There is no cursor to hold, so
+   * nothing can outlive the walk or name a slot the walk would not visit, and @a func may not
+   * create a metric, which would be an attempt to grow the store from inside a pass over it.
+   */
+  template <typename F>
+  void
+  for_each(F &&func) const
   {
-    auto id = lookup(name);
-
-    if (id == NOT_FOUND) {
-      return end();
-    } else {
-      return iterator(*this, id);
-    }
+    _storage->for_each(std::forward<F>(func));
   }
 
 private:
@@ -337,7 +333,7 @@ class Metrics
       _blobs[0] = std::make_unique<NamesAndAtomics>();
       release_assert(_blobs[0]);
       // Reserve slot 0 for errors, this should always be 0
-      release_assert(0 == create("proxy.process.api.metrics.bad_id", MetricType::COUNTER));
+      release_assert(0 == create(BAD_ID_NAME, MetricType::COUNTER));
     }
 
     ~Storage() {}
@@ -349,6 +345,46 @@ class Metrics
     AtomicType      *lookup(Metrics::IdType id, std::string_view *out_name = nullptr, MetricType *out_type = nullptr) const;
     std::string_view name(IdType id) const;
     MetricType       type(IdType id) const;
+    bool             set_listed(IdType id, bool listed);
+    bool             listed(IdType id) const;
+
+    /** Visit every listed slot, in creation order.
+     *
+     * @see Metrics::for_each, which is how callers reach this.
+     *
+     * The bound is read once, up front. Acquiring it acquires every slot below it, which is what
+     * lets the walk read names and values without the mutex: a slot's name is written before the
+     * release store that publishes it, and never changes.
+     */
+    template <typename F>
+    void
+    for_each(F &&func) const
+    {
+      auto const [last_blob, last_off] = _splitID(next_free_id());
+
+      for (uint16_t blob = 0; blob <= last_blob; ++blob) {
+        NamesAndAtomics const *entries = _blobs[blob].get();
+
+        // The bound covers every blob below it, so this is belt and braces.
+        if (entries == nullptr) {
+          break;
+        }
+
+        uint16_t const limit = blob == last_blob ? last_off : MAX_SIZE;
+
+        for (uint16_t off = 0; off < limit; ++off) {
+          if ((std::get<2>(*entries)[off].load(MEMORY_ORDER) & UNLISTED) != 0) {
+            continue;
+          }
+
+          auto const &slot = std::get<0>(*entries)[off];
+
+          // The type comes from the slot's own id, not from the position, so it is the type the
+          // metric was created with.
+          func(std::string_view{std::get<0>(slot)}, _extractType(std::get<1>(slot)), std::get<1>(*entries)[off].load());
+        }
+      }
+    }
 
     /// The id the next slot will get, which is also iteration's exclusive bound.
     IdType

src/records/RecCore.cc

@@ -585,7 +585,7 @@ RecLookupMatchingRecords(unsigned rec_type, const char *match, void (*callback)(
   if ((rec_type & (RECT_PROCESS | RECT_NODE | RECT_PLUGIN))) {
     // First find the new metrics, this is a bit of a hack, because we still use the old
     // librecords callback with a "pseudo" record.
-    for (auto &&[name, type, val] : ts::Metrics::instance()) {
+    ts::Metrics::instance().for_each([&](std::string_view name, ts::Metrics::MetricType type, int64_t val) {
       if (regex.exec(name.data())) {
         RecRecord tmp{};
 
@@ -596,7 +596,7 @@ RecLookupMatchingRecords(unsigned rec_type, const char *match, void (*callback)(
         tmp.data.rec_int = val;
         callback(&tmp, data);
       }
-    }
+    });
     // Finally check string metrics
     ts::Metrics::StaticString::instance().for_each([&](const std::string &name, const std::string &value) {
       if (regex.exec(name)) {
@@ -617,14 +617,14 @@ RecLookupMatchingRecords(unsigned rec_type, const char *match, void (*callback)(
   if (rec_type & RECT_HIDDEN_METRIC) {
     // Opt-in only: hidden metrics are never reachable through RECT_ALL, see RecDefs.h.
     auto &hidden = ts::Metrics::hidden_instance();
-    // Slot 0 of every Storage is the reserved bad_id placeholder, so it exists under the same name
-    // in both stores. Skip it here, otherwise a query matching it returns two identically named
-    // records that differ only in value.
-    auto it = hidden.begin();
 
-    ++it;
-    for (; it != hidden.end(); ++it) {
-      auto &&[name, type, val] = *it;
+    hidden.for_each([&](std::string_view name, ts::Metrics::MetricType type, int64_t val) {
+      // Slot 0 of every Storage is the reserved bad_id placeholder, so it exists under the same
+      // name in both stores. Skip it here, otherwise a query matching it returns two identically
+      // named records that differ only in value.
+      if (name == ts::Metrics::BAD_ID_NAME) {
+        return;
+      }
 
       if (regex.exec(name.data())) {
         RecRecord tmp{};
@@ -641,7 +641,7 @@ RecLookupMatchingRecords(unsigned rec_type, const char *match, void (*callback)(
         tmp.data.rec_int = val;
         callback(&tmp, data);
       }
-    }
+    });
   }
 
   int num_records = g_num_records;
@@ -968,11 +968,11 @@ RecDumpRecords(RecT rec_type, RecDumpEntryCb callback, void *edata)
   // Dump all new metrics as well (no "type" for them)
   RecData datum;
 
-  for (auto &&[name, type, val] : ts::Metrics::instance()) {
+  ts::Metrics::instance().for_each([&](std::string_view name, Metrics::MetricType type, int64_t val) {
     datum.rec_int = val;
     callback(RECT_PLUGIN, edata, true, name.data(),
              type == Metrics::MetricType::COUNTER ? TS_RECORDDATATYPE_COUNTER : TS_RECORDDATATYPE_INT, &datum);
-  }
+  });
 
   ts::Metrics::StaticString::instance().for_each([&](const std::string &name, const std::string &value) {
     datum.rec_string = const_cast<char *>(value.c_str());