Per upstream server connection metrics via hidden and derived metrics

[cmcfarlen]

Publishes per-upstream-server connection metrics, optionally aggregated per hostname, using hidden metrics as the inputs and a derived metric for the aggregation, so the aggregation work happens on the stat sync task rather than on every connection.

Depends on #13505, which adds the hidden-metric store and Derived::add_source / Op::MAX that this builds on. The first 8 commits here are that PR; the last 7 are new. Please merge #13505 first, after which this diff reduces to those 7 commits.

Note #13505 has since gained one more commit (tagging hidden metrics with RECT_HIDDEN_METRIC) that is not in this branch yet, so this needs a rebase once that settles.

Two settings, and why

The per server metrics are controlled by a pair of settings rather than a single level. Both are reloadable and overridable per remap rule.

proxy.config.http.per_server.connection.metric_enabled keeps the meaning it shipped with in 10.2.0:

Value	Behavior
0	No per server metrics.
1	Per server metrics are collected for each upstream server group.

proxy.config.http.per_server.connection.metric_aggregate is new and decides what is published:

Value	Behavior
0	No aggregate. The per group metrics are published under their own names. Default, and identical to 10.2.0.
1	Per hostname aggregates published, and so are the per group metrics.
2	Only the per hostname aggregates are published; the per group metrics stay hidden, keeping the published metric count proportional to hostnames rather than to groups.

An earlier version of this PR folded both into metric_enabled as a three-valued level. That was backward incompatible in a way that would have been easy to miss: 10.2.0 validates metric_enabled as [0-1] and publishes the per group metrics at 1, so redefining 1 to mean "aggregates only, per group hidden" would silently remove those metrics on upgrade — and for any match type other than both, where no aggregate exists, would leave nothing published at all. Splitting the two concerns keeps the default behavior byte-identical to 10.2.0 and makes the low-cardinality mode an explicit opt-in.

With metric_aggregate: 2, a group that has no aggregate to belong to gets its per group metrics published anyway, since otherwise nothing at all would be reported for it.

The value is applied when a connection group is created, which is the existing semantics of metric_enabled. Where two mappings that disagree resolve to the same group, the transaction that creates the group decides; a group is discarded once its connection count reaches zero, so the choice is remade the next time that upstream is reopened. This affects only which metrics exist — enforcement of per_server.connection.max uses the group's own count and is unaffected.

API

Two keys are added, TS_CONFIG_HTTP_PER_SERVER_CONNECTION_METRIC_ENABLED and TS_CONFIG_HTTP_PER_SERVER_CONNECTION_METRIC_AGGREGATE, appended to TSOverridableConfigKey and to OVERRIDABLE_CONFIGS so no existing key value shifts. The X-macro drives overridable_txn_vars, the InkAPI dispatch, the Lua bindings and the SDK test array from one entry each.

Neither converter clamps, matching SERVER_MATCH_CONV directly above them: SDK_API_OVERRIDABLE_CONFIGS sets every overridable INT config to an arbitrary value and requires it to read back unchanged. Range checking lives in the records layer instead — records.yaml validates the value and the reload callbacks clamp — so an out of range value is only reachable by a plugin that sets one deliberately, and both settings degrade safely if that happens. MetricAggregate has a fixed underlying type so storing such a value is defined rather than UB.

Metrics

Three hidden per group metrics (current_connection, total_connection, blocked_connection) feed four published per hostname aggregates: those three summed across the hostname's groups, plus current_connection_max, the MAX of the groups' current counts.

current_connection_max is deliberately the maximum of the groups' instantaneous counts, not a monotone high-water mark. It answers "how close is the busiest group of this hostname to per_server.connection.max right now", which matters because that limit is enforced per group rather than per hostname. Because it rises and falls, a monitoring system can compute max-over-time over any window from it; a monotone value would collapse the time dimension and only report that a peak happened at some point, not when. There is no per group current_connection_max — it exists only as an aggregate.

Aggregates are registered only for the both match type, the only one with more than one group per hostname. For host, Group::metric_name already returns the bare FQDN, so a host aggregate would collide with the single group's own name on one metric.

The per group metrics are always created in the hidden store whenever metric_enabled is set. Only what is registered for publication varies, so changing metric_aggregate at runtime never has to migrate a metric between the two stores.

No new hot-path work

TxnState::reserve, release and blocked are unchanged. They already branch on _count_metric != nullptr and use the typed mutators, and createHiddenPtr returns the same pointer types, so those branches keep working with the metrics simply being hidden now.

Also fixes the per group peak count

Group::_count_max, reported as the max field of the connection tracker group dump, had two defects, both pre-existing:

• update_max_count() made a single compare_exchange_weak attempt with no retry, so a racing update — or a spurious failure of the weak form, which is permitted by the standard and does occur on LL/SC architectures — silently discarded the sample.
• It was only called when a maximum was configured. With metrics enabled and no configured maximum the count was reserved and then discarded, so the peak stayed at zero.

Testing

tests/gold_tests/origin_connection/per_server_connection_max.test.py is extended to cover:

• metric_aggregate: 2 — host aggregates published, and all three per group names absent from a normal query.
• metric_aggregate: 1 — host aggregates plus the per group metrics published.
• Both modes: per group metrics visible via traffic_ctl metric match per_server --include-hidden.
• An aggregate genuinely spanning two groups — one hostname mapped to two origin ports under match: both, with different concurrency per group (2 and 3) so the SUM (5) and the MAX (3) are distinguishable rather than coincidentally equal. Nothing previously covered a multi-group aggregate.
• Both gauges draining back to 0 after traffic stops and a further sync interval passes. This is the assertion that distinguishes an instantaneous gauge from a monotone peak, which could never satisfy it.
• metric_enabled overridden per remap rule — two mappings to different origin ports, metrics enabled globally and turned off for one of them with conf_remap, asserting the disabled mapping's group appears in neither the published nor the hidden store.

Two changes make the file cheaper to run. Aggregates are recomputed by Metrics::Derived::update_derived() from raw_stat_sync_cont, whose period comes from proxy.config.raw_stat_sync_interval_ms; each ATS instance now sets that short so the assertions can wait ~2s instead of >5s. (An earlier revision of this test asserted that no record drove that interval — it does.) The four test classes also share one microDNS server rather than starting five identically configured ones, since each process costs several seconds at teardown.

The autest passes. Documentation is added for metric_enabled, metric_aggregate and metric_prefix, none of which was documented before, plus the metrics themselves under the monitoring guide.

Co-authored-by: @serrislew

[Copilot]

Pull request overview

This PR extends ts::Metrics with a hidden metrics store and richer derived-metric aggregation, then uses those facilities to publish per-upstream-server connection metrics (with configurable publication levels) while shifting aggregation work onto the periodic stats sync task rather than the connection hot path.

Changes:

• Add a hidden metrics store (Metrics::hidden_instance()), plus derived-metric enhancements (MAX/MIN ops and incremental Derived::add_source()).
• Implement per-upstream-server connection metrics based on hidden per-group metrics and derived per-hostname aggregates; update traffic_ctl metric match to optionally include hidden metrics.
• Expand unit/integration tests and documentation to cover hidden/derived metrics and the new per-server connection metric behavior.

Reviewed changes

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

Show a summary per file

File	Description
tests/gold_tests/origin_connection/per_server_connection_max.test.py	Extends gold test coverage for per-server connection metrics levels, hidden visibility, and multi-group aggregates.
tests/gold_tests/jsonrpc/metric_match_include_hidden.test.py	Adds an end-to-end JSONRPC test ensuring --include-hidden is accepted and still returns published metrics.
src/tsutil/unit_tests/test_Metrics.cc	Expands unit tests for derived ops, add_source behavior, hidden store semantics, and blob boundary handling.
src/tsutil/Metrics.cc	Implements hidden store instance, store exhaustion guard, derived ops (SUM/MAX/MIN), and incremental source registration.
include/tsutil/Metrics.h	Adds public APIs for hidden metrics and derived-metric ops/add_source; exposes hidden metric creation helpers.
src/iocore/net/ConnectionTracker.cc	Moves per-group connection metrics into hidden store, registers derived per-host aggregates, and (level 2) mirrors group metrics into published store.
include/iocore/net/ConnectionTracker.h	Introduces MetricLevel, documents publication levels, adds hostname aggregate naming helper, and fixes max-count update loop.
src/proxy/http/HttpSM.cc	Ensures peak count is updated even when metrics are enabled without a configured maximum.
src/traffic_ctl/traffic_ctl.cc	Adds traffic_ctl metric match --include-hidden CLI option and usage hint.
src/traffic_ctl/CtrlCommands.h	Extends record_fetch API to optionally include hidden metric record types.
src/traffic_ctl/CtrlCommands.cc	Wires --include-hidden into metric match RPC requests.
include/shared/rpc/RPCRequests.h	Adds metric record-type set including RECT_HIDDEN_METRIC.
src/mgmt/rpc/handlers/records/Records.cc	Allows JSONRPC decoding of RECT_HIDDEN_METRIC as an opt-in record type.
include/records/RecDefs.h	Adds RECT_HIDDEN_METRIC bit outside RECT_ALL.
src/records/RecCore.cc	Enables matching hidden metrics during record lookups when explicitly requested.
src/records/RecordsConfig.cc	Updates metric_enabled validation range to include level 2.
doc/developer-guide/internal-libraries/Metrics.en.rst	Adds internal documentation for hidden metrics and derived metric aggregation APIs/semantics.
doc/developer-guide/internal-libraries/index.en.rst	Adds Metrics docs to internal libraries index.
doc/appendices/command-line/traffic_ctl.en.rst	Documents traffic_ctl metric match --include-hidden.
doc/admin-guide/monitoring/statistics/core/http-connection.en.rst	Documents per-server connection metrics, naming, and aggregate behavior.
doc/admin-guide/files/records.yaml.en.rst	Documents metric_enabled levels and metric_prefix configuration.

[Copilot]

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

[cmcfarlen]

[approve ci autest 1]

[serrislew]

Similar to proxy.config.http.per_server.connection.max, could we make this metric_enabled feature overridable

[Copilot]

Pull request overview

Copilot reviewed 27 out of 27 changed files in this pull request and generated no new comments.

Suppressed comments (2)

src/tsutil/Metrics.cc:84

• When the store is exhausted, Storage::create() always returns literal 0 (the COUNTER bad_id). For GAUGE metrics this produces an id whose encoded type is COUNTER, which can cause type() / iteration / any id-based logic to mis-classify the metric type even though the caller requested a GAUGE. It’s safer to return a type-correct id that still aliases blob 0 / offset 0.

  if (_cur_blob >= MAX_BLOBS - 1 && _cur_off >= MAX_SIZE - 1) {
    return 0; // Slot 0 is the reserved bad_id. Cannot grow further.
  }

src/records/RecCore.cc:623

• Hidden metrics are emitted with tmp.rec_type = RECT_PROCESS. That makes a JSONRPC request that asks for only RECT_HIDDEN_METRIC (64) fail the type validation (recType & rec_type == 0), and also prevents clients from distinguishing hidden metrics in responses. Emitting them as RECT_HIDDEN_METRIC keeps type filtering consistent and preserves the opt-in semantics.

        RecRecord tmp;

        tmp.rec_type = RECT_PROCESS;

        tmp.name         = name.data();

[Copilot]

Pull request overview

Copilot reviewed 27 out of 27 changed files in this pull request and generated no new comments.

Suppressed comments (1)

tests/gold_tests/origin_connection/per_server_connection_max.test.py:399

• MultiGroupAggregateTest starts the slow curl clients in an earlier TestRun whose only command is sleep 1, then performs the “while held” metric assertions in later TestRuns. If AuTest tears down or waits on non-daemon processes at the end of a TestRun, the slow curl processes may have already exited (or been terminated) by the time traffic_ctl metric match runs, making the assertions flaky or incorrect. Consider running the traffic_ctl checks in the same TestRun as the slow requests (or explicitly keeping those client processes alive across TestRuns) so the metrics are sampled while connections are actually open.

        tr.Processes.Default.Command = 'sleep 1'
        tr.Processes.Default.ReturnCode = 0
        tr.Processes.Default.TimeOut = 30

        self._test_metrics_while_held()

[Copilot]

Pull request overview

Copilot reviewed 27 out of 27 changed files in this pull request and generated no new comments.

[Copilot]

Pull request overview

Copilot reviewed 27 out of 27 changed files in this pull request and generated no new comments.

Suppressed comments (5)

src/iocore/net/ConnectionTracker.cc:89

• MetricAggregate setter clamps after narrowing MgmtInt (int64_t) to int. If the input is outside int range, the cast is implementation-defined and can cause clamping to pick the wrong level. Clamp in MgmtInt first and only then cast to the enum.

  [](void *data, MgmtInt i) -> void {
    auto level = std::clamp(static_cast<int>(i), static_cast<int>(ConnectionTracker::AGGREGATE_NONE),
                            static_cast<int>(ConnectionTracker::AGGREGATE_ONLY));
    *static_cast<ConnectionTracker::MetricAggregate *>(data) = static_cast<ConnectionTracker::MetricAggregate>(level);
  }};

src/iocore/net/ConnectionTracker.cc:191

• Same narrowing-before-clamp issue here: data.rec_int is wider than int, so casting to int before clamping can overflow and select the wrong aggregate level. Clamp as int64_t first, then cast to MetricAggregate.

  if (RECD_INT == dtype) {
    auto level               = std::clamp(static_cast<int>(data.rec_int), static_cast<int>(ConnectionTracker::AGGREGATE_NONE),
                                          static_cast<int>(ConnectionTracker::AGGREGATE_ONLY));
    config->metric_aggregate = static_cast<ConnectionTracker::MetricAggregate>(level);
    return true;

src/iocore/net/ConnectionTracker.cc:177

• This config update clamps after casting data.rec_int to int. Since rec_int is wider, extreme values can overflow in the cast and then clamp incorrectly. Clamp in int64_t first, then cast.

  if (RECD_INT == dtype) {
    config->metric_enabled = std::clamp(static_cast<int>(data.rec_int), 0, 1);
    return true;

src/iocore/net/ConnectionTracker.cc:81

• MgmtInt is int64_t, but this setter clamps after casting to int. Large values can overflow during the narrowing cast, which can flip sign and clamp to the wrong value (e.g., a huge positive could become negative and clamp to 0). Clamp in MgmtInt first, then cast.

This issue also appears in the following locations of the same file:

• line 85
• line 175
• line 187

  [](void *data, MgmtInt i) -> void {
    *static_cast<decltype(TxnConfig::metric_enabled) *>(data) = std::clamp(static_cast<int>(i), 0, 1);
  }};

src/records/RecordsConfig.cc:401

• The PR description says proxy.config.http.per_server.connection.metric_enabled became a 0/1/2 level, but the code keeps it as a 0/1 flag (^[0-1]$) and introduces proxy.config.http.per_server.connection.metric_aggregate as the 0/1/2 publication level. Please update the PR description to match the implemented/configured behavior.

  {RECT_CONFIG, "proxy.config.http.per_server.connection.metric_enabled", RECD_INT, "0", RECU_DYNAMIC, RR_NULL, RECC_STR, "^[0-1]$", RECA_NULL}
  ,
  {RECT_CONFIG, "proxy.config.http.per_server.connection.metric_aggregate", RECD_INT, "0", RECU_DYNAMIC, RR_NULL, RECC_STR, "^[0-2]$", RECA_NULL}
  ,

[Copilot]

Pull request overview

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

Suppressed comments (2)

src/iocore/net/ConnectionTracker.cc:195

• Config_Update_Conntrack_Metric_Aggregate clamps out-of-range values to AGGREGATE_ONLY (2), but the comment above (and the intended “degrade safely” behavior) says out-of-range values should behave like AGGREGATE_GROUP (publish both aggregate + per-group). As written, a plugin (or direct RecSet) that sets a value >2 will unexpectedly suppress per-group publication instead of enabling it.

  if (RECD_INT == dtype) {
    auto level               = std::clamp(static_cast<int>(data.rec_int), static_cast<int>(ConnectionTracker::AGGREGATE_NONE),
                                          static_cast<int>(ConnectionTracker::AGGREGATE_ONLY));
    config->metric_aggregate = static_cast<ConnectionTracker::MetricAggregate>(level);

src/records/RecordsConfig.cc:400

• The PR description says proxy.config.http.per_server.connection.metric_enabled “becomes a level” with values 0..2, but the implementation here still validates metric_enabled as ^[0-1]$ and introduces a separate metric_aggregate (0..2) to control publication level. Please reconcile the PR description (and any related release notes) with the actual configuration interface.

  {RECT_CONFIG, "proxy.config.http.per_server.connection.metric_enabled", RECD_INT, "0", RECU_DYNAMIC, RR_NULL, RECC_STR, "^[0-1]$", RECA_NULL}
  ,
  {RECT_CONFIG, "proxy.config.http.per_server.connection.metric_aggregate", RECD_INT, "0", RECU_DYNAMIC, RR_NULL, RECC_STR, "^[0-2]$", RECA_NULL}

[cmcfarlen]

[approve ci autest 2]

[cmcfarlen]

@serrislew @bryancall — on the add_source linear scan, since you both landed on it independently.

I measured rather than guessed. DerivedMetric is 40 bytes (IdType is int32_t, plus a 24-byte source vector and the op), so std::find_if is a contiguous sweep comparing one int32_t at stride 40:

distinct group identities	derived entries (default metric_aggregate: 0, 3/group)	per group creation (3 scans)
~33	~100	~4 KB swept, ~0.5 µs
500	~1500	~180 KB swept, ~10–20 µs

For context on what shares that call path: the Group constructor already does 3–7 std::string concatenations (a heap allocation each) plus 3–7 _create calls, each taking the metrics-store mutex and hashing a ~50 character name — on the order of 1–3 µs. So at the cardinality this actually runs at, the scan is cheaper than the string work sitting next to it, and an id-indexed map would not be measurable. It starts to matter in the high hundreds of distinct upstreams, and even then it is ~10 µs on a path that also does a DNS lookup and a TCP connect.

The contention concern is the sharper version of this, and the map does not address it either: the serialization is _outbound_table._mutex held across the whole constructor, plus metrics_lock, which update() also takes every sync tick while walking every entry. Making the lookup O(1) shortens the hold slightly without changing which locks are held or for how long in aggregate.

What actually makes the linear scan the right structure is bounding N, which is the registry lifetime issue Bryan raised separately: ~Group never unregisters, so entries accumulate per distinct (fqdn, addr, port) identity ever seen, and update() walks dead groups forever. Worth being precise about the growth, though — add_source dedups by source pointer and the hidden slots are name-keyed, so a reaped group that comes back reuses its existing entry. Churn is free; growth is bounded by distinct upstream identity cardinality, which plateaus for a fixed origin set. Not a per-connection leak.

We are deferring that cleanup to a follow-up rather than growing this PR, because the design needs a discussion first:

1. Whether real deployments churn the identity space enough for the growth to matter at all.
2. If so, remove_source needs a final refresh before erasing an entry (otherwise a published gauge freezes at its last synced value instead of dropping to 0, since the hot path only writes the hidden copy) and refcounted registrations (otherwise an overlapping reap and re-create can strip a live group's publication).
3. Retiring metrics from Storage is the harder half. Since pointers handed out are raw and never invalidated, and a slot is only 8 bytes plus a name, reclamation buys almost nothing — the costs that bite are enumeration and update(). A tombstone that hides a metric from both, without ever invalidating a pointer, looks like the right first step.

Filing that as a separate issue so it does not get lost.

[bryancall]

I re-read the current head (edb63d3f9e) now that #13505 has merged and the diff is down to the 13 files that are actually this change. All 15 checks are green, including the six platform builds and AuTest 2of4.

Resolved since my last review: the clamping converters are fixed by 5e7cc8ec93, so SDK_API_OVERRIDABLE_CONFIGS round-trips and the platform builds went green. MetricOverrideTest now covers the per-transaction override path, which I had said was untested; that was wrong for metric_enabled, and it holds only for metric_aggregate, which no test overrides. AggregateOnlyWithoutHostAggregateTest covers the !has_aggregate fallback, which nothing else in the file would have caught.

On the add_source linear scan: your measurement convinced me, and I withdraw the objection. A 40-byte contiguous stride sitting next to 3 to 7 heap-allocating string concatenations and 3 to 7 mutex-taking _create calls in the same constructor is not the cost that matters, and an id-indexed map would not touch the real serialization, which is _outbound_table._mutex held across the whole constructor. Deferring remove_source and the Storage tombstones to a follow-up is the right scoping call, and the two traps you named there, a final refresh before erasing and refcounted registrations, are exactly the ones I would have worried about.

One consequence of that deferral has to land here, though, and it is the main thing I am asking for. Everything I am requesting is documentation wording; I am not asking for the code change in this PR.

What I am asking for

1. The reload semantics are one-directional, and the docs read as if they are not. Publication can be added at runtime but never removed. add_source calls _create when the group is constructed, ~Group unregisters nothing, and there is no Derived::remove_source, so:

• metric_enabled 1 to 0 leaves every published name in the store for the life of the process, frozen at its last sampled value. A counter parks at some non-zero number and stops moving, which reads as a live metric rather than a retired one.
• metric_aggregate 0 or 1 to 2 does not hide the per group mirrors that are already published.
• metric_aggregate 1 to 0 keeps publishing the hostname aggregates.

The third one is the reason I am treating this as blocking rather than a nit. AGGREGATE_ONLY is the feature's headline value, bounding the published metric count to hostnames instead of groups, it is marked :reloadable:, and it cannot be applied to a running server. An operator who is drowning in metric cardinality will read that page, set metric_aggregate: 2, reload, and see no change. I would keep the :reloadable: marker, since the setting genuinely is reloadable in one direction, and add the caveat. Suggested wording inline on records.yaml.en.rst.

2. A hostname aggregate can silently cover a subset of its groups. Source registration is per group, gated on the creating transaction's metric_aggregate. Under match: both, a mapping with metric_aggregate: 1 registers its group and a mapping with 0 does not, so total_connection.<host> sums a strict subset and current_connection_max.<host> maxes over a subset, while the monitoring page says "each summed across the groups of that hostname". This became reachable through ordinary configuration when metric_aggregate became overridable. Documenting the actual rule is enough for this PR; deciding membership per hostname rather than per group is the real fix and can wait for the follow-up. Suggested wording inline.

3. Two match types can collide on one published name. server_match is itself overridable (OverridableConfigDefs.h:229). For match: host, Group::metric_name returns the bare FQDN, so foo.com publishes ...current_connection.foo.com. For match: both, host_metric_name returns the same string, so foo.com publishes ...current_connection.foo.com as well. add_source finds the existing DerivedMetric by id, appends the source, and ignores the later caller's op (Metrics.cc:329). One published name then carries the union of a per-group mirror and a cross-group aggregate, with no diagnostic. The doc comment on host_metric_name reasons only about a single match type. Suggested wording inline.

Worth fixing, not blocking

4. per_server_metric_enabled.test.py was not updated and now depends on a sync tick. It sets metric_enabled: 1 and match: port, does not set raw_stat_sync_interval_ms, so it gets the 5000ms default, and asserts per_server.total_connection.bar.127.0.0.1:<port> 1 after sleep(keep_alive_timeout * 3), which is 6 seconds. That name is now a hidden metric mirrored on a derived tick, so the assertion needs a tick to land inside a 6 second window against a 5 second period: always at least 1 second of margin and never more, under AddressSanitizer on shared CI. It passes today, so this is a flake waiting to happen rather than a break, but it is worth giving it the same _STAT_SYNC_RECORDS treatment as the file you did update, while the mechanism is fresh.

5. The behavior change at the default setting is not in the upgrading notes. Anyone already running metric_enabled: 1 had per group metrics written on every connection event. They are now sampled mirrors, up to 5 seconds stale, reading 0 for a full interval after a group first appears, and no metric_aggregate value restores direct publication. The monitoring page documents the sampling well, but doc/release-notes/upgrading.en.rst is untouched, so an operator whose health check opens a connection and immediately reads the metric will report a regression that does not exist.

6. metric_prefix stayed global while the enable became per-transaction. A prefix reload renames nothing, so combined with item 1 it starts a second family of names while the old aggregate keeps publishing a shrinking subset: two live metrics and neither one correct. It is also read from ET_NET threads in the Group constructor with no synchronization against Config_Update_Conntrack_Metric_Prefix, which does a bare config->metric_prefix = data.rec_string at ConnectionTracker.cc:205. That race is pre-existing, but this change puts a second code path on it.

7. Smaller items, inline where they anchor: no diagnostic on either new converter or reload callback, the Group constructor default arguments, and the "single source SUM is an identity" comment, which describes something the design deliberately is not.

Two more that have no natural anchor. It is worth saying in the PR body that anchoring the pattern to ^[0-1]$ is also an incompatible change, since recordRegexCheck is an unanchored search and a records.yaml carrying metric_enabled: 2 used to load and now fails validation. In MultiGroupAggregateTest, _hold_seconds = 6 needs five concurrent /delay/6 requests all in flight when a tick lands, across a TestRun boundary, so current_connection == 5 and current_connection_max == 3 are the likeliest flakes in the new file; widening the hold costs nothing at a 500ms interval. The comment there says no clamping applies below httpbin's 10 second /delay cap, but go-httpbin returns 400 above it, which --fail turns into a hard failure rather than a clamp.

What is good here

Every enum synchronization point is X-macro generated with its own static assertions, covering the Lua constants, the InkAPITest array, and overridable_txn_vars order and count, so nothing is missed and no Lua source change is needed. Appending before TS_CONFIG_LAST_ENTRY correctly shifts no existing key value.

Walking back the metric_enabled 0/1/2 redefinition was the right call, and the PR description explains why better than I could: redefining 1 would have silently dropped the per group metrics for anyone upgrading from 10.2.0, and for any match type other than both would have left nothing published at all.

The update_max_count rewrite is a real bug fix independent of this feature. The old single compare_exchange_weak could discard a sample on a spurious failure, which the standard permits and which does happen on load-linked/store-conditional architectures. Relaxed ordering is defensible for a pure statistic. Feeding reserve()'s result through in the branch with no configured maximum fixes a peak that was previously always zero.

The configuration migration is complete: no reader of the old global metric_enabled remains, and both guards in HttpSM::do_http_server_open moved together, which is what keeps an inactive TxnState from being dereferenced.

I went looking for a use-after-free on the retained raw pointer and there isn't one. Hidden Storage blobs outlive everything, slots are never reused, and the gauge drains to 0 before a group dies, so a dead group contributes 0 to both SUM and MAX. The problem in item 1 is the retained registration, not a dangling pointer.

The unclamped converter decision is genuinely forced by SDK_API_OVERRIDABLE_CONFIGS, and the comment defending it is accurate rather than hand-waving. Giving MetricAggregate a fixed underlying type makes storing an out-of-enumerator value well defined, which is an improvement on the neighbouring MatchType, whose identical static_cast is undefined behavior. That looks worth a separate issue.

Converting the two consecutive Streams.All = assignments to += fixes a real latent bug: at base only the last tester on each stream ran, so the total_connection assertions were dead code. That means total_connection is being asserted here for the first time, so if it ever fails, look for a genuine value mismatch before assuming flakiness.

current_connection_max is the best-documented thing in this change: a maximum over the instantaneous gauge, explicitly not a high-water mark, with the right operational advice and a code comment that explains why rather than what. Asserting that both gauges drain back to 0 is the one thing a monotone peak could never satisfy, and picking 2 and 3 for the per group concurrency so the sum and the maximum are distinguishable rather than coincidentally equal is exactly right.

This is also the first documentation metric_enabled and metric_prefix have ever had, which is a genuine improvement on top of the feature itself.

[bryancall]

This is the sentence I would change. "the choice is made again the next time that upstream is reopened" reads as full reversibility, and it only holds for increasing publication. Turning metrics off, or switching metric_aggregate to 2, leaves the already-published names in place for the life of the process.

Suggested change

	determines its metrics, and later transactions do not change them. A group is discarded once its
	connection count reaches zero, so the choice is made again the next time that upstream is
	reopened. This affects only which metrics exist; enforcement of
	determines its metrics, and later transactions do not change them. A group is discarded once its
	connection count reaches zero, so *raising* the level of publication is picked up the next time
	that upstream is reopened: enabling metrics, or enabling the aggregates, takes effect as upstreams
	reconnect. Lowering it does not. Metrics are never retired once published, so disabling this
	setting, or switching
	:ts:cv:`proxy.config.http.per_server.connection.metric_aggregate` to ``2``, leaves the names that
	are already published in place, frozen at their last sampled value, until Traffic Server is
	restarted. This affects only which metrics exist; enforcement of

The same caveat is needed on metric_aggregate below, which currently just points back here with "the same consequence for mappings that disagree and resolve to the same group". It is worth spelling out there too, since 2 is the value an operator reaches for specifically to reduce a metric count, and that is the one direction a reload cannot deliver.

[bryancall]

"the only match type with more than one group per hostname" is not accurate, and the same claim appears in records.yaml.en.rst, in the MetricAggregate doc comment, and on host_metric_name. Group::hash and Group::equal key MATCH_IP on the address alone, so one hostname with several A records already yields several groups.

The real reason is narrower and, I think, more useful to a future reader: ip and port keys carry no hostname at all, and one such group is shared by every hostname that resolves to that address, so there is nothing to aggregate it under. As written, someone will eventually relax the restriction on the strength of a premise that is false.

Suggested change

	``proxy.process.http.per_server.<counter>.<hostname>``. Aggregates exist only for match type
	``both``, because that is the only match type with more than one group per hostname; for match type
	``host`` the group name is already the bare hostname, so an aggregate would carry the same name as
	the single group it summarises.
	``proxy.process.http.per_server.<counter>.<hostname>``. Aggregates exist only for match type
	``both``, because that is the only match type whose group key carries the hostname. An ``ip`` or
	``port`` group is keyed on the address alone and is shared by every hostname that resolves to it, so
	there is no single hostname to aggregate it under. For match type ``host`` the group name is already
	the bare hostname, so an aggregate would carry the same name as the single group it summarises.

[bryancall]

"each summed across the groups of that hostname" is what an operator will rely on, and it can be false. Aggregate membership is registered per group, gated on the metric_aggregate value of the transaction that created that group, so under match: both a mapping with metric_aggregate: 1 joins the aggregate and a mapping with 0 for the same hostname does not. The sum then covers a strict subset, and current_connection_max takes its maximum over that subset, with nothing indicating it.

Suggested change

	For a hostname aggregate, ``<counter>`` is one of those three, each summed across the groups of that
	hostname, plus:
	For a hostname aggregate, ``<counter>`` is one of those three, each summed across the groups of that
	hostname which have aggregation enabled, plus:

I would add a short paragraph after the current_connection_max entry as well, along the lines of: because metric_aggregate is overridable, a group joins its hostname's aggregate only if the mapping that first opened that upstream had aggregation enabled, so mappings that disagree for one hostname produce an aggregate over part of it. Making membership a per-hostname decision is the better fix, but that can go with the remove_source follow-up.

[bryancall]

Two problems in this paragraph. The first sentence repeats the inaccurate reason described on the monitoring page. The second is that the collision argument is right within one match type and does not hold across two, which matters because server_match is itself overridable: foo.com can be MATCH_HOST on one mapping and MATCH_BOTH on another, at which point metric_name for the host group and host_metric_name for the aggregate produce the identical string. add_source then finds the existing entry by id, appends the source, and ignores the later caller's op, so one published name silently carries the union of a per-group mirror and a cross-group aggregate.

Suggested change

	* Only @c MATCH_BOTH groups have more than one group per hostname. For @c MATCH_HOST there is
	* exactly one group per hostname, so an aggregate would be over a set of one, and
	* @c Group::metric_name already returns the FQDN alone for that match type - identical to what
	* this would return, so publishing both would collide on one name.
	* Only @c MATCH_BOTH keys carry both a hostname and an address, so it is the only match type
	* whose groups can be gathered by hostname at all. @c MATCH_IP and @c MATCH_PORT key on the
	* address alone and one such group is shared by every hostname resolving to it, so there is no
	* single hostname to aggregate it under. For @c MATCH_HOST there is exactly one group per
	* hostname, so an aggregate would be over a set of one, and @c Group::metric_name already
	* returns the FQDN alone for that match type - identical to what this would return, so
	* publishing both would collide on one name.
	*
	* Note that reasoning holds within a single match type. @c TxnConfig::server_match is
	* overridable, so one hostname can be @c MATCH_HOST on one mapping and @c MATCH_BOTH on
	* another, and then that group's own published name and this aggregate name are the same
	* string and are merged into one derived metric.

Documenting it is enough for this PR. Including the match type in the aggregate name, or warning on the collision, is the actual fix and can go with the follow-up.

[bryancall]

"the published value always equals the hidden source" is the one claim in this change that contradicts its own design. The published copy is stale between derived ticks and reads 0 from creation until the first one, which is precisely the property http-connection.en.rst takes care to explain. A single-source SUM does not combine anything, but it is still a sample rather than an identity.

Suggested change

	// Mirror the per group metrics into the published store under their own name. A single
	// source SUM is an identity: the published value always equals the hidden source.
	// Mirror the per group metrics into the published store under their own name. A single
	// source SUM combines nothing, but the published value is still a sample: it is whatever
	// the last derived tick read, and it reads 0 from creation until that first tick.

[bryancall]

Not blocking, and I accept the reasoning in the comment above for why neither converter clamps. What I would still like is a diagnostic somewhere on this path, because right now there is none anywhere in the new code.

SERVER_MATCH_CONV, three converters up, has a formatter and calls Warning_Bad_Match_Type. Both new reload callbacks clamp silently, and their return false on a wrong dtype is swallowed, since the RecCore lambda returns REC_ERR_OKAY unconditionally and traffic_ctl reports success either way. The sibling Config_Update_Conntrack_Match does at least emit Warning("Invalid type for '%s'"). The net effect is that a plugin setting metric_aggregate = 7 reads back 7, behaves as AGGREGATE_GROUP, and leaves no trace of either fact.

Related, and the same size of fix: the debug print at line 520 says Registered per_server_connection.{} without saying what was actually registered, hidden or published, aggregate or per group. That line is the first place an operator will look to answer "why can I not see my metric", which is going to be a common question given how many ways this feature can decline to publish something.

[bryancall]

Suggestion only. I would drop both default arguments. There are two construction sites, and only the INBOUND one 21 lines away in obtain_inbound uses the defaults, so the defaults buy one short call and cost a silent failure mode: a future outbound site that forgets these arguments compiles cleanly and gets no metrics.

The sharper version is that min_keep_alive and metric_enabled are adjacent, both int, so transposing them also compiles and produces a group with metrics keyed off a keep-alive count. Taking bool metric_enabled would remove that particular hazard, and it matches how the value is actually used here, which is a plain truth test.

[cmcfarlen]

Thanks Bryan — this is a genuinely useful review, and the two corrections to our own claims are the valuable part. Pushed 4e57a3e183 and 8038a05aa9.

Applied

1. Reload directionality. Took your wording on metric_enabled and spelled the caveat out on metric_aggregate rather than leaving it as a back-reference, since as you say 2 is the value someone reaches for specifically to cut cardinality and it's the one direction a reload can't deliver. Kept :reloadable:.

2. The wrong premise. You're right, and I verified it: Group::equal keys MATCH_IP on the address alone, so one hostname with several A records already yields several groups. Worth flagging that the claim had spread to four places, not one — http-connection.en.rst:220, records.yaml.en.rst:2051, and both ConnectionTracker.h:211 and :491 — so I fixed all of them with your reasoning: ip/port keys carry no hostname and are shared by every hostname resolving to that address. Good catch on "someone will eventually relax the restriction on the strength of a premise that is false"; that's exactly the failure mode.

3. Subset aggregates and the cross-match-type collision. Both documented, on the monitoring page and on host_metric_name. Confirmed server_match is overridable at OverridableConfigDefs.h:229, so the MATCH_HOST/MATCH_BOTH collision is reachable by configuration, and add_source does keep the first caller's op. Agreed that including the match type in the aggregate name, and deciding membership per hostname, are the real fixes and belong with the follow-up.

4. per_server_metric_enabled.test.py. Fixed — it now sets raw_stat_sync_interval_ms the same way the other file does. Your analysis was right: a 6 second wait against a 5000ms default is at least one second of margin and never more.

7 (partial). Corrected the "single source SUM is an identity" comment. You're right that it contradicts the design it sits in: a single-source SUM combines nothing, but the published value is still a sample, and it reads 0 from creation until the first tick.

One correction back

The unanchored-pattern note isn't right as stated — though I suspect it's a holdover from reviewing the first iteration of this work, which did define metric_enabled as a three-valued 0/1/2 level before we split it into two settings for backward compatibility. Against the pattern that's actually on master, a records.yaml carrying metric_enabled: 2 did not load: the pre-PR pattern was unanchored [0-1], and recordRegexCheck searching "2" for [0-1] finds no match, so validation failed then too. What actually regresses is multi-character values containing a 0 or 1 — 10, 01, 12 — which used to match on a substring and now don't. Real incompatibility, different trigger, and vanishingly unlikely in a real config, so I'd rather not put a note in the PR body implying 2 used to be accepted. Happy to add one about the substring case if you think it earns the space.

Deferred, with reasons

• 5, upgrading notes. Skipping this one here. upgrading.en.rst is still headed "Upgrading to ATS v10.x" and this change is on 11.x-dev, so starting that section is a bigger editorial call than this PR should make. The sampling-latency note lands with the follow-up, which also has to document the retirement behaviour, and the monitoring page already covers the mechanism in the meantime.

• 6, metric_prefix global + unsynchronised write. Agreed on both halves. The race is pre-existing and the fix wants to be the same change that makes registration lifetime-aware, so it goes with the follow-up.

• 7, remaining items. The diagnostics on the converters and reload callbacks and the more informative Registered per_server_connection debug line are worth doing — your point that "why can I not see my metric" will be the common question is well taken, given how many ways this feature can decline to publish. Dropping the Group constructor defaults and taking bool metric_enabled is a good call too; the adjacent-int transposition hazard is real. All code changes, none of them blocking, and I'd rather not reopen the platform builds on this PR for them.

• MultiGroupAggregateTest hold time. Fair, and widening it costs nothing at a 500ms interval. Also right that the comment's reasoning is wrong — go-httpbin returns 400 above its cap and --fail makes that a hard failure, not a clamp. Folding both into the follow-up unless you'd rather see it here.

Following up separately on the MatchType static_cast UB you spotted — you're right that it's a real find independent of this change, and it deserves its own issue rather than being buried in this thread.

[bryancall]

Approving.

The reload directionality and the corrected MATCH_BOTH premise are both right, and so is your correction back to me on the anchoring. recordRegexCheck searching "2" for the unanchored [0-1] finds no match either, so metric_enabled: 2 failed validation before this PR as well. My note was wrong. The substring case you identified (10, 01, 12) is the real regression, and I agree it does not earn a line in the PR body.

One straggler on the premise fix, non-blocking: the AggregateOnlyWithoutHostAggregateTest docstring at per_server_connection_max.test.py:517 still says 'both' is "the only match type with more than one group per hostname". That is a fifth copy of the claim you corrected in the other four places.

Agreed on the deferrals, including leaving upgrading.en.rst alone while it is still headed at v10.x.

[cmcfarlen]

Cherry-picked to the 10.2.x branch as 1a8102d for the 10.2.1 release.