Allow a metric to be unlisted

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

@@ -188,6 +188,46 @@ 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.
+
+``find`` is the exception: it returns ``end()`` for an unlisted metric. Iteration never visits an
+unlisted slot, so an iterator pointing at one would be a range bound that a walk steps straight over
+and never reaches. Use ``lookup`` to read an unlisted metric.
+
+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::
+
+   Iteration is a snapshot taken when the iterator is created. A metric created after ``begin()``
+   is not visited by that iterator.
+
 Storage limits
 ==============
 

include/tsutil/Metrics.h

@@ -94,13 +94,20 @@ class Metrics
   static constexpr int      METRIC_TYPE_MASK = 0x1FFF;
 
 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 +152,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)
   {
@@ -194,15 +252,25 @@ class Metrics
   // Static methods to encapsulate access to the atomic's
   class iterator
   {
+    friend class Metrics;
+
+    /// Tag for the end sentinel, which has no position and reads no storage.
+    struct end_tag {
+    };
+
+    // Only Metrics hands these out, through begin(), end() and find(). A caller that could name an
+    // arbitrary position could name an unlisted one, which iteration must never visit.
+    explicit iterator(const Metrics &m);
+    iterator(const Metrics &m, IdType pos);
+    iterator(const Metrics &m, end_tag);
+
   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++()
     {
@@ -231,43 +299,80 @@ class Metrics
       return std::make_tuple(name, type, metric->_value.load());
     }
 
+    /** Equality.
+     *
+     * Three way rather than a plain position compare: any exhausted iterator equals the end
+     * sentinel, and equals any other exhausted iterator, since two of them may have skipped a
+     * different number of unlisted slots. Two live iterators still compare by position.
+     *
+     * Two positional iterators may hold different snapshots, so exhaustion between them is judged
+     * against the earlier bound. Otherwise a walk could pass its own bound while a stop iterator
+     * made later was still live: they would never compare equal and @c operator++ could not make
+     * progress. The sentinel keeps its own answer, since its bound is meaningless.
+     *
+     * @note A snapshot is the sequence: iterators from different ones are no more comparable than
+     *   iterators into different containers, and mixing them is unspecified. Within one snapshot
+     *   equality is the equivalence relation an input iterator requires. The rule above keeps the
+     *   unspecified case terminating rather than hanging.
+     */
     bool
     operator==(const iterator &o) const
     {
-      return _it == o._it && std::addressof(_metrics) == std::addressof(o._metrics);
-    }
+      if (std::addressof(_metrics) != std::addressof(o._metrics)) {
+        return false;
+      }
 
-    bool
-    operator!=(const iterator &o) const
-    {
-      return _it != o._it || std::addressof(_metrics) != std::addressof(o._metrics);
+      if (_end || o._end) {
+        return at_end() == o.at_end();
+      }
+
+      auto const bound = _bound < o._bound ? _bound : o._bound;
+      bool const a = _it >= bound, b = o._it >= bound;
+
+      if (a || b) {
+        return a && b;
+      }
+      return _it == o._it;
     }
 
   private:
     void next();
+    void advance();
+    void skip_unlisted();
+
+    bool
+    at_end() const
+    {
+      return _end || _it >= _bound;
+    }
 
     const Metrics  &_metrics;
-    Metrics::IdType _it;
+    Metrics::IdType _it{0};
+    /// One past the last slot allocated when this iterator was made. Iteration is a snapshot.
+    Metrics::IdType _bound{0};
+    bool            _end{false};
   };
 
   iterator
   begin() const
   {
-    return iterator(*this, 0);
+    return iterator(*this);
   }
 
   iterator
   end() const
   {
-    return iterator(*this, _storage->next_free_id());
+    return iterator(*this, iterator::end_tag{});
   }
 
   iterator
   find(const std::string_view name) const
   {
     auto id = lookup(name);
 
-    if (id == NOT_FOUND) {
+    // An unlisted slot is never visited by iteration, so handing out an iterator to one would
+    // produce a bound that a skipping walk steps straight over. Reach it with lookup() instead.
+    if (id == NOT_FOUND || !listed(id)) {
       return end();
     } else {
       return iterator(*this, id);
@@ -349,6 +454,8 @@ 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;
 
     /// The id the next slot will get, which is also iteration's exclusive bound.
     IdType

src/records/unit_tests/test_RecHiddenMetricLookup.cc

@@ -115,3 +115,59 @@ TEST_CASE("RecLookupMatchingRecords - hidden metrics", "[librecords][RecLookup][
     }
   }
 }
+
+TEST_CASE("RecLookupMatchingRecords - unlisted metrics", "[librecords][RecLookup][unlisted]")
+{
+  const std::string name = "proxy.test.lookup.unlisted_gauge";
+  auto             *m    = ts::Metrics::Gauge::createPtr(name);
+
+  REQUIRE(m != nullptr);
+  m->store(7);
+
+  auto &metrics = ts::Metrics::instance();
+  auto  id      = metrics.lookup(name);
+
+  REQUIRE(id != ts::Metrics::NOT_FOUND);
+  REQUIRE(metrics.unlist(id));
+
+  SECTION("an unlisted metric is not enumerated")
+  {
+    std::vector<LookupEntry> entries;
+
+    REQUIRE(RecLookupMatchingRecords(RECT_ALL, name.c_str(), collect, &entries) == REC_ERR_OKAY);
+
+    for (const auto &e : entries) {
+      CHECK(e.name != name);
+    }
+  }
+
+  SECTION("an unlisted metric is still found by exact name")
+  {
+    // RecLookupRecord resolves through Metrics::lookup() rather than iteration, which is what keeps
+    // logging fields and TSStatFindName working across an unlisting.
+    std::vector<LookupEntry> entries;
+
+    REQUIRE(RecLookupRecord(name.c_str(), collect, &entries) == REC_ERR_OKAY);
+    REQUIRE(entries.size() == 1);
+    CHECK(entries[0].name == name);
+    CHECK(entries[0].int_value == 7);
+  }
+
+  SECTION("relisting puts it back in enumeration")
+  {
+    REQUIRE(metrics.relist(id));
+
+    std::vector<LookupEntry> entries;
+    bool                     found = false;
+
+    REQUIRE(RecLookupMatchingRecords(RECT_ALL, name.c_str(), collect, &entries) == REC_ERR_OKAY);
+    for (const auto &e : entries) {
+      if (e.name == name) {
+        found = true;
+        CHECK(e.int_value == 7);
+      }
+    }
+
+    REQUIRE(found);
+  }
+}