From 321a9709b968e608df7ffda137f9ab0f03c6752f Mon Sep 17 00:00:00 2001 From: Chris McFarlen Date: Wed, 9 Sep 2026 10:23:13 -0500 Subject: [PATCH 1/6] Allow a metric to be unlisted A metric name, once created, was published for the life of the process. Any metric whose name or publication policy depends on a runtime changeable setting could therefore never retract a name it had already published, so such a setting only ever took effect for names created after the change. Unlisting takes a slot out of the store's listing. It keeps its slot, its name and its atomic, so lookup by name still resolves and creating the name again relists it with its value intact. Iteration skips it, which is what removes it from traffic_ctl, the JSONRPC record lookup and stats_over_http without any of them changing. --- .../internal-libraries/Metrics.en.rst | 40 +++ include/tsutil/Metrics.h | 126 +++++++-- .../unit_tests/test_RecHiddenMetricLookup.cc | 56 ++++ src/tsutil/Metrics.cc | 75 +++++- src/tsutil/unit_tests/test_Metrics.cc | 248 ++++++++++++++++++ 5 files changed, 527 insertions(+), 18 deletions(-) diff --git a/doc/developer-guide/internal-libraries/Metrics.en.rst b/doc/developer-guide/internal-libraries/Metrics.en.rst index 4822170546e..f78af61a40f 100644 --- a/doc/developer-guide/internal-libraries/Metrics.en.rst +++ b/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 ============== diff --git a/include/tsutil/Metrics.h b/include/tsutil/Metrics.h index 741b1e069a5..48666775809 100644 --- a/include/tsutil/Metrics.h +++ b/include/tsutil/Metrics.h @@ -94,13 +94,20 @@ class Metrics static constexpr int METRIC_TYPE_MASK = 0x1FFF; private: - using NameAndId = std::tuple; - using LookupTable = std::unordered_map; - using NameStorage = std::array; - using AtomicStorage = std::array; - using NamesAndAtomics = std::tuple; + using NameAndId = std::tuple; + using LookupTable = std::unordered_map; + using NameStorage = std::array; + using AtomicStorage = std::array; + /// 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, MAX_SIZE>; + using NamesAndAtomics = std::tuple; using BlobStorage = std::array, 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,6 +252,18 @@ 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; @@ -201,8 +271,6 @@ class Metrics using pointer = value_type *; using reference = value_type &; - iterator(const Metrics &m, IdType pos) : _metrics(m), _it(pos) {} - iterator & operator++() { @@ -231,35 +299,55 @@ 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. + */ 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); + bool const a = at_end(), b = o.at_end(); + + 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 @@ -267,7 +355,9 @@ class Metrics { 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 +439,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 diff --git a/src/records/unit_tests/test_RecHiddenMetricLookup.cc b/src/records/unit_tests/test_RecHiddenMetricLookup.cc index d1e2d4c4fcb..c4f6e2e3b16 100644 --- a/src/records/unit_tests/test_RecHiddenMetricLookup.cc +++ b/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 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 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 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); + } +} diff --git a/src/tsutil/Metrics.cc b/src/tsutil/Metrics.cc index 0b9ef8dc5ac..a4eea599402 100644 --- a/src/tsutil/Metrics.cc +++ b/src/tsutil/Metrics.cc @@ -77,6 +77,10 @@ Metrics::Storage::create(std::string_view name, const MetricType type) auto it = _lookups.find(name); if (it != _lookups.end()) { + // Re-creating a name relists it: same slot, same atomic, and whatever value it accumulated + // while it was out of the listing. A name in _lookups always names an allocated slot. + set_listed(it->second, true); + return it->second; } @@ -190,9 +194,61 @@ Metrics::Storage::type(IdType id) const return _extractType(id); } +bool +Metrics::Storage::set_listed(Metrics::IdType id, bool listed) +{ + if (!_is_allocated(id)) { + return false; + } + + auto [blob_ix, offset] = _splitID(id); + Metrics::NamesAndAtomics *blob = _blobs[blob_ix].get(); + + // Only this bit, so a flag added later is not clobbered by unlisting or relisting. + if (listed) { + std::get<2>(*blob)[offset].fetch_and(static_cast(~UNLISTED), MEMORY_ORDER); + } else { + std::get<2>(*blob)[offset].fetch_or(UNLISTED, MEMORY_ORDER); + } + + return true; +} + +bool +Metrics::Storage::listed(Metrics::IdType id) const +{ + if (!_is_allocated(id)) { + return false; + } + + auto [blob_ix, offset] = _splitID(id); + Metrics::NamesAndAtomics *blob = _blobs[blob_ix].get(); + + return (std::get<2>(*blob)[offset].load(MEMORY_ORDER) & UNLISTED) == 0; +} + // Iterator implementation +Metrics::iterator::iterator(const Metrics &m) : _metrics(m), _it(0), _bound(m._storage->next_free_id()) +{ + skip_unlisted(); +} + +Metrics::iterator::iterator(const Metrics &m, IdType pos) : _metrics(m), _bound(m._storage->next_free_id()) +{ + // A metric id carries its type at METRIC_TYPE_BITS, but positions are compared numerically + // against a bound with no type bits. Keep only the blob and offset, as advance() does, or a GAUGE + // id would compare past the end of the store and the iterator would look exhausted. + auto [blob, offset] = _metrics._splitID(pos); + + _it = _makeId(blob, offset, MetricType::COUNTER); + + skip_unlisted(); +} + +Metrics::iterator::iterator(const Metrics &m, end_tag) : _metrics(m), _end(true) {} + void -Metrics::iterator::next() +Metrics::iterator::advance() { auto [blob, offset] = _metrics._splitID(_it); @@ -204,6 +260,23 @@ Metrics::iterator::next() _it = _makeId(blob, offset, MetricType::COUNTER); } +void +Metrics::iterator::skip_unlisted() +{ + // Bounded by the snapshot so a slot created and unlisted after this iterator was made cannot draw + // the scan past the end of what this iterator agreed to visit. + while (!at_end() && !_metrics._storage->listed(_it)) { + advance(); + } +} + +void +Metrics::iterator::next() +{ + advance(); + skip_unlisted(); +} + namespace details { struct DerivedMetric { diff --git a/src/tsutil/unit_tests/test_Metrics.cc b/src/tsutil/unit_tests/test_Metrics.cc index f267097807f..da9012ad1f7 100644 --- a/src/tsutil/unit_tests/test_Metrics.cc +++ b/src/tsutil/unit_tests/test_Metrics.cc @@ -707,3 +707,251 @@ TEST_CASE("Metrics id lookup is safe against concurrent creation", "[libtsapi][M // would mean the sweep above never left the first one. REQUIRE(hi - lo > Metrics::MAX_SIZE); } + +TEST_CASE("Metrics unlisting", "[libtsapi][Metrics]") +{ + auto &m = Metrics::instance(); + + SECTION("an unlisted metric is skipped by iteration") + { + Metrics::Counter::create("unlisted.iter.before"); + auto target = Metrics::Counter::create("unlisted.iter.target"); + Metrics::Counter::create("unlisted.iter.after"); + + REQUIRE(m.unlist(target)); + + bool saw_before = false, saw_target = false, saw_after = false; + + for (auto &&[name, type, value] : m) { + saw_before |= (name == "unlisted.iter.before"); + saw_target |= (name == "unlisted.iter.target"); + saw_after |= (name == "unlisted.iter.after"); + } + + REQUIRE(saw_before); + REQUIRE_FALSE(saw_target); + REQUIRE(saw_after); + } + + SECTION("creating an unlisted name again relists it") + { + auto p = Metrics::Counter::createPtr("unlisted.resurrect"); + auto id = m.lookup("unlisted.resurrect"); + + Metrics::Counter::increment(p, 5); + REQUIRE(m.unlist(id)); + REQUIRE_FALSE(m.listed(id)); + + // Same name, same id, same atomic, and the mark is gone. + auto p2 = Metrics::Counter::createPtr("unlisted.resurrect"); + REQUIRE(p2 == p); + REQUIRE(m.lookup("unlisted.resurrect") == id); + REQUIRE(m.listed(id)); + + // Visible again, with its value intact. + bool found = false; + for (auto &&[name, type, value] : m) { + if (name == "unlisted.resurrect") { + found = true; + REQUIRE(value == 5); + } + } + REQUIRE(found); + } + + SECTION("unlist and relist by name") + { + auto id = Metrics::Counter::create("unlisted.byname"); + + REQUIRE(m.unlist("unlisted.byname")); + REQUIRE_FALSE(m.listed(id)); + + REQUIRE(m.relist("unlisted.byname")); + REQUIRE(m.listed(id)); + + bool found = false; + for (auto &&[name, type, value] : m) { + found |= (name == "unlisted.byname"); + } + REQUIRE(found); + + // A name that was never created cannot be marked. + REQUIRE_FALSE(m.unlist("unlisted.byname.never.created")); + } + + SECTION("an unlisted metric is still resolvable and still counts") + { + auto p = Metrics::Counter::createPtr("unlisted.resolvable"); + auto id = m.lookup("unlisted.resolvable"); + + REQUIRE(m.unlist(id)); + + // Hidden from enumeration is not gone: by name, by id, and through the atomic it is unchanged. + REQUIRE(m.lookup("unlisted.resolvable") == id); + REQUIRE(m.lookup(id) == p); + REQUIRE(m.valid(id)); + REQUIRE(m.name(id) == "unlisted.resolvable"); + REQUIRE(m.type(id) == Metrics::MetricType::COUNTER); + + Metrics::Counter::increment(p, 3); + REQUIRE(Metrics::Counter::load(p) == 3); + } + + SECTION("begin() skips an unlisted first slot") + { + // Slot 0 is the reserved bad_id and is what begin() would otherwise return. + auto bad_id = m.lookup("proxy.process.api.metrics.bad_id"); + REQUIRE(bad_id == 0); + + REQUIRE(m.unlist(bad_id)); + REQUIRE(std::get<0>(*m.begin()) != "proxy.process.api.metrics.bad_id"); + + REQUIRE(m.relist(bad_id)); + REQUIRE(std::get<0>(*m.begin()) == "proxy.process.api.metrics.bad_id"); + } + + SECTION("an unlisted run at the end of the store terminates iteration") + { + // Skipping the last slots in the store is the case where the skip loop has nothing unmarked + // left to land on. + constexpr int COUNT = 8; + std::vector names; + + names.reserve(COUNT); + for (int i = 0; i < COUNT; ++i) { + names.push_back("unlisted.tail." + std::to_string(i)); + REQUIRE(m.unlist(Metrics::Counter::create(names[i]))); + } + + auto count = std::distance(m.begin(), m.end()); + REQUIRE(count > 0); + + for (auto &&[name, type, value] : m) { + for (auto const &n : names) { + REQUIRE(name != n); + } + } + } + + SECTION("iterator comparison") + { + auto a = m.begin(); + auto b = m.begin(); + auto e = m.end(); + + REQUIRE(a == b); + + ++a; + REQUIRE(a != b); // two live iterators still compare by position + + while (a != e) { + ++a; + } + REQUIRE(a == e); // exhausted equals the sentinel + + while (b != e) { + ++b; + } + REQUIRE(b == a); // and equals another exhausted iterator + } + + SECTION("iterating to a bound that is not end()") + { + // A sub-range delimited by a positional iterator has to terminate even when marked slots fall + // inside it. Both ends skip by the same rule, so the walk still lands exactly on the bound. + auto first = Metrics::Counter::create("unlisted.range.1"); + auto skip1 = Metrics::Counter::create("unlisted.range.2"); + auto skip2 = Metrics::Counter::create("unlisted.range.3"); + Metrics::Counter::create("unlisted.range.4"); + Metrics::Counter::create("unlisted.range.5"); + + REQUIRE(m.unlist(skip1)); + REQUIRE(m.unlist(skip2)); + + auto stop = m.find("unlisted.range.5"); + REQUIRE(stop != m.end()); + + std::vector seen; + + for (auto it = m.find("unlisted.range.1"); it != stop; ++it) { + seen.push_back(std::string(std::get<0>(*it))); + REQUIRE(seen.size() <= 4); // do not spin if the bound is never reached + } + + REQUIRE(seen == std::vector{"unlisted.range.1", "unlisted.range.4"}); + REQUIRE(first != Metrics::NOT_FOUND); + } + + SECTION("find() works for a gauge, whose id carries type bits") + { + // A metric id encodes its type at METRIC_TYPE_BITS, while the iteration bound is built with + // COUNTER type bits. Comparing a GAUGE id against that bound numerically makes it look past + // the end of the store. + Metrics::Gauge::createPtr("unlisted.typed.gauge"); + Metrics::Counter::createPtr("unlisted.typed.counter"); + + auto g = m.find("unlisted.typed.gauge"); + REQUIRE(g != m.end()); + REQUIRE(std::get<0>(*g) == "unlisted.typed.gauge"); + REQUIRE(std::get<1>(*g) == Metrics::MetricType::GAUGE); + + auto c = m.find("unlisted.typed.counter"); + REQUIRE(c != m.end()); + REQUIRE(std::get<0>(*c) == "unlisted.typed.counter"); + } + + SECTION("find() on an unlisted metric yields end()") + { + // Iteration never visits a marked slot, so there must be no way to get an iterator that points + // at one. Otherwise using it as a range bound is a walk that never terminates: the skipping + // iterator steps straight over the bound and runs off the end of the store. + auto id = Metrics::Counter::create("unlisted.unfindable"); + + REQUIRE(m.find("unlisted.unfindable") != m.end()); + REQUIRE(m.unlist(id)); + REQUIRE(m.find("unlisted.unfindable") == m.end()); + + // lookup() is the supported way to reach a unlisted metric, and is unaffected. + REQUIRE(m.lookup("unlisted.unfindable") == id); + } + + SECTION("an id that names no allocated slot is neither listed nor unlistable") + { + // Storage::_is_allocated is the gate; this only checks that unlist and listed go through it. + // Blob 100 was never allocated, the largest id names an offset past MAX_SIZE, and create() + // advances after writing so the id one past the last one created is not allocated yet. + auto last = Metrics::Counter::create("unlisted.next.free"); + + for (auto id : {Metrics::IdType{100 << 16}, std::numeric_limits::max(), last + 1}) { + CHECK_FALSE(m.unlist(id)); + CHECK_FALSE(m.listed(id)); + } + } + + SECTION("the hidden store unlists independently") + { + auto &h = Metrics::hidden_instance(); + + Metrics::Counter::createPtr("unlisted.dual"); + Metrics::Counter::createHiddenPtr("unlisted.dual"); + + auto pub_id = m.lookup("unlisted.dual"); + auto hid_id = h.lookup("unlisted.dual"); + + REQUIRE(h.unlist(hid_id)); + REQUIRE_FALSE(h.listed(hid_id)); + REQUIRE(m.listed(pub_id)); + + bool in_published = false, in_hidden = false; + + for (auto &&[name, type, value] : m) { + in_published |= (name == "unlisted.dual"); + } + for (auto &&[name, type, value] : h) { + in_hidden |= (name == "unlisted.dual"); + } + + REQUIRE(in_published); + REQUIRE_FALSE(in_hidden); + } +} From e776b656418d03affedf655fcc4c4601a418db0a Mon Sep 17 00:00:00 2001 From: Chris McFarlen Date: Wed, 9 Sep 2026 11:07:08 -0500 Subject: [PATCH 2/6] Make iterator comparison terminate across snapshots Each iterator captures its own bound, and exhaustion was judged against that. A subrange whose stop iterator was made later held a larger bound, so the walk could pass its own bound and go on comparing unequal to a stop that was still live, with operator++ unable to make progress. Two find() calls with a metric created between them was enough. Exhaustion between two positional iterators is now judged against the earlier of the two bounds, so such a subrange ends at the earlier snapshot. The sentinel keeps its own answer, since its bound means nothing. --- include/tsutil/Metrics.h | 12 +++++++++++- src/tsutil/unit_tests/test_Metrics.cc | 22 ++++++++++++++++++++++ 2 files changed, 33 insertions(+), 1 deletion(-) diff --git a/include/tsutil/Metrics.h b/include/tsutil/Metrics.h index 48666775809..271e9ee63ea 100644 --- a/include/tsutil/Metrics.h +++ b/include/tsutil/Metrics.h @@ -304,6 +304,11 @@ class Metrics * 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. */ bool operator==(const iterator &o) const @@ -312,7 +317,12 @@ class Metrics return false; } - bool const a = at_end(), b = o.at_end(); + 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; diff --git a/src/tsutil/unit_tests/test_Metrics.cc b/src/tsutil/unit_tests/test_Metrics.cc index da9012ad1f7..1c84d3f5da5 100644 --- a/src/tsutil/unit_tests/test_Metrics.cc +++ b/src/tsutil/unit_tests/test_Metrics.cc @@ -882,6 +882,28 @@ TEST_CASE("Metrics unlisting", "[libtsapi][Metrics]") REQUIRE(first != Metrics::NOT_FOUND); } + SECTION("a subrange from iterators made at different times terminates") + { + // Each iterator snapshots its own bound at construction. If exhaustion is judged against each + // one's own bound, the walk can pass its own end while the stop iterator, made later and so + // holding a larger bound, is still live -- they never compare equal and ++ makes no progress. + Metrics::Counter::create("unlisted.snap.start"); + + auto start = m.find("unlisted.snap.start"); + REQUIRE(start != m.end()); + + Metrics::Counter::create("unlisted.snap.stop"); + + auto stop = m.find("unlisted.snap.stop"); + REQUIRE(stop != m.end()); + + int steps = 0; + + for (auto it = start; it != stop; ++it) { + REQUIRE(++steps < 64); // fails rather than spinning if the two never meet + } + } + SECTION("find() works for a gauge, whose id carries type bits") { // A metric id encodes its type at METRIC_TYPE_BITS, while the iteration bound is built with From 1f0f4cacdde314732beb8702f99a929e387aa92c Mon Sep 17 00:00:00 2001 From: Chris McFarlen Date: Wed, 9 Sep 2026 11:07:19 -0500 Subject: [PATCH 3/6] Anchor the unlisted-tail test in its own metric It asserted the store had at least one listed metric left, which depends on what other sections put there. A listed metric of its own says the same thing without that coupling. --- src/tsutil/unit_tests/test_Metrics.cc | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/src/tsutil/unit_tests/test_Metrics.cc b/src/tsutil/unit_tests/test_Metrics.cc index 1c84d3f5da5..9751c119215 100644 --- a/src/tsutil/unit_tests/test_Metrics.cc +++ b/src/tsutil/unit_tests/test_Metrics.cc @@ -813,24 +813,29 @@ TEST_CASE("Metrics unlisting", "[libtsapi][Metrics]") SECTION("an unlisted run at the end of the store terminates iteration") { // Skipping the last slots in the store is the case where the skip loop has nothing unmarked - // left to land on. + // left to land on. The anchor is a listed metric of this section's own, so the loop below is + // known to have run without depending on what other sections left in the shared store. constexpr int COUNT = 8; std::vector names; + Metrics::Counter::create("unlisted.tail.anchor"); + names.reserve(COUNT); for (int i = 0; i < COUNT; ++i) { names.push_back("unlisted.tail." + std::to_string(i)); REQUIRE(m.unlist(Metrics::Counter::create(names[i]))); } - auto count = std::distance(m.begin(), m.end()); - REQUIRE(count > 0); + bool saw_anchor = false; for (auto &&[name, type, value] : m) { + saw_anchor |= (name == "unlisted.tail.anchor"); for (auto const &n : names) { REQUIRE(name != n); } } + + REQUIRE(saw_anchor); } SECTION("iterator comparison") From 03452ff5fe86662a608a10072244a237097d0974 Mon Sep 17 00:00:00 2001 From: Chris McFarlen Date: Wed, 9 Sep 2026 14:21:45 -0500 Subject: [PATCH 4/6] Say which iterator comparisons are meaningful Exhaustion is a property of an iterator's own snapshot bound, so two taken at different times can compare equal to each other while disagreeing about end. That is not a total equivalence relation, which makes these unfit for a generic algorithm; only same snapshot comparisons, and comparison against end, are meaningful. --- include/tsutil/Metrics.h | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/include/tsutil/Metrics.h b/include/tsutil/Metrics.h index 271e9ee63ea..2d7dfc7a3ad 100644 --- a/include/tsutil/Metrics.h +++ b/include/tsutil/Metrics.h @@ -309,6 +309,12 @@ class Metrics * 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 Only iterators taken from the same snapshot are meaningfully comparable with each + * other. Because exhaustion is a property of an iterator's own bound, two taken at different + * times can compare equal to each other while disagreeing about @c end, so this is not a + * total equivalence relation and these are not iterators to hand to a generic algorithm. + * Use @c end to test for exhaustion. */ bool operator==(const iterator &o) const From 0d16232576b1dcabeb592d3690155af0fc592359 Mon Sep 17 00:00:00 2001 From: Chris McFarlen Date: Wed, 9 Sep 2026 16:59:31 -0500 Subject: [PATCH 5/6] Frame the snapshot as the sequence, not as a caveat The previous note disclaimed the equivalence relation while the type still declared input_iterator_tag, which advertises what it then denied. A snapshot is the sequence: iterators from different ones are no more comparable than iterators into different containers, so mixing them is unspecified rather than broken, and within one snapshot equality is the relation an input iterator requires. --- include/tsutil/Metrics.h | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/include/tsutil/Metrics.h b/include/tsutil/Metrics.h index 2d7dfc7a3ad..73453ab7670 100644 --- a/include/tsutil/Metrics.h +++ b/include/tsutil/Metrics.h @@ -310,11 +310,10 @@ class Metrics * 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 Only iterators taken from the same snapshot are meaningfully comparable with each - * other. Because exhaustion is a property of an iterator's own bound, two taken at different - * times can compare equal to each other while disagreeing about @c end, so this is not a - * total equivalence relation and these are not iterators to hand to a generic algorithm. - * Use @c end to test for exhaustion. + * @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 From f340f143681b31711cdd5674b6b3224639f6126b Mon Sep 17 00:00:00 2001 From: Chris McFarlen Date: Mon, 14 Sep 2026 10:08:53 -0700 Subject: [PATCH 6/6] Replace metric iteration with for_each A public iterator lets a caller name a position, and a position stops meaning anything once iteration skips unlisted slots. An iterator held at a slot that is later unlisted becomes a range bound the walk steps straight over and never reaches, and find() could hand back the next listed metric rather than the one asked for. Supporting either would mean defining iterator invalidation for listing changes, to keep a surface with no callers: every consumer walks the whole store, and find() had none at all. for_each is the whole store or nothing. With no cursor to outlive the walk, the equality rules, the snapshot bound comparison and find() go away along with the defects they carried. lookup() remains the way to reach a single metric by name. --- .../internal-libraries/Metrics.en.rst | 26 +- include/tsutil/Metrics.h | 185 +++++--------- src/records/RecCore.cc | 24 +- src/tsutil/Metrics.cc | 54 +---- src/tsutil/unit_tests/test_Metrics.cc | 225 ++++++------------ 5 files changed, 170 insertions(+), 344 deletions(-) diff --git a/doc/developer-guide/internal-libraries/Metrics.en.rst b/doc/developer-guide/internal-libraries/Metrics.en.rst index f78af61a40f..ba68d1a9df1 100644 --- a/doc/developer-guide/internal-libraries/Metrics.en.rst +++ b/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 =============== @@ -210,10 +228,6 @@ metric still resolves through ``lookup``, so an exact name query, a logging fiel 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. @@ -225,8 +239,8 @@ runtime changeable setting needs a way to retract a name it has already publishe .. note:: - Iteration is a snapshot taken when the iterator is created. A metric created after ``begin()`` - is not visited by that iterator. + The set walked is fixed when ``for_each`` begins, so a metric created while it runs is not + visited. Storage limits ============== diff --git a/include/tsutil/Metrics.h b/include/tsutil/Metrics.h index 73453ab7670..344b3cd892b 100644 --- a/include/tsutil/Metrics.h +++ b/include/tsutil/Metrics.h @@ -93,6 +93,10 @@ 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; using LookupTable = std::unordered_map; @@ -249,134 +253,21 @@ class Metrics return _storage->valid(id); } - // 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; - using difference_type = ptrdiff_t; - using pointer = value_type *; - using reference = value_type &; - - 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()); - } - - /** 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 - { - if (std::addressof(_metrics) != std::addressof(o._metrics)) { - return false; - } - - 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{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); - } - - iterator - end() const + /** Visit every listed metric. + * + * @a func is called as func(std::string_view name, MetricType type, int64_t value) 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 + void + for_each(F &&func) const { - return iterator(*this, iterator::end_tag{}); - } - - iterator - find(const std::string_view name) const - { - auto id = lookup(name); - - // 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); - } + _storage->for_each(std::forward(func)); } private: @@ -442,7 +333,7 @@ class Metrics _blobs[0] = std::make_unique(); 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() {} @@ -457,6 +348,44 @@ class Metrics 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 + 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 next_free_id() const diff --git a/src/records/RecCore.cc b/src/records/RecCore.cc index 91dd163717c..36ffa3ea489 100644 --- a/src/records/RecCore.cc +++ b/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(value.c_str()); diff --git a/src/tsutil/Metrics.cc b/src/tsutil/Metrics.cc index a4eea599402..83c4c8d19a3 100644 --- a/src/tsutil/Metrics.cc +++ b/src/tsutil/Metrics.cc @@ -141,8 +141,8 @@ Metrics::Storage::lookup(Metrics::IdType id, std::string_view *out_name, Metrics } if (out_type) { - // don't trust the passed in id to get the type as it might have been manufactured (i.e. from iterators) - // so get the type from the storage tuple. + // don't trust the passed in id to get the type as it might have been manufactured, so get the + // type from the storage tuple. *out_type = _extractType(std::get<1>(std::get<0>(*blob)[offset])); } @@ -227,56 +227,6 @@ Metrics::Storage::listed(Metrics::IdType id) const return (std::get<2>(*blob)[offset].load(MEMORY_ORDER) & UNLISTED) == 0; } -// Iterator implementation -Metrics::iterator::iterator(const Metrics &m) : _metrics(m), _it(0), _bound(m._storage->next_free_id()) -{ - skip_unlisted(); -} - -Metrics::iterator::iterator(const Metrics &m, IdType pos) : _metrics(m), _bound(m._storage->next_free_id()) -{ - // A metric id carries its type at METRIC_TYPE_BITS, but positions are compared numerically - // against a bound with no type bits. Keep only the blob and offset, as advance() does, or a GAUGE - // id would compare past the end of the store and the iterator would look exhausted. - auto [blob, offset] = _metrics._splitID(pos); - - _it = _makeId(blob, offset, MetricType::COUNTER); - - skip_unlisted(); -} - -Metrics::iterator::iterator(const Metrics &m, end_tag) : _metrics(m), _end(true) {} - -void -Metrics::iterator::advance() -{ - auto [blob, offset] = _metrics._splitID(_it); - - if (++offset == MAX_SIZE) { - ++blob; - offset = 0; - } - - _it = _makeId(blob, offset, MetricType::COUNTER); -} - -void -Metrics::iterator::skip_unlisted() -{ - // Bounded by the snapshot so a slot created and unlisted after this iterator was made cannot draw - // the scan past the end of what this iterator agreed to visit. - while (!at_end() && !_metrics._storage->listed(_it)) { - advance(); - } -} - -void -Metrics::iterator::next() -{ - advance(); - skip_unlisted(); -} - namespace details { struct DerivedMetric { diff --git a/src/tsutil/unit_tests/test_Metrics.cc b/src/tsutil/unit_tests/test_Metrics.cc index 9751c119215..639eeeaf175 100644 --- a/src/tsutil/unit_tests/test_Metrics.cc +++ b/src/tsutil/unit_tests/test_Metrics.cc @@ -26,7 +26,6 @@ #include #include #include -#include #include #include #include @@ -40,33 +39,37 @@ TEST_CASE("Metrics", "[libtsapi][Metrics]") { auto &m = Metrics::instance(); - SECTION("iterator") + SECTION("for_each") { - auto [name, type, value] = *m.begin(); - REQUIRE(value == 0); - REQUIRE(type == Metrics::MetricType::COUNTER); - REQUIRE(name == "proxy.process.api.metrics.bad_id"); + std::vector names; + int64_t first_value = -1; + Metrics::MetricType first_type{}; + + m.for_each([&](std::string_view name, Metrics::MetricType type, int64_t value) { + if (names.empty()) { + first_value = value; + first_type = type; + } + names.emplace_back(name); + }); - REQUIRE(m.begin() != m.end()); + // The reserved bad_id occupies the first slot of every store, so it is always visited first. + REQUIRE_FALSE(names.empty()); + REQUIRE(names.front() == Metrics::BAD_ID_NAME); + REQUIRE(first_value == 0); + REQUIRE(first_type == Metrics::MetricType::COUNTER); - // Other test cases share this process-wide store, so the number of metrics already present - // is not knowable here. Assert the delta from creating one metric instead of an absolute - // iterator position. - auto pre_count = std::distance(m.begin(), m.end()); + // Other test cases share this process-wide store, so the number of metrics already present is + // not knowable here. Assert the delta from creating one metric instead of an absolute count. + auto const pre_count = names.size(); - Metrics::Counter::create("iterator.marker"); - REQUIRE(std::distance(m.begin(), m.end()) == pre_count + 1); + Metrics::Counter::create("for_each.marker"); - auto it = m.begin(); - std::advance(it, pre_count); - REQUIRE(it != m.end()); - ++it; - REQUIRE(it == m.end()); + names.clear(); + m.for_each([&](std::string_view name, Metrics::MetricType, int64_t) { names.emplace_back(name); }); - auto it2 = m.begin(); - std::advance(it2, pre_count); - it2++; - REQUIRE(it2 == m.end()); + REQUIRE(names.size() == pre_count + 1); + REQUIRE(names.back() == "for_each.marker"); // creation order, so the newest is last } SECTION("New metric") @@ -437,18 +440,12 @@ TEST_CASE("Metrics hidden store", "[libtsapi][Metrics]") // Not visible in the published store, by name or by iteration. REQUIRE(m.lookup("hidden.only") == Metrics::NOT_FOUND); - for (auto &&[name, type, value] : m) { - REQUIRE(name != "hidden.only"); - } + m.for_each([](std::string_view name, Metrics::MetricType, int64_t) { REQUIRE(name != "hidden.only"); }); // Visible in the hidden store. REQUIRE(h.lookup("hidden.only") != Metrics::NOT_FOUND); bool found = false; - for (auto &&[name, type, value] : h) { - if (name == "hidden.only") { - found = true; - } - } + h.for_each([&](std::string_view name, Metrics::MetricType, int64_t) { found |= (name == "hidden.only"); }); REQUIRE(found); } @@ -722,11 +719,11 @@ TEST_CASE("Metrics unlisting", "[libtsapi][Metrics]") bool saw_before = false, saw_target = false, saw_after = false; - for (auto &&[name, type, value] : m) { + m.for_each([&](std::string_view name, Metrics::MetricType, int64_t) { saw_before |= (name == "unlisted.iter.before"); saw_target |= (name == "unlisted.iter.target"); saw_after |= (name == "unlisted.iter.after"); - } + }); REQUIRE(saw_before); REQUIRE_FALSE(saw_target); @@ -750,12 +747,12 @@ TEST_CASE("Metrics unlisting", "[libtsapi][Metrics]") // Visible again, with its value intact. bool found = false; - for (auto &&[name, type, value] : m) { + m.for_each([&](std::string_view name, Metrics::MetricType, int64_t value) { if (name == "unlisted.resurrect") { found = true; REQUIRE(value == 5); } - } + }); REQUIRE(found); } @@ -770,9 +767,7 @@ TEST_CASE("Metrics unlisting", "[libtsapi][Metrics]") REQUIRE(m.listed(id)); bool found = false; - for (auto &&[name, type, value] : m) { - found |= (name == "unlisted.byname"); - } + m.for_each([&](std::string_view name, Metrics::MetricType, int64_t) { found |= (name == "unlisted.byname"); }); REQUIRE(found); // A name that was never created cannot be marked. @@ -797,17 +792,40 @@ TEST_CASE("Metrics unlisting", "[libtsapi][Metrics]") REQUIRE(Metrics::Counter::load(p) == 3); } - SECTION("begin() skips an unlisted first slot") + SECTION("for_each skips an unlisted first slot") { - // Slot 0 is the reserved bad_id and is what begin() would otherwise return. - auto bad_id = m.lookup("proxy.process.api.metrics.bad_id"); + // Slot 0 is the reserved bad_id, so it is the first slot the walk considers. The anchor keeps + // the assertions below from passing on an empty walk. + auto bad_id = m.lookup(Metrics::BAD_ID_NAME); REQUIRE(bad_id == 0); + Metrics::Counter::create("unlisted.first.anchor"); + + auto first_name = [&]() { + std::string first; + bool seen = false; + + m.for_each([&](std::string_view name, Metrics::MetricType, int64_t) { + if (!seen) { + first = name; + seen = true; + } + }); + + return first; + }; + REQUIRE(m.unlist(bad_id)); - REQUIRE(std::get<0>(*m.begin()) != "proxy.process.api.metrics.bad_id"); + auto const while_unlisted = first_name(); + // Relist before asserting: a failed assertion ends the section, and leaving bad_id unlisted + // would break every later test case that expects to see it. REQUIRE(m.relist(bad_id)); - REQUIRE(std::get<0>(*m.begin()) == "proxy.process.api.metrics.bad_id"); + auto const while_listed = first_name(); + + REQUIRE_FALSE(while_unlisted.empty()); // the walk did visit something + REQUIRE(while_unlisted != Metrics::BAD_ID_NAME); + REQUIRE(while_listed == Metrics::BAD_ID_NAME); } SECTION("an unlisted run at the end of the store terminates iteration") @@ -828,118 +846,37 @@ TEST_CASE("Metrics unlisting", "[libtsapi][Metrics]") bool saw_anchor = false; - for (auto &&[name, type, value] : m) { + m.for_each([&](std::string_view name, Metrics::MetricType, int64_t) { saw_anchor |= (name == "unlisted.tail.anchor"); for (auto const &n : names) { REQUIRE(name != n); } - } + }); REQUIRE(saw_anchor); } - SECTION("iterator comparison") - { - auto a = m.begin(); - auto b = m.begin(); - auto e = m.end(); - - REQUIRE(a == b); - - ++a; - REQUIRE(a != b); // two live iterators still compare by position - - while (a != e) { - ++a; - } - REQUIRE(a == e); // exhausted equals the sentinel - - while (b != e) { - ++b; - } - REQUIRE(b == a); // and equals another exhausted iterator - } - - SECTION("iterating to a bound that is not end()") - { - // A sub-range delimited by a positional iterator has to terminate even when marked slots fall - // inside it. Both ends skip by the same rule, so the walk still lands exactly on the bound. - auto first = Metrics::Counter::create("unlisted.range.1"); - auto skip1 = Metrics::Counter::create("unlisted.range.2"); - auto skip2 = Metrics::Counter::create("unlisted.range.3"); - Metrics::Counter::create("unlisted.range.4"); - Metrics::Counter::create("unlisted.range.5"); - - REQUIRE(m.unlist(skip1)); - REQUIRE(m.unlist(skip2)); - - auto stop = m.find("unlisted.range.5"); - REQUIRE(stop != m.end()); - - std::vector seen; - - for (auto it = m.find("unlisted.range.1"); it != stop; ++it) { - seen.push_back(std::string(std::get<0>(*it))); - REQUIRE(seen.size() <= 4); // do not spin if the bound is never reached - } - - REQUIRE(seen == std::vector{"unlisted.range.1", "unlisted.range.4"}); - REQUIRE(first != Metrics::NOT_FOUND); - } - - SECTION("a subrange from iterators made at different times terminates") - { - // Each iterator snapshots its own bound at construction. If exhaustion is judged against each - // one's own bound, the walk can pass its own end while the stop iterator, made later and so - // holding a larger bound, is still live -- they never compare equal and ++ makes no progress. - Metrics::Counter::create("unlisted.snap.start"); - - auto start = m.find("unlisted.snap.start"); - REQUIRE(start != m.end()); - - Metrics::Counter::create("unlisted.snap.stop"); - - auto stop = m.find("unlisted.snap.stop"); - REQUIRE(stop != m.end()); - - int steps = 0; - - for (auto it = start; it != stop; ++it) { - REQUIRE(++steps < 64); // fails rather than spinning if the two never meet - } - } - - SECTION("find() works for a gauge, whose id carries type bits") + SECTION("for_each reports the type each metric was created with") { - // A metric id encodes its type at METRIC_TYPE_BITS, while the iteration bound is built with - // COUNTER type bits. Comparing a GAUGE id against that bound numerically makes it look past - // the end of the store. + // The type comes from the slot's own stored id rather than from the walk's position, which is + // what keeps a gauge from being reported as a counter. Metrics::Gauge::createPtr("unlisted.typed.gauge"); Metrics::Counter::createPtr("unlisted.typed.counter"); - auto g = m.find("unlisted.typed.gauge"); - REQUIRE(g != m.end()); - REQUIRE(std::get<0>(*g) == "unlisted.typed.gauge"); - REQUIRE(std::get<1>(*g) == Metrics::MetricType::GAUGE); - - auto c = m.find("unlisted.typed.counter"); - REQUIRE(c != m.end()); - REQUIRE(std::get<0>(*c) == "unlisted.typed.counter"); - } + bool saw_gauge = false, saw_counter = false; - SECTION("find() on an unlisted metric yields end()") - { - // Iteration never visits a marked slot, so there must be no way to get an iterator that points - // at one. Otherwise using it as a range bound is a walk that never terminates: the skipping - // iterator steps straight over the bound and runs off the end of the store. - auto id = Metrics::Counter::create("unlisted.unfindable"); - - REQUIRE(m.find("unlisted.unfindable") != m.end()); - REQUIRE(m.unlist(id)); - REQUIRE(m.find("unlisted.unfindable") == m.end()); + m.for_each([&](std::string_view name, Metrics::MetricType type, int64_t) { + if (name == "unlisted.typed.gauge") { + saw_gauge = true; + REQUIRE(type == Metrics::MetricType::GAUGE); + } else if (name == "unlisted.typed.counter") { + saw_counter = true; + REQUIRE(type == Metrics::MetricType::COUNTER); + } + }); - // lookup() is the supported way to reach a unlisted metric, and is unaffected. - REQUIRE(m.lookup("unlisted.unfindable") == id); + REQUIRE(saw_gauge); + REQUIRE(saw_counter); } SECTION("an id that names no allocated slot is neither listed nor unlistable") @@ -971,12 +908,8 @@ TEST_CASE("Metrics unlisting", "[libtsapi][Metrics]") bool in_published = false, in_hidden = false; - for (auto &&[name, type, value] : m) { - in_published |= (name == "unlisted.dual"); - } - for (auto &&[name, type, value] : h) { - in_hidden |= (name == "unlisted.dual"); - } + m.for_each([&](std::string_view name, Metrics::MetricType, int64_t) { in_published |= (name == "unlisted.dual"); }); + h.for_each([&](std::string_view name, Metrics::MetricType, int64_t) { in_hidden |= (name == "unlisted.dual"); }); REQUIRE(in_published); REQUIRE_FALSE(in_hidden);