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..73453ab7670 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,70 @@ 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 @@ -267,7 +370,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 +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 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..9751c119215 100644 --- a/src/tsutil/unit_tests/test_Metrics.cc +++ b/src/tsutil/unit_tests/test_Metrics.cc @@ -707,3 +707,278 @@ 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. 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]))); + } + + 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") + { + 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") + { + // 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); + } +}