From 321a9709b968e608df7ffda137f9ab0f03c6752f Mon Sep 17 00:00:00 2001 From: Chris McFarlen Date: Wed, 9 Sep 2026 10:23:13 -0500 Subject: [PATCH 1/5] 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/5] 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/5] 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/5] 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/5] 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