cmcfarlen opened a new pull request, #13616:
URL: https://github.com/apache/trafficserver/pull/13616
## 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
Adds a *tombstone*: a per slot mark that makes a metric invisible to
iteration.
```cpp
auto &m = ts::Metrics::instance();
m.tombstone(id); // by id
m.tombstone("proxy.process.example"); // or by name
m.tombstone(id, false); // clear the mark
```
A tombstoned 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 resurrected by `create()` on the same name, returning the same id with
its accumulated value intact.
A tombstone expresses a publication policy, not a lifetime. Any `IdType` or
`AtomicType *` a caller already holds stays valid across a tombstone and
resurrect.
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.
## 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.
Set and clear use `fetch_or` / `fetch_and(~TOMBSTONE)` rather than a whole
word store, so a flag added later is not clobbered.
**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. That is strictly less locking than before, where `end()`
locked on every construction. It relies on iteration ids being monotonic in
(blob, offset), which holds because both ends use `MetricType::COUNTER` so the
type bits are zero.
Iteration is now explicitly a snapshot taken at `begin()`: a metric created
mid iteration is never seen rather than sometimes seen. This 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 marked slots; two live iterators still compare by position,
unchanged. The hand written `operator!=` is removed in favor of the C++20
synthesized one.
**`find()` returns `end()` for a tombstoned metric.** Iteration never visits
a marked slot, so an iterator pointing at one is a range bound that a skipping
walk steps straight over and never reaches. Use `lookup()` to read a tombstoned
metric.
## Tests
`test_Metrics.cc`: skipped by iteration, still resolvable by name and id,
resurrect via `create()`, tombstone by name, clearing the mark, `begin()`
skipping a marked first slot, a marked run at the end of the store, iterating
to a bound that is not `end()` with marked slots inside the range, `find()`
yielding `end()`, iterator comparison, an unallocated id, and independence
between the published and hidden stores.
`test_RecHiddenMetricLookup.cc`: a tombstoned metric is not enumerated by
`RecLookupMatchingRecords`, is still found by `RecLookupRecord`, and returns to
enumeration when the mark is cleared.
Documented in `doc/developer-guide/internal-libraries/Metrics.en.rst`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
