Retract per server metrics when the mode changes

[cmcfarlen]

Uses ts::Metrics::unlist from #13616, now merged, so this is rebased onto it and the diff is its own.

Problem

proxy.config.http.per_server.connection.metric_aggregate is RECU_DYNAMIC and overridable, but the decision it drives — which per server metric names get published — was made once, in the ConnectionTracker::Group constructor, and a published metric name could not be withdrawn. So the setting only ever took effect for names created after it changed.

Seen in production. A box that ran for a while at metric_aggregate 0 before being switched to 2 reports both shapes, and no reload removes the first set:

proxy.process.http.per_server.current_connection.ocsp.apple.com.17.253.67.133:80 0
...
proxy.process.http.per_server.current_connection.ocsp.apple.com 0
proxy.process.http.per_server.current_connection.max.ocsp.apple.com 0

The metric store hands out ids in allocation order and traffic_ctl prints them that way, so the dump is a timeline: every <fqdn>.<ip>:<port> name was created before the first aggregate name, with no interleaving. The config change took effect for everything after it; what came before was unretractable.

What the modes mean now

The suppressed-per-group mode was specified as a single metric per hostname — the max — not the sums as well. Mode 2 is that, and mode 3 is new for when the totals are wanted too:

value	per group	sums	max
0 AGGREGATE_NONE	published	no	no
1 AGGREGATE_GROUP	published	yes	yes
2 AGGREGATE_MAX	hidden	no	yes
3 AGGREGATE_SUM	hidden	yes	yes

Mode 1 is unchanged. AGGREGATE_ONLY is gone; 2 and 3 replace it.

The constructor now reduces to three independent decisions — publish the sums, publish the max, publish the per group metrics — each of which either registers a derived source or unlists the name. That reads better than the nested condition it replaces, and it is what makes 3 → 2 withdraw the sums rather than leave them behind.

An out of range value from a plugin is normalised to AGGREGATE_GROUP once, at the top of the constructor, rather than being implicit in the conditions.

Metric rename

current_connection_max becomes current_connection.max. ATS separates a qualifier with a dot — proxy.process.eventloop.time.max, .events.max — not an underscore. The metric only exists on master, from #13506, so the rename is free now and would not be after a release carries it.

What converges, and when

A change is applied per group, when that group is next constructed, which happens on the first connection after its count last fell to zero. Group::release() is called from PoolableSession::release_outbound_connection_tracking(), so it is the upstream session closing that erases the group, not the transaction ending. With origin keep alive on, a pooled session holds a group open and that group keeps whatever setting it was built with; a group that never goes idle never re-evaluates.

Two consequences worth knowing rather than discovering:

• The sums are named per hostname, not per group, so where the mappings for one hostname disagree about this setting, the last group constructed decides whether they are published.
• Retraction is not immediate. It follows session churn.

Both are documented at the enum and in records.yaml.en.rst.

Tests

src/iocore/net/unit_tests/test_ConnectionTracker.cc is new. It drives the production sequence in process: run at AGGREGATE_NONE so the per group names publish, switch, open and close another connection, assert the names are gone and the aggregates are there. Nine sections cover each mode, both switch directions for the sums, and the no-aggregate fallback at 2 and 3.

Getting that harness right took a correction worth recording: TxnState::release() only decrements, so a test using it never erases the group and nothing is re-evaluated. It has to follow the real path — TxnState::drop() into the session, then Group::release().

per_server_connection_max.test.py gains AggregateRetractionTest, which drives traffic at 0, asserts the per group name is published so the later assertion cannot pass vacuously, raises the setting with traffic_ctl, drives traffic again and asserts the withdrawal. It disables origin keep alive so group churn is deterministic, and waits after the traffic_ctl call because http_config_cb schedules the reconfigure a second out — without that wait the next request is still served by the previous HttpConfigParams, which looks exactly like a failure to retract.

MultiGroupAggregateTest gains an ExcludesExpression. Every assertion in that file was a ContainsExpression, which is why the original leak went unnoticed; a test that only checks for presence cannot catch a metric that should not be there.

Verified the autest fails without the fix: with the unlist call disabled, exactly one assertion fails, the retraction one. Four autests pass in a Fedora 44 container on the CI image — the three per_server* tests and slow_post, which exercises the same constructor through connection.max enforcement.

Two follow-ups from #13616

Both raised in review there after it merged, so they land here.

Metrics.h used std::forward in for_each while getting <utility> only through another
include. It compiles today, but that is not a property this header controls, so the include is now
direct.

Metrics.h is installed, so removing Metrics::iterator, begin(), end(), find(),
createSpan() and rename() breaks downstream plugins even with no in-tree callers. The v11
section of doc/release-notes/upgrading.en.rst now records the removals and shows the for_each
replacement, which is where an upgrader looks.

[Copilot]

🟡 Changes recommended

Moderate issues remain in the build configuration, metric ownership handling, and retraction test setup.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

Updates per-server connection metrics to support runtime aggregate-mode changes and retract obsolete metric names.

Changes:

• Adds aggregate modes and metric retraction.
• Adds unit and end-to-end coverage.
• Updates configuration, documentation, and Metrics API migration notes.

File summaries

File	Reviewed changes and findings
tests/gold_tests/origin_connection/per_server_connection_max.test.py	Adds retraction coverage. Moderate (1 vote): retain ATS, origin, and DNS processes across runs. Nit (1 vote): rename the obsolete AggregateOnlyWithoutHostAggregateTest. Nit (3 votes): update the assertion message from AGGREGATE_ONLY to AGGREGATE_SUM.
src/records/RecordsConfig.cc	Allows aggregate values 0–3. No findings.
src/iocore/net/unit_tests/test_ConnectionTracker.cc	Tests aggregate modes and transitions. No findings.
src/iocore/net/ConnectionTracker.cc	Implements mode-specific publication and unlisting. Moderate (2 votes): avoid unlisting names still owned by live MATCH_HOST metrics in mixed groups.
src/iocore/net/CMakeLists.txt	Registers the connection-tracker tests. Moderate (3 votes): remove the duplicate source entry in test_net.
include/tsutil/Metrics.h	Adds the direct <utility> dependency. No findings.
include/iocore/net/ConnectionTracker.h	Documents aggregate modes and lifecycle. No findings.
doc/release-notes/upgrading.en.rst	Documents removed Metrics APIs. No findings.
doc/admin-guide/monitoring/statistics/core/http-connection.en.rst	Updates metric documentation. Nit (1 vote): refer to aggregate outputs as metrics, or explicitly identify the counters.
doc/admin-guide/files/records.yaml.en.rst	Documents dynamic aggregation behavior. No findings.

Review details

Suppressed comments (3)

doc/admin-guide/monitoring/statistics/core/http-connection.en.rst:238

• current_connection is documented as a Gauge immediately above, so calling all three aggregate outputs “counters” is inaccurate. Please refer to them as metrics (or name the two counters explicitly).

For a hostname aggregate there are two kinds. The *sums* are those same three counters, each added
across the groups of that hostname which have aggregation enabled, published at

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

• This test now exercises AGGREGATE_MAX, but its class and invocation remain AggregateOnlyWithoutHostAggregateTest, referring to the removed AGGREGATE_ONLY mode. Rename it (for example, AggregateMaxWithoutHostAggregateTest) so test output and future references describe the behavior being covered.

    metric_aggregate 2 (AGGREGATE_MAX) normally leaves the per group metrics hidden and publishes

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

• This helper retains only ATS, but the first run also starts self._server and the shared DNS. The later test runs issue more curls without starting or retaining those processes, so the origin/DNS are not guaranteed to remain alive and the retraction check can fail before exercising metric publication. Keep both long-lived dependencies in StillRunningAfter for every run (and call _use_shared_dns on the later runs).

    def _curl(self, tr) -> None:
        """Drive one request through the remap rule."""
        tr.MakeCurlCommand(f"-v --fail -s -x 127.0.0.1:{self._ts.Variables.port} 'http://retract.origin.com/get'", ts=self._ts)
        tr.Processes.Default.ReturnCode = 0
        tr.StillRunningAfter = self._ts

• Files reviewed: 10/10 changed files
• Comments generated: 3
• Review effort level: Lite

---

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

[bneradt]

Reviewed 3854c19. One correctness issue needs addressing before approval: the new sum-retraction branch can withdraw a live MATCH_HOST group's own metrics when a MATCH_BOTH group for the same hostname is constructed, even when both use metric_aggregate=0. I added the concrete sequence and regression-test request to the existing inline discussion: #13666 (comment).

The four-mode publication logic otherwise looks consistent for MATCH_BOTH groups, and the Metrics include and v11 migration documentation follow-ups are addressed. The duplicate test_ConnectionTracker.cc CMake entry is worth removing as already requested, but I am not treating it as a separate correctness blocker.

Validation: source/test review and clean git diff --check. Thirteen CI checks pass; Clang-Analyzer is still pending. I did not run a local build or tests.

[Copilot]

🟡 Changes recommended

Critical metric race and name-collision issues remain, along with requested regression coverage and documentation updates.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (3)

doc/admin-guide/monitoring/statistics/core/http-connection.en.rst:253

• With modes 2 and 3, the max and sums do not necessarily have the same source set: a mode-2 group contributes to current_connection.max but not to the sums. This paragraph therefore documents incorrect behavior for mixed settings; describe the membership of the max and sums separately.

enabled. Mappings that disagree for one hostname therefore produce an aggregate over part of it: the
sums cover a subset of the groups and ``current_connection.max`` takes its maximum over that same

src/iocore/net/ConnectionTracker.cc:527

• The has_aggregate guard here is specifically needed for a MATCH_HOST group: its *.fqdn per-group names collide with a MATCH_BOTH hostname aggregate, so removing a MATCH_BOTH source must not unlist a still-live host source. The new unit test only covers the MATCH_PORT no-aggregate fallback; please add a regression case that keeps a host group alive while constructing a both group at mode 0 or 2 and verifies all three shared names remain listed.

    } else if (has_aggregate) {
      // Stop contributing rather than unlist: every group of this hostname shares these names, so
      // one that does not want them must not remove a name another is still publishing.
      Metrics::Derived::remove_source(sum_names[0], _count_metric);
      Metrics::Derived::remove_source(sum_names[1], _count_total_metric);
      Metrics::Derived::remove_source(sum_names[2], _blocked_metric);

src/iocore/net/unit_tests/test_ConnectionTracker.cc:218

• The current transitions cover adding the max and removing/re-adding the sums, but never remove a max source after it has actually been registered: publish_max == false is only reached before the max name exists in the NONE -> MAX case. Add a transition from AGGREGATE_MAX (or GROUP/SUM) to AGGREGATE_NONE and assert the hostname max disappears while the per-group names are published again.

  SECTION("switching to AGGREGATE_MAX retracts already published per group metrics")
  {
    // The production sequence: run for a while with the per group metrics published, then change
    // the setting. Without a retraction the first set of names is published forever.
    txn.metric_aggregate = ConnectionTracker::AGGREGATE_NONE;
    open_and_close_connection(txn, addr);
    REQUIRE(is_published(current_group));

    txn.metric_aggregate = ConnectionTracker::AGGREGATE_MAX;
    open_and_close_connection(txn, addr);

    CHECK_FALSE(is_published(current_group));
    CHECK_FALSE(is_published(total_group));
    CHECK_FALSE(is_published(blocked_group));
    CHECK(is_published(host_metric("current_connection.max")));
  }

• Files reviewed: 12/12 changed files
• Comments generated: 3
• Review effort level: Lite

[bneradt]

[P2] Confirmed from the name builders and Derived::add_source: with an empty metric_prefix, MATCH_HOST for max.example.test and MATCH_BOTH for example.test both register proxy.process.http.per_server.current_connection.max.example.test. If the MATCH_HOST source (value 7) registers first, the entry retains SUM; adding two MATCH_BOTH sources with values 2 and 3 produces 12 instead of the expected hostname max of 3. In the reverse construction order it retains MAX and the unrelated host source can inflate that max to 7. Please make the max aggregate namespace distinct from valid per-group hostname names and cover both construction orders. This remains a correctness blocker.

[bneradt]

Reconsidering severity after discussion: the collision is real, but requires mixed MATCH_HOST/MATCH_BOTH configurations and the related hostnames example.test and max.example.test. I have no evidence that this is common, and calling it a merge blocker overstated its practical impact. Treating this as a non-blocking naming follow-up; it does not prevent my approval.

[cmcfarlen]

Three comments, all correct. Two were mechanical; the third found a real defect and it is worse than described.

Duplicate test source — 2c37e2a5aa. The rebase re-added test_ConnectionTracker.cc to test_net after master had already listed it when master added its own cases to that file.

Stale assertion message — 366d7f0d6f. It named AGGREGATE_ONLY, removed by this PR, so a failure pointed at a configuration that no longer exists. It now interpolates the configured value.

Unlisting a name another group owns — 61d9751174 and 2af1eee7a1

Confirmed, and the mixed match type case is not the only route. The same defect reaches through a configuration that overriding metric_aggregate is for:

mapping A -> multi.origin.com:443   metric_aggregate 3   (sums published)
mapping B -> multi.origin.com:8443  metric_aggregate 2   (max only)

Two MATCH_BOTH groups, one hostname, one shared set of aggregate names. Building group B unlisted the aggregate group A was publishing. I checked it rather than reasoned about it: is_published("...current_connection.<fqdn>") came back false after B was constructed. Same match type throughout.

So the underlying rule is about ownership rather than about match types. A per group name has exactly one owner and unlisting it is sound. The sums and the max are named per hostname and shared by every group of that hostname, so no single group may unlist them.

ts::Metrics::Derived::remove_source is the counterpart to add_source: it drops one source and unlists the derived name only when the last source goes, and adding a source again relists it. Groups that do not publish a shared name now stop contributing to it instead of unlisting it. The per group names go the same way even though they have a single source, which has a small side benefit: the derived pass no longer recomputes a value into a name that is not published.

Both directions of 3 <-> 2 still retract and republish the sums as before, so nothing was given up to fix this.

Verified the new test fails against the old code: restoring the unlist call on the shared names fails exactly one assertion, the cross-mapping one.

Documentation was describing the defect

records.yaml.en.rst said the last group rebuilt decides whether the sums are published, and the enum comment said the same. That was me writing down a bug as a rule. Both now state that shared names are only withdrawn once no group of the hostname publishes them, so disagreeing mappings cannot hide each other's aggregate. Metrics.en.rst documents remove_source alongside add_source.

test_tsutil 16323, test_records 335, [ConnectionTracker] 87, and the three per_server autests pass in a Fedora 44 container.

[bneradt]

Re-reviewed 2ec417e. The original MATCH_HOST ownership issue is addressed: removing this group's source leaves other contributors listed. The duplicate CMake entry is also fixed.

Not ready for approval yet. I confirmed two remaining P2 issues in the existing inline discussions:

• The max metric rename collides with a valid MATCH_HOST hostname (max.example.test versus the max aggregate for example.test), merging unrelated sources and using whichever SUM/MAX operation registered first: #13666 (comment).
• The new public Derived::remove_source API can unlist a metric after a concurrent add_source has completed. Source membership and listing transitions need coordinated synchronization: #13666 (comment). ConnectionTracker's table lock serializes its own constructor calls, but does not protect independent users of this public API.

The sharing test is useful, but still does not cover the original live MATCH_HOST/MATCH_BOTH scenario; please add that regression alongside the fixes.

Validation: source/test review and clean git diff --check; no local build or test execution. Docs, Format, and RAT pass; the other 11 reported CI checks are still pending.

[Copilot]

🟡 Changes recommended

Unresolved critical test-lifecycle and moderate metric publication issues block approval.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (6)

doc/admin-guide/files/records.yaml.en.rst:2037

• The same wording says a group is discarded when its connection count reaches zero, but the production path releases the group from PoolableSession::release_outbound_connection_tracking() when the upstream session closes; TxnState::release() alone does not erase the table entry. As a result, keep-alive sessions can defer retraction beyond transaction completion. Please describe the session close/churn requirement rather than implying that any count-to-zero transition is sufficient.

   connection count reaches zero, so a change is picked up the next time that upstream is reopened.
   A group that never goes idle keeps whatever was in effect when it was created.

doc/admin-guide/files/records.yaml.en.rst:2099

• The new documentation says per-group names belong to one group, but the already-documented overridable MATCH_HOST/MATCH_BOTH combination can merge a MATCH_HOST per-group name with a MATCH_BOTH hostname sum (ConnectionTracker::Group::host_metric_name notes this in include/iocore/net/ConnectionTracker.h:244-247). In that case rebuilding one group only removes its source and the shared name can remain listed while another source uses it. Please qualify this statement so the documented retraction behavior matches the source-aware implementation.

   The per group metrics belong to a single group, so raising the value withdraws them as that group
   is rebuilt. The sums and the max are named per hostname and shared by its groups, so a group
   rebuilt for a value that does not publish them only stops contributing; they are withdrawn once
   no group of that hostname publishes them. Mappings that disagree for one hostname therefore

include/iocore/net/ConnectionTracker.h:113

• This describes the re-evaluation trigger as the connection count reaching zero, but transaction-level TxnState::release() only decrements the count; the group is removed by Group::release() from PoolableSession::release_outbound_connection_tracking() when the upstream session closes. With origin keep-alive, a group can therefore survive many drained transactions and retain the old setting. Document the session-lifetime condition here so operators do not expect a reload or transaction churn to retract the metrics.

   * A change is applied per group, when that group is next constructed, which happens on the first
   * connection after its count last fell to zero. A group that never goes idle keeps whatever was
   * in effect when it was created.

src/iocore/net/ConnectionTracker.cc:512

• This aggregate name can collide with a valid MATCH_HOST per-group name: a host named max.<fqdn> publishes current_connection.max.<fqdn> too. Derived::add_source then keeps the first operation for that name, so construction order can either add the unrelated host count to the hostname sum or let it inflate the hostname MAX. Use a namespace or escaping that cannot be produced by Group::metric_name, and cover both construction orders.

    std::string const max_name = "proxy.process.http.per_server.current_connection.max." + _host_metric_name;

src/iocore/net/ConnectionTracker.cc:527

• The PR description says the last group constructed decides whether a hostname's sums remain published when mappings disagree, but this branch only removes the current group's source. If an earlier group still contributes, a mode 0/2 group will not retract the shared sum; the updated records documentation says the opposite (that mappings cannot hide each other's aggregate). Please reconcile the documented contract with the implementation.

      // Stop contributing rather than unlist: every group of this hostname shares these names, so
      // one that does not want them must not remove a name another is still publishing.
      Metrics::Derived::remove_source(sum_names[0], _count_metric);
      Metrics::Derived::remove_source(sum_names[1], _count_total_metric);
      Metrics::Derived::remove_source(sum_names[2], _blocked_metric);

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

• This docstring calls the new publication mechanism a “tombstone,” but metrics are not deleted or reclaimed; the API and surrounding documentation call this unlist. That term is already used for cache slot deletion elsewhere, so it can mislead readers about the lifetime semantics this test is intended to describe.

    the store grew a tombstone, a name published while the setting was 0 kept reporting for the

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

[bneradt]

Re-reviewed c525ced. Approved.

The original shared-source ownership issue is addressed. The latest update also fixes the concurrent add/remove issue by performing the final relist/unlist transitions under the same lock as source membership changes, with a concurrent regression test added.

After reconsidering its practical likelihood, the mixed-match metric-name collision is a non-blocking follow-up, not a reason to hold this PR. I clarified that in the inline discussion.

All 14 reported CI checks pass, and git diff --check is clean. Validation here was source/test review and CI inspection; I did not run a local build or tests.