Allow a metric to be unlisted

[cmcfarlen]

Problem

ts::Metrics::Storage has no removal path. create() allocates a slot and a name and nothing ever undoes either, so a metric name lives for the life of the process.

Any code that decides whether to publish a name based on a runtime changeable input therefore makes a permanent commitment the first time it publishes. The decision is latched at first creation and can never be revisited.

The case that surfaced this is the per upstream server connection metrics from #13506. proxy.config.http.per_server.connection.metric_aggregate is RECU_DYNAMIC and overridable, and at value 2 the per group <fqdn>.<ip>:<port> metrics are supposed to stay hidden while only the per hostname aggregates are published. On a box that ran for a while at 0 before being switched to 2, both shapes are present in traffic_ctl metric match per_server, and no reload can remove the first set. The config change took effect correctly for everything created after it; the names created before it simply cannot be withdrawn.

What this does

Lets a metric be taken out of the store's listing.

auto &m = ts::Metrics::instance();

m.unlist(id);                       // by id
m.unlist("proxy.process.example");  // or by name
m.relist(id);                       // put it back

if (m.listed(id)) { ... }

An unlisted metric:

• is skipped by iteration, so it disappears from traffic_ctl metric match, the JSONRPC record lookup and stats_over_http with no change in any of those consumers;
• still resolves by exact name through lookup(), so RecLookupRecord, LogAccess field resolution and TSStatFindName keep working;
• keeps its atomic, which may still be read and written, so a Derived aggregate sourcing from it is unaffected;
• is relisted by create() on the same name, returning the same id with its accumulated value intact.

An unlisted phone number is the analogy: not in the directory, but it still rings if you know it. This is a publication policy, not a lifetime — any IdType or AtomicType * a caller already holds stays valid across an unlist and relist.

This PR adds the mechanism only. Nothing in the tree calls it, so every existing metric enumerates exactly as before. The ConnectionTracker fix is a follow up.

On the naming

This started out called tombstone, which was wrong twice over. A tombstone elsewhere is a record that something was deleted, and this codebase already uses it that way — CacheShm tombstones a slot to mark it dead and reusable. Nothing is deleted here.

hide / publish would read best in isolation but both words are already load bearing for a different mechanism in this same class: the two separate stores, hidden_instance() and createHiddenPtr() versus the published store. An unlisted metric in the published store would have been "published but not published".

listed collides with neither, and says the useful part out loud.

Implementation notes

Storage. A parallel FlagStorage array in the blob, rather than a member of NameAndId: an std::atomic member would make that tuple neither copyable nor movable, and the slot is written with a tuple assignment. Blobs are already built with make_unique, which value initializes, so flags start zero with no change to addBlob(). Reads are lock free at relaxed ordering, matching the rest of the class. Cost is 1 KiB per 1024 slots against a blob that is already about 48 KiB. The single UNLISTED bit is set and cleared with fetch_or / fetch_and rather than a whole word store, so a flag added later is not clobbered.

Id validation. Storage::allocated() gates both entry points. It is deliberately stricter than the existing valid(): the offset is the low 16 bits of an id and so can name a slot past MAX_SIZE in a blob that is full, and _cur_off is the next free slot, so valid() accepts one slot that does not exist yet. That second case is not theoretical — a test that marked the free slot caused an unrelated metric created later in the same run to come out invisible, because create() only clears the flag when it finds the name already present, not when it allocates a fresh slot.

A static_assert now ties MAX_BLOBS to the mask _splitID applies to the blob index. That relationship is what keeps every _blobs[] subscript in this class in range without an explicit check, and nothing previously enforced it.

Iterator. The skip loop needs an end bound, and deriving it from end() per element would take the storage mutex per element. Instead the iterator captures the bound once at construction and end() becomes a pure sentinel that reads no storage — strictly less locking than before, where end() locked on every construction. Iteration is now explicitly a snapshot taken at begin(): a metric created mid iteration is never seen rather than sometimes seen, which matches the reasoning already recorded in RecLookupRecord about find()/end() racing with concurrent registration.

operator== is three way. 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, unchanged. The hand written operator!= is removed in favor of the C++20 synthesized one.

The three constructors are private to Metrics. A caller able to name an arbitrary position could name an unlisted one, which iteration must never visit and which does not terminate a range walk. begin(), end() and find() are the only ways to obtain an iterator, and find() returns end() for an unlisted metric — use lookup() to read one.

API change

Metrics::iterator's constructors become private, Metrics a friend. The positional constructor,
iterator(const Metrics &, IdType), was previously public, so this is a source level break for
anything constructing an iterator directly. Nothing in tree does, and there is no reason to: a
caller with an id wants lookup, not an iterator over one position.

Keeping it public is what allowed the two iterator defects review found here — a gauge id compared
past the bound, and a subrange from two snapshots that did not terminate. Both come from being able
to name an arbitrary position. Private constructors are what make "iteration never visits an
unlisted slot, and a snapshot is the sequence" enforceable rather than advisory.

Worth a line in the 11.0.0 release notes alongside the createSpan and rename removals from
#13583.

Tests

test_Metrics.cc: skipped by iteration, still resolvable by name and id, relisted by create(), unlist and relist by name, begin() skipping an unlisted first slot, an unlisted run at the end of the store, iterating to a bound that is not end() with unlisted slots inside the range, find() yielding end(), iterator comparison, independence between the published and hidden stores, and four rejected id shapes: an unallocated blob, an offset past the end of a full blob, the next free slot, and the largest possible id.

test_RecHiddenMetricLookup.cc: an unlisted metric is not enumerated by RecLookupMatchingRecords, is still found by RecLookupRecord, and returns to enumeration when relisted.

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

[Copilot]

🟡 Changes recommended

The new tombstone and iterator code paths need additional defensive validation and invariant enforcement to avoid incorrect behavior or potential out-of-bounds access on manufactured/invalid IDs.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

This PR adds a “tombstone” mechanism to ts::Metrics so a metric can be withdrawn from publication (hidden from iteration/enum-based consumers) while remaining resolvable by exact name / id and keeping its backing atomic/value.

Changes:

• Extend ts::Metrics::Storage with a per-slot flag array and public tombstone() / tombstoned() APIs.
• Update ts::Metrics iteration semantics to skip tombstoned slots and to use a snapshot bound captured at iterator construction.
• Add unit tests covering tombstoning behavior across both ts::Metrics and records lookup, plus documentation updates.

File summaries

File	Description
src/tsutil/Metrics.cc	Implements tombstone flagging and iterator behavior changes (snapshot bound + skip).
include/tsutil/Metrics.h	Exposes the tombstone API, adds per-slot flag storage, and updates iterator semantics/contracts.
src/tsutil/unit_tests/test_Metrics.cc	Adds coverage for tombstone behavior, iteration skipping, resurrection, and edge cases.
src/records/unit_tests/test_RecHiddenMetricLookup.cc	Verifies record lookup behavior with tombstoned metrics (enumeration vs exact lookup).
doc/developer-guide/internal-libraries/Metrics.en.rst	Documents the tombstone feature and its interaction with find()/iteration.

Review details

Suppressed comments (2)

src/tsutil/Metrics.cc:288

• Storage::tombstoned() indexes the per-slot flag array with offset without validating that offset < MAX_SIZE (or that the slot is allocated in the current blob). A manufactured/invalid IdType with a large offset can trigger out-of-bounds access; it should safely return false for non-allocated/non-sensical IDs.

Metrics::Storage::tombstoned(Metrics::IdType id) const
{
  auto [blob_ix, offset]         = _splitID(id);
  Metrics::NamesAndAtomics *blob = _blobs[blob_ix].get();

  if (!blob) {
    return false;
  }

  return (std::get<2>(*blob)[offset].load(MEMORY_ORDER) & TOMBSTONE) != 0;
}

src/tsutil/Metrics.cc:296

• The positional iterator ctor iterator(const Metrics&, IdType) does not call skip_tombstoned(). That allows external callers to construct an iterator that points at a tombstoned slot (contradicting the intended "iteration never visits marked slots" invariant) and reintroduces the non-terminating range-walk risk if such an iterator is used as a bound.

Metrics::iterator::iterator(const Metrics &m, IdType pos) : _metrics(m), _it(pos), _bound(m._storage->current_id()) {}

• Files reviewed: 5/5 changed files
• Comments generated: 1
• Review effort level: Lite

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[cmcfarlen]

Thanks — all three findings were real, including the two that were filed as suppressed comments. Fixed in 9ed3ee8.

tombstone() / tombstoned() accepting ids that name no allocated slot. The bound check was blob_ix == _cur_blob && offset > _cur_off, which only constrains the current blob. For any blob below it the offset is the low 16 bits of the id and so can reach 65535 against a 1024-entry array, and tombstone() writes through it. Both now go through a new Storage::allocated():

bool
allocated(IdType id) const
{
  auto [blob, entry] = _splitID(id);

  if (id < 0 || entry >= MAX_SIZE || !_blobs[blob]) {
    return false;
  }

  return blob < _cur_blob || (blob == _cur_blob && entry < _cur_off);
}

This is deliberately stricter than the existing valid() in the second way you noted as well: _cur_off is the next free slot, so valid() accepts one slot that does not exist yet. That mattered more than I expected. A new test marked the free slot, and an unrelated metric created in a later test section landed in it and came out invisible, because create() only clears the flag when it finds the name already present, not when it allocates a fresh slot. So the "harmless" case was a real way to lose a metric.

I left valid() alone rather than tightening it here — other callers depend on its current semantics and that is a separate change.

The positional iterator constructor. You are right that it let a caller rest an iterator on a tombstoned slot, which reintroduces the non-terminating range-walk. Rather than only skipping, the three constructors are now private with friend class Metrics, so begin(), end() and find() are the only ways to obtain one. find() already resolves a tombstoned name to end(). The positional constructor also skips now, so the invariant holds for any future in-class use.

New tests covering each case: an offset past the end of a full blob, the next free slot, and a blob index that was never allocated.

[cmcfarlen]

Follow-up on a review question: allocated() indexes _blobs with a blob index taken from a caller-supplied id and does not range check it.

That is in fact safe, but only by coincidence. _splitID masks the blob index with METRIC_TYPE_MASK (0x1FFF), and MAX_BLOBS is 8192, so the index is always a valid _blobs subscript by construction rather than by a check. Nothing in the code tied those two constants together, and every other accessor that splits an id — lookup, name, rename, valid — depends on the same relationship.

b4fee22 makes it explicit:

static_assert(MAX_BLOBS == METRIC_TYPE_MASK + 1, "a masked blob index must always be a valid _blobs index");

Verified it fires: dropping MAX_BLOBS to 4096 fails the build rather than silently producing out-of-range subscripts throughout the class.

Also added a test for the largest possible id, which exercises the other half — the offset is not masked to the blob size, so it needs the explicit entry >= MAX_SIZE check to avoid running off the end of a blob.

Worth noting for a possible follow-up, out of scope here: METRIC_TYPE_MASK is misnamed. It has exactly one use, masking the blob index in _splitID, and has nothing to do with the metric type, which lives at METRIC_TYPE_BITS. Renaming it would be a one-line change but it is a public constant, so I left it alone.

[Copilot]

Warning

Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.

[Copilot]

Warning

Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.

[Copilot]

🔵 Needs a closer look

It changes core metrics storage/iteration semantics used broadly across ATS, and needs human validation with full CI results and concurrency/compatibility scrutiny.

Review details

• Files reviewed: 5/5 changed files
• Comments generated: 0 new
• Review effort level: Lite

[cmcfarlen]

Went through all six. Two were already addressed, three are the same point and are now fixed, and one is incorrect — details below.

Fixed in ef97aa6

create() reimplementing the flag clear, and the duplicated ~UNLISTED mask (three of the comments): create() now calls set_listed(it->second, true), so the mask appears in exactly one place and there is one code path for relisting. On the null-blob concern: a name present in _lookups always names an allocated slot, so neither the old inline check nor the new allocated() gate can fail there — but routing through one function means the failure modes cannot drift apart, which was the substance of the comment.

current() returning std::pair<int16_t, int16_t> built from two uint16_t members is now std::pair<uint16_t, uint16_t>. The values are small enough that the narrowing never mattered, but see below for why it was worth removing.

Not a defect: the negative-offset concern

entry comes from _splitID(id) as an int16_t (low 16 bits). If those low bits are >= 0x8000, entry becomes negative and will pass entry >= MAX_SIZE

_splitID returns std::tuple<uint16_t, uint16_t>, so entry is unsigned. Low bits of 0xFFFF give entry == 65535, which fails entry >= MAX_SIZE and is rejected. There is no signed value and no negative subscript. The existing test for std::numeric_limits<IdType>::max() covers exactly this input — its offset bits are 0xFFFF, high bit set — and asserts both unlist and listed return false.

The int16_t in the comment is real, though, just in a different function: current(), which is what I changed above. That is very likely where the reading came from, so removing the narrowing is worth it even though allocated() was never affected.

Already addressed

The Storage::tombstone() allocated-slot comment was fixed in 9ed3ee8, which is what introduced allocated().

The PR description mismatch was real when filed; the description now documents unlist/relist/listed and includes a section on why the name changed. No compatibility alias: tombstone was never released, and it is the wrong word for this — a tombstone marks something deleted and reusable, which is how CacheShm in this same tree uses it. Nothing is deleted here.

[Copilot]

🟡 Changes recommended

The new per-slot atomic flag storage needs explicit initialization to avoid nondeterministic “unlisted” state, and one unit-test section can dereference begin() after making the store empty (undefined behavior).

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

src/tsutil/unit_tests/test_Metrics.cc:740

• This section dereferences *m.begin() after unlisting id 0; if this TEST_CASE path runs before any other metric is created, begin()==end() and dereferencing is undefined. Create a guaranteed-listed metric first so begin() is always safe to dereference.

• Files reviewed: 5/5 changed files
• Comments generated: 1
• Review effort level: Lite

[cmcfarlen]

FlagStorage uses std::atomic<uint8_t>, whose default constructor does not initialize the stored value.

That was true through C++17, but P0883 changed std::atomic's default constructor to value-initialize the contained value in C++20, and this tree builds -std=c++20.

It also does not depend on that change. addBlob() allocates with std::make_unique<Metrics::NamesAndAtomics>(), which is new T() — value-initialization — and that recurses through the tuple and the std::array to every element. Under the pre-C++20 rules the atomic's defaulted default constructor was trivial, so value-initialization zero-initialized its storage anyway. Both readings give a zero flag byte, so a new slot starts listed.

I would rather not argue that from the standard, so 02d30f9 asserts it instead. The blob growth test already creates MAX_SIZE + 100 metrics, which spans a blob boundary; it now also requires listed(id) for every one of them, so a full blob's worth of freshly allocated slots is checked.

Confirmed the assertion is not vacuous. Storing 0xFF into the flag array immediately after the allocation fails it:

test_Metrics.cc:600: FAILED:
  REQUIRE( h.listed(id) )

That matters more than the standard argument, because reading uninitialized heap frequently does return zero — fresh pages are zero-filled — so this class of bug hides well and a test that only samples a metric or two would not catch it.

I did not add an explicit initialization loop. It would be dead work on every blob, and the real risk is not today's behavior but a future change to something like make_unique_for_overwrite; the test catches that, and addBlob() now says so at the allocation.

[Copilot]

🟡 Changes recommended

Metrics::iterator can incorrectly treat listed GAUGE metrics as “end” due to type-bit contamination in find() positional iterators when using the new _bound numeric comparison.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

• Files reviewed: 5/5 changed files
• Comments generated: 1
• Review effort level: Lite

[cmcfarlen]

This one is a real bug, and mine. Fixed in 7b19905.

find() returned an iterator whose _it was the stored id, type bits and all, while _bound comes from current_id() and is built with COUNTER type bits. For a GAUGE the type bit at METRIC_TYPE_BITS puts _it above any realistic bound, so at_end() was immediately true and the iterator compared equal to end(). find() was therefore broken for every gauge in the store, not just unlisted ones. advance() already normalized the position; the positional constructor did not.

It is a regression from the bound comparison I introduced with unlisting — before that, operator== compared raw ids and the type bits cancelled out. No production code calls find(), so nothing was broken in the field, but every test I wrote for it happened to use a counter, which is why it got through.

Fixed as suggested, by keeping only the blob and offset:

auto [blob, offset] = _metrics._splitID(pos);

_it = _makeId(blob, offset, MetricType::COUNTER);

Dereferencing is unaffected: Storage::lookup(id, ...) deliberately reads the type from the slot rather than the id, precisely because iterators manufacture positions.

Test added first and watched fail on REQUIRE(g != m.end()), now covering both a gauge and a counter through find(), including that the dereferenced type comes back as GAUGE.

Separately, I have dropped the commit I added earlier about FlagStorage initialization. That comment was incorrect and a reply should have been the whole response; the extra commit was noise.

[Copilot]

🟡 Changes recommended

The new allocated()-based checks introduce unsynchronized reads of shared storage state that can be exercised by the new public APIs, creating a C++ data race risk under concurrent metric registration.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

• Files reviewed: 5/5 changed files
• Comments generated: 1
• Review effort level: Lite

[cmcfarlen]

Correct as stated, but deliberately out of scope here.

allocated() reads _blobs[blob], _cur_blob and _cur_off without the mutex because that is what every other id-taking accessor in this class already does:

• Storage::valid() — the same two fields, unlocked
• Storage::lookup(IdType, ...) — !blob || (blob_ix == _cur_blob && offset > _cur_off), unlocked
• Storage::name(IdType) — the same guard, unlocked
• Storage::rename() — the same guard, before it takes the lock

So this is not a race introduced by unlisting; it is the existing synchronization model of Storage, and allocated() was written to match it rather than to invent a second convention in the same class. Making just this one function lock while its neighbours do not would be misleading about the guarantees, and taking _mutex in allocated() would also put a lock in the iterator's skip path, which is currently lock free by design.

The synchronization of this class is being addressed directly in #13583, "Metrics: close the id lookup race and bounds gaps left by the lock revert". That is the right place for it — it is a property of the whole store, not of this feature, and fixing it in two PRs at once would just produce conflicts.

Whichever of the two lands second should extend the fix to cover the other's accessors: if #13583 goes first, allocated() needs the same treatment; if this one goes first, #13583 picks it up along with valid(), lookup() and name().

[Copilot]

Warning

Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.

[Copilot]

Warning

Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.

[cmcfarlen]

Five comments, one real bug. Taking them in order of consequence.

The subrange across snapshots — real, fixed in e776b65

This is a genuine defect and reachable through the public API. Two find() calls with a metric created between them is enough:

auto start = m.find("a");        // captures bound B1
Metrics::Counter::create("z");   // store grows
auto stop  = m.find("z");        // bound B2 > B1, and its position is >= B1

for (auto it = start; it != stop; ++it) { ... }   // never terminates

it passes B1, becomes exhausted against its own bound, stop is still live against B2, so they never compare equal and operator++ cannot make progress. I wrote the test first, with a step cap so it fails rather than hanging, and watched it fail.

Fixed by judging exhaustion between two positional iterators 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 is meaningless — folding it into the minimum would make every other iterator compare exhausted immediately.

My earlier subrange test missed this because it created both endpoints after all the metrics, so both held the same bound.

The count > 0 coupling — fair, fixed in 1f0f4ca

Filed twice, and right both times: that assertion did depend on the shared store holding at least one listed metric. In practice slot 0 is always listed so it could not actually fail, but the coupling is real and pointless. The section now creates a listed metric of its own and asserts it is observed.

One correction to the suggestion: an anchor placed before the unlisted tail proves the loop ran, not that iteration reached the tail. The "none of the tail names appear" assertion is what covers the tail. The comment says only the former.

Structured bindings and -Werror — not an issue

The concern is that binding [name, type, value] and using only name trips an unused-variable warning under -Werror. Neither clang nor gcc warns per element of a structured binding, and I can show it rather than argue it: the exact translation unit compiles with

-Wall -Werror -Wextra -Wsuggest-override -Wthread-safety ...

and is clean. The pattern is also already in the tree — two occurrences in this same file before this PR, three in RecCore.cc. Rewriting to std::get<0>(entry) would diverge from that for no benefit.

listed() const-correctness — declining, but happy to be overruled

Correct that NamesAndAtomics const * would be tighter in a const method. I left it because lookup(IdType, ...) and name() immediately above it do exactly the same thing, and changing one of three makes the file less consistent, not more. It is a worthwhile sweep across all of them as its own change; say the word if you would rather I just do the one here.

Both commits build and pass test_tsutil individually.

[Copilot]

Warning

Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.

[Copilot]

Warning

Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

Suppressed comments (1)

src/tsutil/Metrics.cc:1

• iterator(const Metrics&, IdType) normalizes _it to MetricType::COUNTER, but Storage::type() derives the type from the id bits (_extractType(id)). If the iterator id is coerced to COUNTER, iteration and find() dereference can report the wrong metric type (e.g., GAUGE/COUNTER mismatches) and potentially break any logic that depends on the encoded type. A robust fix is to keep _it as the canonical id (with its real type bits) and compare exhaustion/ordering using a separate positional value (e.g., blob+offset linear index), or to fetch the type for dereference from the slot’s stored id rather than from _it.

/** @file

[cmcfarlen]

Two to answer, since the FlagStorage one is a repeat and already addressed above.

Iterator equality is not an equivalence relation — correct, documented in 03452ff

The counterexample is real and I can be precise about it. With bounds B1 < B2 and an iterator a holding B1, b holding B2, and b positioned in [B1, B2): a == b (both past the shared minimum), a == end() (past its own bound), but b != end(). Transitivity fails.

The cause is structural rather than a slip in the comparison: exhaustion is a property of each iterator's own snapshot, so any iterator-versus-end test is per-iterator, and no comparison rule over a single type can paper over that.

I looked at the sentinel design you suggest. It does fix it properly — iterator-versus-iterator becomes pure position equality, iterator-versus-sentinel carries the end test, and the two are different relations so nothing is required to hold across them. The cost is that end()'s return type changes in an installed header and std::distance(m.begin(), m.end()) stops compiling; there are two such lines in this file already, predating this PR.

We decided against it for now and documented the limitation instead. These are input_iterator_tag, the only uses in tree are range-for over the whole store, std::distance(begin, end), and find() != end(), and none of them depends on transitivity. Trading a public type change for a property nothing relies on did not seem the right call inside this PR. The note on operator== now says plainly that only same snapshot comparisons and comparison against end() are meaningful, and that these should not be handed to a generic algorithm — so the next person meets the constraint at the point of use rather than deriving it. If you would rather have the sentinel, it is a contained change and I am happy to do it.

Worth adding: the min-bound rule this replaced a hang, not correct behaviour. Before it, that same subrange did not terminate at all.

Many SECTIONs mutating the singleton store — declining

The remedy does not do what it is meant to here. Splitting into separate TEST_CASEs would only reduce accumulation if the shared body registered metrics on each re-run, and this one does not:

TEST_CASE("Metrics unlisting", "[libtsapi][Metrics]")
{
  auto &m = Metrics::instance();

  SECTION(...)

The body is a reference binding. Every registration happens inside a SECTION, so it happens exactly once whichever way the cases are split, and the total is identical. Splitting also would not reset anything — it is the same process and the same singleton either way, and there is deliberately no reset hook, since the store never frees a slot.

On volume: the largest fill in this test case is 8 metrics, and it adds a few dozen in total against a store that holds 8M. The pattern of many sections over Metrics::instance() is also what the rest of this file already does.

The one place this was a real problem was an assertion that depended on the store holding at least one listed metric from elsewhere, which is fixed in 1f0f4ca.

[Copilot]

Warning

Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.

[cmcfarlen]

Both of these arrived twice; answering once each.

input_iterator_tag versus the note — fair, and my note caused it. Reframed in 0d16232

The objection lands on the previous wording rather than on the code: declaring std::input_iterator_tag and then saying "not iterators to hand to a generic algorithm" advertises a contract and denies it in the same breath. That was mine and it was the wrong framing.

The framing that fits what the code does: a snapshot is the sequence. begin() fixes the extent to be walked; iterators from two different snapshots are no more comparable than iterators into two different containers, and the standard already treats that as outside the domain rather than as a broken relation. Within one snapshot every iterator shares a bound, so equality is exactly the equivalence relation an input iterator requires, and the tag is honest.

The min-bound rule then has a narrower job than the old note implied: it makes the out-of-domain case terminate instead of hang. Before it, that comparison did not terminate at all, which is a worse kind of unspecified.

On the two suggested remedies: the sentinel design does fix the relation properly and I priced it out — end()'s return type changes in an installed header and std::distance(m.begin(), m.end()) stops compiling, which is two lines in this file that predate the PR. We decided that was not a trade to make inside this PR. It remains a contained change if a maintainer prefers it.

Private iterator constructors are a source break — correct, and now documented

Accurate: iterator(const Metrics &, IdType) was public and is not any more.

I am keeping it private. Nothing in tree constructs an iterator directly, and there is no sensible reason to — a caller holding an id wants lookup, not an iterator positioned at one slot. More to the point, that public constructor is where both iterator defects found in this review came from: a gauge id whose type bits put it past the bound, and a subrange built from two snapshots that never terminated. Both required naming an arbitrary position. The suggested compromise, keeping it public but normalising unlisted positions to end(), closes neither.

What was missing was disclosure, so the PR description now has an API change section recording it, for the 11.0.0 release notes alongside the createSpan and rename removals from #13583.

[Copilot]

Warning

Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.

[cmcfarlen]

Follow-up #13666 opens the consumer of unlist: the per server connection metrics retract their published names when metric_aggregate changes. It is a draft while this PR is open, since it carries this branch's commit until then.