From 6e67e93ab47ec26bb93238d483e0e13087f3eaf1 Mon Sep 17 00:00:00 2001 From: Chris McFarlen Date: Tue, 1 Sep 2026 17:15:22 -0500 Subject: [PATCH 01/11] Withdraw per group metrics when metric_aggregate is raised metric_aggregate is dynamic and overridable, but the publication decision is made when a group is constructed and a published metric name was never removable. A name published while the setting was 0 therefore kept reporting for the life of the process, leaving per group and per hostname metrics side by side at metric_aggregate 2. AGGREGATE_ONLY now tombstones the per group names it declines to publish. A group is rebuilt on the first connection after its count falls to zero, so the change converges as groups go idle. --- include/iocore/net/ConnectionTracker.h | 5 + src/iocore/net/CMakeLists.txt | 1 + src/iocore/net/ConnectionTracker.cc | 27 +++- .../net/unit_tests/test_ConnectionTracker.cc | 147 ++++++++++++++++++ .../per_server_connection_max.test.py | 100 ++++++++++++ 5 files changed, 274 insertions(+), 6 deletions(-) diff --git a/include/iocore/net/ConnectionTracker.h b/include/iocore/net/ConnectionTracker.h index e18f6aa66c6..9c047a94ad2 100644 --- a/include/iocore/net/ConnectionTracker.h +++ b/include/iocore/net/ConnectionTracker.h @@ -94,6 +94,11 @@ class ConnectionTracker * is only a change of what is registered for publication, with no metric to migrate between the * two stores. * + * 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. Retracting a published name relies on the metric store's + * listing, see @c ts::Metrics::unlist. + * * The records layer validates and clamps this to 0..2. A plugin setting the overridable config * directly is not clamped, see @c METRIC_AGGREGATE_CONV; any other value behaves as * @c AGGREGATE_GROUP, publishing both the aggregate and the per group metrics. diff --git a/src/iocore/net/CMakeLists.txt b/src/iocore/net/CMakeLists.txt index 5de6afc44dc..c84ed8becfd 100644 --- a/src/iocore/net/CMakeLists.txt +++ b/src/iocore/net/CMakeLists.txt @@ -146,6 +146,7 @@ if(BUILD_TESTING) NetVCTest.cc unit_tests/test_ConnectionTracker.cc unit_tests/test_NetHandler.cc + unit_tests/test_ConnectionTracker.cc unit_tests/test_ProxyProtocol.cc unit_tests/test_SSLCertLookup.cc unit_tests/test_SSLNetVConnectionAsyncEp.cc diff --git a/src/iocore/net/ConnectionTracker.cc b/src/iocore/net/ConnectionTracker.cc index 28b3c7fa9cd..ca251537917 100644 --- a/src/iocore/net/ConnectionTracker.cc +++ b/src/iocore/net/ConnectionTracker.cc @@ -27,6 +27,7 @@ #include "swoc/IPAddr.h" #include +#include using namespace std::literals; @@ -499,6 +500,12 @@ ConnectionTracker::Group::Group(DirectionType direction, Key const &key, std::st Metrics::MetricType::GAUGE, _count_metric, Metrics::Derived::Op::MAX); } + std::array const published_names{ + "proxy.process.http.per_server.current_connection." + _metric_name, + "proxy.process.http.per_server.total_connection." + _metric_name, + "proxy.process.http.per_server.blocked_connection." + _metric_name, + }; + // AGGREGATE_ONLY suppresses the per group metrics to keep the published count proportional to // hostnames. Without an aggregate to stand in for them there would be nothing at all reported // for this group, so in that case publish them regardless. @@ -506,12 +513,20 @@ ConnectionTracker::Group::Group(DirectionType direction, Key const &key, std::st // 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. - Metrics::Derived::add_source("proxy.process.http.per_server.current_connection." + _metric_name, Metrics::MetricType::GAUGE, - _count_metric, Metrics::Derived::Op::SUM); - Metrics::Derived::add_source("proxy.process.http.per_server.total_connection." + _metric_name, Metrics::MetricType::COUNTER, - _count_total_metric, Metrics::Derived::Op::SUM); - Metrics::Derived::add_source("proxy.process.http.per_server.blocked_connection." + _metric_name, Metrics::MetricType::COUNTER, - _blocked_metric, Metrics::Derived::Op::SUM); + Metrics::Derived::add_source(published_names[0], Metrics::MetricType::GAUGE, _count_metric, Metrics::Derived::Op::SUM); + Metrics::Derived::add_source(published_names[1], Metrics::MetricType::COUNTER, _count_total_metric, + Metrics::Derived::Op::SUM); + Metrics::Derived::add_source(published_names[2], Metrics::MetricType::COUNTER, _blocked_metric, Metrics::Derived::Op::SUM); + } else { + // metric_aggregate is dynamic and overridable, so this group may well have published these + // names under an earlier value. A published name is never removed from the store, so without + // withdrawing them here they would report for the life of the process no matter what the + // setting says. The add_source calls above republish them if the setting changes back. + auto &metrics = Metrics::instance(); + + for (auto const &name : published_names) { + metrics.unlist(name); + } } if (dbg_ctl.on()) { diff --git a/src/iocore/net/unit_tests/test_ConnectionTracker.cc b/src/iocore/net/unit_tests/test_ConnectionTracker.cc index ccd9d169d6f..fe29cf687bd 100644 --- a/src/iocore/net/unit_tests/test_ConnectionTracker.cc +++ b/src/iocore/net/unit_tests/test_ConnectionTracker.cc @@ -22,10 +22,14 @@ */ #include "iocore/net/ConnectionTracker.h" +#include "iocore/net/Net.h" +#include "tscore/ink_inet.h" +#include "tsutil/Metrics.h" #include #include +#include TEST_CASE("Connection tracker server match conversion", "[libinknet][ConnectionTracker]") { @@ -61,3 +65,146 @@ TEST_CASE("Connection tracker server match conversion", "[libinknet][ConnectionT CHECK(match == ConnectionTracker::MATCH_BOTH); } } + +namespace +{ + +constexpr std::string_view FQDN{"unit.test.origin"}; + +// Whether the published store enumerates this name. Deliberately for_each rather than lookup(), +// because enumeration is what traffic_ctl, the JSONRPC record lookup and stats_over_http walk, and +// so is what "published" means to an operator. +bool +is_published(std::string_view metric_name) +{ + bool found = false; + + ts::Metrics::instance().for_each([&](std::string_view name, ts::Metrics::MetricType, int64_t) { + found |= (name == metric_name); + }); + + return found; +} + +std::string +group_metric(std::string_view stem, std::string_view addr) +{ + return std::string("proxy.process.http.per_server.").append(stem).append(".").append(FQDN).append(".").append(addr); +} + +std::string +host_metric(std::string_view stem) +{ + return std::string("proxy.process.http.per_server.").append(stem).append(".").append(FQDN); +} + +// One upstream connection, opened and closed, following the same path as production: HttpSM +// reserves and then drops the group into the PoolableSession, and the session releases it when the +// connection closes. Group::release() is what erases the group at a zero count, and only that makes +// the next transaction to the same upstream construct a fresh Group and re-evaluate +// metric_aggregate. TxnState::release() alone decrements without erasing. +void +open_and_close_connection(ConnectionTracker::TxnConfig const &txn, IpEndpoint const &addr) +{ + auto state = ConnectionTracker::obtain_outbound(txn, FQDN, addr); + + REQUIRE(state.is_active()); + state.reserve(); + + auto group = state.drop(); + group->release(); +} + +ConnectionTracker::TxnConfig & +test_config() +{ + // config_init keeps pointers to these for the records callbacks, so they must outlive the test. + static ConnectionTracker::GlobalConfig global; + static ConnectionTracker::TxnConfig txn; + static bool initialized = false; + + if (!initialized) { + ink_net_init(NET_SYSTEM_MODULE_PUBLIC_VERSION); + ConnectionTracker::config_init(&global, &txn, [](const char *, RecDataT, RecData, void *) -> int { return REC_ERR_OKAY; }); + initialized = true; + } + + return txn; +} + +} // namespace + +TEST_CASE("ConnectionTracker aggregate metric publication", "[libinknet][ConnectionTracker]") +{ + auto &txn = test_config(); + + txn.metric_enabled = 1; + txn.server_match = ConnectionTracker::MATCH_BOTH; + + IpEndpoint addr; + REQUIRE(ats_ip_pton("10.9.8.7:443", &addr) == 0); + + const std::string current_group = group_metric("current_connection", "10.9.8.7:443"); + const std::string total_group = group_metric("total_connection", "10.9.8.7:443"); + const std::string blocked_group = group_metric("blocked_connection", "10.9.8.7:443"); + + SECTION("AGGREGATE_NONE publishes the per group metrics") + { + txn.metric_aggregate = ConnectionTracker::AGGREGATE_NONE; + open_and_close_connection(txn, addr); + + CHECK(is_published(current_group)); + CHECK(is_published(total_group)); + CHECK(is_published(blocked_group)); + CHECK_FALSE(is_published(host_metric("current_connection_max"))); + } + + SECTION("switching to AGGREGATE_ONLY retracts an already published per group metric") + { + // This is 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_ONLY; + open_and_close_connection(txn, addr); + + CHECK_FALSE(is_published(current_group)); + CHECK_FALSE(is_published(total_group)); + CHECK_FALSE(is_published(blocked_group)); + + // The aggregate stands in for them. + CHECK(is_published(host_metric("current_connection"))); + CHECK(is_published(host_metric("total_connection"))); + CHECK(is_published(host_metric("blocked_connection"))); + CHECK(is_published(host_metric("current_connection_max"))); + } + + SECTION("switching back to AGGREGATE_GROUP republishes them") + { + txn.metric_aggregate = ConnectionTracker::AGGREGATE_ONLY; + open_and_close_connection(txn, addr); + REQUIRE_FALSE(is_published(current_group)); + + txn.metric_aggregate = ConnectionTracker::AGGREGATE_GROUP; + open_and_close_connection(txn, addr); + + CHECK(is_published(current_group)); + CHECK(is_published(host_metric("current_connection"))); + } + + SECTION("AGGREGATE_ONLY still publishes a group that has no aggregate to stand in for it") + { + // Only MATCH_BOTH yields a hostname to gather under, so a MATCH_PORT group has no aggregate. + // Suppressing it would report nothing at all for that upstream. + txn.server_match = ConnectionTracker::MATCH_PORT; + txn.metric_aggregate = ConnectionTracker::AGGREGATE_ONLY; + + IpEndpoint port_addr; + REQUIRE(ats_ip_pton("10.9.8.6:80", &port_addr) == 0); + open_and_close_connection(txn, port_addr); + + CHECK(is_published("proxy.process.http.per_server.current_connection.10.9.8.6:80")); + } +} diff --git a/tests/gold_tests/origin_connection/per_server_connection_max.test.py b/tests/gold_tests/origin_connection/per_server_connection_max.test.py index 05f2c5587c1..da2f93c9809 100644 --- a/tests/gold_tests/origin_connection/per_server_connection_max.test.py +++ b/tests/gold_tests/origin_connection/per_server_connection_max.test.py @@ -384,6 +384,13 @@ def _test_metrics_while_held(self) -> None: f'per_server.current_connection_max.multi.origin.com {group_max}', 'While held open, current_connection_max should be the largest single group current ' 'count (MAX), not the sum across the two groups.') + # The per group names end in the address, so anything matching this is a group metric and + # not the hostname aggregate. Every other assertion in this file is a ContainsExpression, + # which cannot catch a metric that should not be there at all. + tr.Processes.Default.Streams.All += Testers.ExcludesExpression( + r'per_server\.\w+_connection\.multi\.origin\.com\.\d', + 'At metric_aggregate 2 the per group metrics must stay hidden, leaving only the ' + 'hostname aggregate published.') def _test_metrics_after_drain(self) -> None: """After traffic drains and a further sync tick passes, both live gauges must read 0. @@ -580,9 +587,102 @@ def run(self) -> None: self._test_metrics() +class AggregateRetractionTest: + """Verify that raising metric_aggregate to 2 withdraws already published per group metrics. + + metric_aggregate is dynamic, but the publication decision is made in the ConnectionTracker + Group constructor, and a published metric name is never removed from the metric store. Before + the store grew a tombstone, a name published while the setting was 0 kept reporting for the + life of the process no matter what the setting was changed to, which is exactly what was seen + in production: per group and per hostname metrics side by side at metric_aggregate 2. + + Origin keep alive is disabled so each request opens and closes its own upstream connection. + That returns the group count to zero, which erases the group, so the next request constructs a + fresh one and re-evaluates the setting. A group that never goes idle would keep whatever was in + effect when it was created. + """ + + def __init__(self) -> None: + """Configure the processes for the test.""" + self._dns = _dns + self._server = Test.MakeHttpBinServer("retract_server") + self._configure_trafficserver() + + def _configure_trafficserver(self) -> None: + """Configure an ATS that starts out publishing the per group metrics.""" + self._ts = Test.MakeATSProcess("retract_ts") + self._ts.Disk.records_config.update( + { + **_STAT_SYNC_RECORDS, + 'proxy.config.dns.nameservers': f"127.0.0.1:{self._dns.Variables.Port}", + 'proxy.config.dns.resolv_conf': 'NULL', + 'proxy.config.http.per_server.connection.metric_enabled': 1, + # Start with the per group metrics published, then raise it at runtime below. + 'proxy.config.http.per_server.connection.metric_aggregate': 0, + 'proxy.config.http.per_server.connection.match': 'both', + # Force the upstream connection closed after each transaction so the group is + # erased and the next request rebuilds it. + 'proxy.config.http.keep_alive_enabled_out': 0, + }) + self._ts.Disk.remap_config.AddLine( + f"map http://retract.origin.com/ http://retract.origin.com:{self._server.Variables.Port}/") + + 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 + + def run(self) -> None: + """Publish the per group metrics, raise the setting, then verify they are withdrawn.""" + tr = Test.AddTestRun("Drive traffic with the per group metrics published") + _use_shared_dns(tr) + tr.Processes.Default.StartBefore(self._server) + tr.Processes.Default.StartBefore(self._ts) + self._curl(tr) + + tr = Test.AddTestRun("Verify the per group metrics are published at metric_aggregate 0") + tr.Processes.Default.Command = f'sleep {_STAT_SYNC_WAIT_SECONDS}; traffic_ctl metric match per_server' + tr.Processes.Default.ReturnCode = 0 + tr.Processes.Default.Env = self._ts.Env + tr.Processes.Default.TimeOut = _STAT_SYNC_WAIT_SECONDS + 30 + tr.Processes.Default.Streams.All = Testers.ContainsExpression( + r'per_server\.current_connection\.retract\.origin\.com\.\d', + 'At metric_aggregate 0 the per group metric is published under its own name. Without ' + 'this the retraction below would be vacuous.') + tr.StillRunningAfter = self._ts + + tr = Test.AddTestRun("Raise metric_aggregate to 2") + tr.Processes.Default.Command = ( + 'traffic_ctl config set proxy.config.http.per_server.connection.metric_aggregate 2 && ' + 'traffic_ctl config reload') + tr.Processes.Default.ReturnCode = 0 + tr.Processes.Default.Env = self._ts.Env + tr.Processes.Default.TimeOut = 30 + tr.StillRunningAfter = self._ts + + tr = Test.AddTestRun("Drive traffic again so the group is rebuilt under the new setting") + self._curl(tr) + + tr = Test.AddTestRun("Verify the per group metrics were withdrawn") + tr.Processes.Default.Command = f'sleep {_STAT_SYNC_WAIT_SECONDS}; traffic_ctl metric match per_server' + tr.Processes.Default.ReturnCode = 0 + tr.Processes.Default.Env = self._ts.Env + tr.Processes.Default.TimeOut = _STAT_SYNC_WAIT_SECONDS + 30 + tr.Processes.Default.Streams.All = Testers.ExcludesExpression( + r'per_server\.\w+_connection\.retract\.origin\.com\.\d', + 'Once metric_aggregate is 2 and the group has been rebuilt, the per group metrics must ' + 'no longer be published, even though they were published earlier in this process.') + tr.Processes.Default.Streams.All += Testers.ContainsExpression( + r'per_server\.current_connection_max\.retract\.origin\.com', + 'The hostname aggregate stands in for the withdrawn per group metrics.') + tr.StillRunningAfter = self._ts + + PerServerConnectionMaxTest().run() ConnectMethodTest(3, metric_aggregate=2).run(blocked=2, gold_file="gold/two_503_congested.gold") ConnectMethodTest(0, metric_aggregate=1).run(blocked=0, gold_file="gold/two_200_ok.gold") MultiGroupAggregateTest().run() MetricOverrideTest().run() AggregateOnlyWithoutHostAggregateTest().run() +AggregateRetractionTest().run() From 08e59e7e062d890ac5a012f7856732cc50cdc502 Mon Sep 17 00:00:00 2001 From: Chris McFarlen Date: Tue, 1 Sep 2026 18:03:06 -0500 Subject: [PATCH 02/11] Rename the per hostname MAX metric to current_connection.max ATS metric names separate a qualifier with a dot, as in proxy.process.eventloop.time.max, not an underscore. The aggregate added in #13506 has only ever existed on master, so renaming it now costs nothing. Also wait for the reconfigure in the retraction autest: http_config_cb schedules it a second out, so a request made as soon as traffic_ctl returns is still served by the previous configuration. --- .../statistics/core/http-connection.en.rst | 8 ++-- src/iocore/net/ConnectionTracker.cc | 2 +- .../net/unit_tests/test_ConnectionTracker.cc | 4 +- .../per_server_connection_max.test.py | 37 ++++++++++++++----- 4 files changed, 35 insertions(+), 16 deletions(-) diff --git a/doc/admin-guide/monitoring/statistics/core/http-connection.en.rst b/doc/admin-guide/monitoring/statistics/core/http-connection.en.rst index 47075ce6fa9..120462e52b5 100644 --- a/doc/admin-guide/monitoring/statistics/core/http-connection.en.rst +++ b/doc/admin-guide/monitoring/statistics/core/http-connection.en.rst @@ -237,18 +237,18 @@ blocked_connection For a hostname aggregate, ```` is one of those three, each summed across the groups of that hostname which have aggregation enabled, plus: -current_connection_max +current_connection.max Gauge. The largest ``current_connection`` value among the groups of that hostname at the moment of sampling, so the maximum rather than the sum of the groups' current counts. This is useful because :ts:cv:`proxy.config.http.per_server.connection.max` is enforced per group rather than per hostname, so the busiest group is what determines whether connections are about to be blocked. Like ``current_connection`` it rises and falls with traffic and is not a high-water - mark. There is no per group ``current_connection_max``; it exists only as a hostname aggregate. + mark. There is no per group ``current_connection.max``; it exists only as a hostname aggregate. Because :ts:cv:`proxy.config.http.per_server.connection.metric_aggregate` is overridable, a group joins its hostname's aggregate only if the mapping that first opened that upstream had aggregation 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 +sums cover a subset of the groups and ``current_connection.max`` takes its maximum over that same subset, with nothing in the metric to indicate it. Keeping the setting uniform across the mappings for a hostname avoids this. @@ -261,7 +261,7 @@ Every published per server metric is recomputed periodically, currently every 5 on every connection event, so a reader sees a value up to that interval old. This is true of the hostname aggregates and of the published per group metrics alike: those are mirrored from the internal ones by the same periodic mechanism, not written as connections open and -close. It applies to ``current_connection_max`` too, which reports the maximum across groups as of +close. It applies to ``current_connection.max`` too, which reports the maximum across groups as of the last sample rather than a running peak. To obtain the peak over a longer window, compute a maximum over time from this gauge in the monitoring system. diff --git a/src/iocore/net/ConnectionTracker.cc b/src/iocore/net/ConnectionTracker.cc index ca251537917..a54ba37315b 100644 --- a/src/iocore/net/ConnectionTracker.cc +++ b/src/iocore/net/ConnectionTracker.cc @@ -496,7 +496,7 @@ ConnectionTracker::Group::Group(DirectionType direction, Key const &key, std::st // The largest current count among this hostname's groups, sampled. Deliberately taken over // the instantaneous gauge rather than each group's all time peak, so the value falls again // and a maximum over time can be computed by whatever scrapes it. - Metrics::Derived::add_source("proxy.process.http.per_server.current_connection_max." + _host_metric_name, + Metrics::Derived::add_source("proxy.process.http.per_server.current_connection.max." + _host_metric_name, Metrics::MetricType::GAUGE, _count_metric, Metrics::Derived::Op::MAX); } diff --git a/src/iocore/net/unit_tests/test_ConnectionTracker.cc b/src/iocore/net/unit_tests/test_ConnectionTracker.cc index fe29cf687bd..f26f0e484c0 100644 --- a/src/iocore/net/unit_tests/test_ConnectionTracker.cc +++ b/src/iocore/net/unit_tests/test_ConnectionTracker.cc @@ -156,7 +156,7 @@ TEST_CASE("ConnectionTracker aggregate metric publication", "[libinknet][Connect CHECK(is_published(current_group)); CHECK(is_published(total_group)); CHECK(is_published(blocked_group)); - CHECK_FALSE(is_published(host_metric("current_connection_max"))); + CHECK_FALSE(is_published(host_metric("current_connection.max"))); } SECTION("switching to AGGREGATE_ONLY retracts an already published per group metric") @@ -178,7 +178,7 @@ TEST_CASE("ConnectionTracker aggregate metric publication", "[libinknet][Connect CHECK(is_published(host_metric("current_connection"))); CHECK(is_published(host_metric("total_connection"))); CHECK(is_published(host_metric("blocked_connection"))); - CHECK(is_published(host_metric("current_connection_max"))); + CHECK(is_published(host_metric("current_connection.max"))); } SECTION("switching back to AGGREGATE_GROUP republishes them") diff --git a/tests/gold_tests/origin_connection/per_server_connection_max.test.py b/tests/gold_tests/origin_connection/per_server_connection_max.test.py index da2f93c9809..1b8f97ec8af 100644 --- a/tests/gold_tests/origin_connection/per_server_connection_max.test.py +++ b/tests/gold_tests/origin_connection/per_server_connection_max.test.py @@ -36,6 +36,11 @@ # scheduling jitter and the traffic_ctl round trip rather than racing the tick. _STAT_SYNC_WAIT_SECONDS: int = 2 +# How long to wait after changing an overridable record at runtime before driving traffic that +# should see the new value. http_config_cb schedules the reconfigure one second out, so a request +# made immediately after traffic_ctl returns is still served by the previous HttpConfigParams. +_CONFIG_APPLY_WAIT_SECONDS: int = 5 + # The records.yaml settings every ATS instance in this file needs for the waits above to hold. _STAT_SYNC_RECORDS: dict = { 'proxy.config.raw_stat_sync_interval_ms': _STAT_SYNC_INTERVAL_MS, @@ -134,7 +139,7 @@ def _test_metrics(self) -> None: # A 'port' match has one group per address:port and no hostname, so no aggregate should be # registered for it at all. tr.Processes.Default.Streams.All += Testers.ExcludesExpression( - 'per_server.current_connection_max.', 'A non-"both" match type must not register a hostname aggregate.') + 'per_server.current_connection.max.', 'A non-"both" match type must not register a hostname aggregate.') def run(self) -> None: """Configure the TestRun.""" @@ -234,7 +239,7 @@ def _test_metrics(self, blocked) -> None: f'per_server.total_connection.{group_name} 5', 'The per group metric should be published at AGGREGATE_GROUP.') else: # AGGREGATE_ONLY keeps the per group metrics hidden, so none of the three per group - # names may appear in a normal query. current_connection_max is not among them: it only + # names may appear in a normal query. current_connection.max is not among them: it only # ever exists as a hostname aggregate, never per group. for counter in ('current_connection', 'total_connection', 'blocked_connection'): tr.Processes.Default.Streams.All += Testers.ExcludesExpression( @@ -294,7 +299,7 @@ class MultiGroupAggregateTest: distinct groups sharing one host aggregate. The two groups are given different concurrency so the SUM and the MAX are distinguishable from each other. - current_connection and current_connection_max are instantaneous gauges recomputed from the live + current_connection and current_connection.max are instantaneous gauges recomputed from the live per group values every ~5s, so they rise and fall with traffic rather than remembering a peak. Observing a non-zero value therefore requires holding connections open across a sync tick. The most robust assertion, and the one that actually distinguishes this instantaneous behavior from @@ -381,8 +386,8 @@ def _test_metrics_while_held(self) -> None: 'While held open, the host aggregate current_connection should be the SUM of the ' 'currently open connections across both groups.') tr.Processes.Default.Streams.All += Testers.ContainsExpression( - f'per_server.current_connection_max.multi.origin.com {group_max}', - 'While held open, current_connection_max should be the largest single group current ' + f'per_server.current_connection.max.multi.origin.com {group_max}', + 'While held open, current_connection.max should be the largest single group current ' 'count (MAX), not the sum across the two groups.') # The per group names end in the address, so anything matching this is a group metric and # not the hostname aggregate. Every other assertion in this file is a ContainsExpression, @@ -410,8 +415,8 @@ def _test_metrics_after_drain(self) -> None: 'per_server.current_connection.multi.origin.com 0', 'Once all connections close, the host aggregate current_connection must drain to 0.') tr.Processes.Default.Streams.All += Testers.ContainsExpression( - 'per_server.current_connection_max.multi.origin.com 0', - 'Once all connections close, current_connection_max must also come back down to 0: it ' + 'per_server.current_connection.max.multi.origin.com 0', + 'Once all connections close, current_connection.max must also come back down to 0: it ' 'is a live gauge, not a monotone peak.') def run(self) -> None: @@ -653,12 +658,26 @@ def run(self) -> None: tr.StillRunningAfter = self._ts tr = Test.AddTestRun("Raise metric_aggregate to 2") + # http_config_cb schedules the reconfigure a second after the record changes + # (HttpConfig.cc), so the new HttpConfigParams is not in place the instant traffic_ctl + # returns. Without this wait the next request is served by the old configuration and + # rebuilds the group under the old setting, which looks exactly like a failure to retract. tr.Processes.Default.Command = ( 'traffic_ctl config set proxy.config.http.per_server.connection.metric_aggregate 2 && ' - 'traffic_ctl config reload') + 'traffic_ctl config reload && ' + f'sleep {_CONFIG_APPLY_WAIT_SECONDS}') + tr.Processes.Default.ReturnCode = 0 + tr.Processes.Default.Env = self._ts.Env + tr.Processes.Default.TimeOut = _CONFIG_APPLY_WAIT_SECONDS + 30 + tr.StillRunningAfter = self._ts + + tr = Test.AddTestRun("Verify the new metric_aggregate is in effect") + tr.Processes.Default.Command = 'traffic_ctl config get proxy.config.http.per_server.connection.metric_aggregate' tr.Processes.Default.ReturnCode = 0 tr.Processes.Default.Env = self._ts.Env tr.Processes.Default.TimeOut = 30 + tr.Processes.Default.Streams.All = Testers.ContainsExpression( + r'metric_aggregate: 2', 'The record must carry the new value before behavior is asserted against it.') tr.StillRunningAfter = self._ts tr = Test.AddTestRun("Drive traffic again so the group is rebuilt under the new setting") @@ -674,7 +693,7 @@ def run(self) -> None: 'Once metric_aggregate is 2 and the group has been rebuilt, the per group metrics must ' 'no longer be published, even though they were published earlier in this process.') tr.Processes.Default.Streams.All += Testers.ContainsExpression( - r'per_server\.current_connection_max\.retract\.origin\.com', + r'per_server\.current_connection\.max\.retract\.origin\.com', 'The hostname aggregate stands in for the withdrawn per group metrics.') tr.StillRunningAfter = self._ts From 6d2c9ac7e8daa4c5476c4f03f238c9a3b113f059 Mon Sep 17 00:00:00 2001 From: Chris McFarlen Date: Tue, 1 Sep 2026 19:18:39 -0500 Subject: [PATCH 03/11] Split the aggregate-only mode into max-only and sums-plus-max The requirement for the suppressed-per-group mode was a single metric per hostname, the max, rather than the sums as well. Mode 2 is now that max alone, and mode 3 is the sums and the max, for when the totals are wanted too. Mode 1 is unchanged. The sums are withdrawn the same way the per group metrics are when a mode stops asking for them. --- doc/admin-guide/files/records.yaml.en.rst | 64 +++++++----- .../statistics/core/http-connection.en.rst | 12 ++- include/iocore/net/ConnectionTracker.h | 47 ++++++--- src/iocore/net/ConnectionTracker.cc | 82 ++++++++++------ .../net/unit_tests/test_ConnectionTracker.cc | 98 ++++++++++++++++--- src/records/RecordsConfig.cc | 2 +- .../per_server_connection_max.test.py | 51 ++++++---- 7 files changed, 242 insertions(+), 114 deletions(-) diff --git a/doc/admin-guide/files/records.yaml.en.rst b/doc/admin-guide/files/records.yaml.en.rst index 5d75b68bafc..b05e6c85697 100644 --- a/doc/admin-guide/files/records.yaml.en.rst +++ b/doc/admin-guide/files/records.yaml.en.rst @@ -2033,13 +2033,15 @@ Origin Server Connect Attempts this setting resolve to the same group -- that is, the same key under :ts:cv:`proxy.config.http.per_server.connection.match` -- the transaction that creates the group 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 |TS| is restarted. This - affects only which metrics exist; enforcement of + 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. + + Disabling this setting does not retire metrics that are already published: they stay in place, + frozen at their last sampled value, until |TS| is restarted. Changing + :ts:cv:`proxy.config.http.per_server.connection.metric_aggregate` does retract what it no longer + asks for, as each group is rebuilt. + + This affects only which metrics exist; enforcement of :ts:cv:`proxy.config.http.per_server.connection.max` uses the group's own connection count and is unaffected. @@ -2051,24 +2053,30 @@ Origin Server Connect Attempts :ts:cv:`proxy.config.http.per_server.connection.metric_enabled`. Has no effect when that setting is ``0``. - A per hostname aggregate sums a counter across every group belonging to that hostname that has - aggregation enabled, and exists only for + There are two kinds of per hostname aggregate. The *sums* add ``current_connection``, + ``total_connection`` and ``blocked_connection`` across every group belonging to that hostname + that has aggregation enabled. The *max* is ``current_connection.max``, the largest + ``current_connection`` among those groups, which is the one that answers how close the busiest + group is to :ts:cv:`proxy.config.http.per_server.connection.max`. Both exist only for :ts:cv:`match type ` ``both``, since that is the only match type whose group key carries the hostname. See :ref:`per-server-connection-metrics`. - ===== ====================================================================================== - Value Effect - ===== ====================================================================================== - ``0`` No aggregates. The per group metrics are published under their own names. - ``1`` Publish the per hostname aggregates and the per group metrics. - ``2`` Publish only the per hostname aggregates. The per group metrics from which they are - computed are collected but not published, which keeps the number of published metrics - proportional to hostnames rather than to groups. - ===== ====================================================================================== + ===== =========== ====== ===== + Value Per group Sums Max + ===== =========== ====== ===== + ``0`` published no no + ``1`` published yes yes + ``2`` hidden no yes + ``3`` hidden yes yes + ===== =========== ====== ===== - With value ``2``, a group that has no aggregate to belong to -- any match type other than - ``both`` -- has its per group metrics published anyway, since otherwise nothing at all would be - reported for it. + ``2`` is the smallest useful configuration: one metric per hostname. ``3`` adds that hostname's + totals. Both keep the number of published metrics proportional to hostnames rather than to + groups. + + With values ``2`` and ``3``, a group that has no aggregate to belong to -- any match type other + than ``both`` -- has its per group metrics published anyway, since otherwise nothing at all would + be reported for it. Values ``0`` and ``1`` can produce a very large number of metrics when the match type includes the address or port, since there is then one set per address and port rather than one per hostname. @@ -2079,12 +2087,14 @@ Origin Server Connect Attempts upstream had aggregation enabled, so mappings that disagree for one hostname produce an aggregate that covers only part of it. - The reload is one-directional for the same reason given under - :ts:cv:`proxy.config.http.per_server.connection.metric_enabled`. Raising the value takes effect - as upstreams reconnect, but moving to ``2`` does not hide per group metrics that are already - published, and moving from ``1`` to ``0`` does not stop the hostname aggregates from publishing. - Reducing the number of published metrics therefore requires a restart, which matters most for - ``2``, the value chosen specifically to bound that number. + A change in either direction takes effect as upstreams reconnect: a group publishes what the + new value asks for and withdraws what it does not, when that group is next rebuilt. Metrics + withdrawn this way stop appearing in :program:`traffic_ctl` output and in the other metric + consumers; they are not destroyed, and moving back republishes them with their accumulated + values intact. + + Because the sums are named per hostname rather than per group, where the mappings for one + hostname disagree about this setting the last group rebuilt decides whether they are published. .. ts:cv:: CONFIG proxy.config.http.per_server.connection.metric_prefix STRING NULL :reloadable: diff --git a/doc/admin-guide/monitoring/statistics/core/http-connection.en.rst b/doc/admin-guide/monitoring/statistics/core/http-connection.en.rst index 120462e52b5..d5b438fcb01 100644 --- a/doc/admin-guide/monitoring/statistics/core/http-connection.en.rst +++ b/doc/admin-guide/monitoring/statistics/core/http-connection.en.rst @@ -234,8 +234,10 @@ blocked_connection Counter. The total number of connection attempts to the group blocked by :ts:cv:`proxy.config.http.per_server.connection.max`. Never decreases. -For a hostname aggregate, ```` is one of those three, each summed across the groups of that -hostname which have aggregation enabled, plus: +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 +:ts:cv:`metric_aggregate ` ``1`` and +``3``. The *max*, published at ``1``, ``2`` and ``3``, is: current_connection.max Gauge. The largest ``current_connection`` value among the groups of that hostname at the moment @@ -265,9 +267,9 @@ close. It applies to ``current_connection.max`` too, which reports the maximum a the last sample rather than a running peak. To obtain the peak over a longer window, compute a maximum over time from this gauge in the monitoring system. -At :ts:cv:`metric_aggregate ` value ``2`` -the per group metrics still exist internally, since the aggregates are computed from them, but are not -published. They can be listed with ``traffic_ctl metric match per_server --include-hidden``, which +At :ts:cv:`metric_aggregate ` values +``2`` and ``3`` the per group metrics still exist internally, since the aggregates are computed from +them, but are not published. They can be listed with ``traffic_ctl metric match per_server --include-hidden``, which reads them directly and so is not subject to the sampling delay above. That visibility is intended for debugging and is not a stable interface: the existence, granularity and naming of the per group metrics may change independently of the published aggregates. diff --git a/include/iocore/net/ConnectionTracker.h b/include/iocore/net/ConnectionTracker.h index 9c047a94ad2..50bc75daa64 100644 --- a/include/iocore/net/ConnectionTracker.h +++ b/include/iocore/net/ConnectionTracker.h @@ -79,16 +79,30 @@ class ConnectionTracker * * This is independent of @c TxnConfig::metric_enabled, which decides only whether per server * metrics exist for a group at all. The per group metrics are always created in the hidden metric - * store; what varies here is what gets published from them: - * - @c AGGREGATE_NONE: no aggregate. The per group metrics are published under their own names. - * This is the default and matches the behavior of releases that had no aggregate support. - * - @c AGGREGATE_GROUP: the per hostname aggregates are published, and so are the per group - * metrics they are computed from. - * - @c AGGREGATE_ONLY: the per hostname aggregates are published and the per group metrics stay - * hidden, which keeps the published metric count proportional to hostnames rather than to - * groups. Where a group has no aggregate to belong to -- see @c Group::host_metric_name, which - * only yields a name for match type @c MATCH_BOTH -- the per group metrics are published - * anyway, since otherwise nothing at all would be reported for that group. + * store; what varies here is what gets published from them. + * + * Two kinds of per hostname aggregate exist. The *sums* are @c current_connection, + * @c total_connection and @c blocked_connection added across the groups of a hostname. The *max* + * is @c current_connection.max, the largest @c current_connection among those groups. Which of + * them are published, and whether the per group metrics are published alongside, is what this + * selects: + * + * | value | per group | sums | max | + * |----------------------|-----------|------|-----| + * | @c AGGREGATE_NONE | yes | no | no | + * | @c AGGREGATE_GROUP | yes | yes | yes | + * | @c AGGREGATE_MAX | no | no | yes | + * | @c AGGREGATE_SUM | no | yes | yes | + * + * @c AGGREGATE_NONE is the default and matches the behavior of releases that had no aggregate + * support. @c AGGREGATE_MAX is the smallest useful configuration: one metric per hostname, + * answering how close the busiest group is to @c per_server.connection.max. @c AGGREGATE_SUM adds + * the totals for that hostname. Both keep the published metric count proportional to hostnames + * rather than to groups. + * + * Where a group has no aggregate to belong to -- see @c Group::host_metric_name, which only + * yields a name for match type @c MATCH_BOTH -- the per group metrics are published whatever this + * says, since otherwise nothing at all would be reported for that group. * * Keeping the per group metrics in the hidden store in every case means changing this at runtime * is only a change of what is registered for publication, with no metric to migrate between the @@ -97,16 +111,19 @@ class ConnectionTracker * 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. Retracting a published name relies on the metric store's - * listing, see @c ts::Metrics::unlist. + * listing, see @c ts::Metrics::unlist. The sums are named per hostname rather than per group, so + * where the mappings for one hostname disagree about this setting, the last group constructed + * decides whether they are published. * - * The records layer validates and clamps this to 0..2. A plugin setting the overridable config + * The records layer validates and clamps this to 0..3. A plugin setting the overridable config * directly is not clamped, see @c METRIC_AGGREGATE_CONV; any other value behaves as - * @c AGGREGATE_GROUP, publishing both the aggregate and the per group metrics. + * @c AGGREGATE_GROUP, publishing everything. */ enum MetricAggregate : int { AGGREGATE_NONE = 0, ///< No hostname aggregate; the per group metrics are published. - AGGREGATE_GROUP = 1, ///< Hostname aggregates published, along with the per group metrics. - AGGREGATE_ONLY = 2, ///< Hostname aggregates published, per group metrics kept hidden. + AGGREGATE_GROUP = 1, ///< Hostname sums and max, published along with the per group metrics. + AGGREGATE_MAX = 2, ///< Hostname max only; the per group metrics stay hidden. + AGGREGATE_SUM = 3, ///< Hostname sums and max; the per group metrics stay hidden. }; /// Per transaction configuration values. diff --git a/src/iocore/net/ConnectionTracker.cc b/src/iocore/net/ConnectionTracker.cc index a54ba37315b..fad0380f10d 100644 --- a/src/iocore/net/ConnectionTracker.cc +++ b/src/iocore/net/ConnectionTracker.cc @@ -76,8 +76,8 @@ const MgmtConverter ConnectionTracker::SERVER_MATCH_CONV{ // records paths do the range checking instead -- records.yaml validates the value and the reload // callbacks below clamp -- so an out of range value is only reachable by a plugin that sets one // deliberately. Both settings degrade safely if that happens: any non-zero metric_enabled enables -// metrics, and any metric_aggregate outside 0..2 publishes both the aggregate and the per group -// metrics, the same as AGGREGATE_GROUP. +// metrics, and any metric_aggregate outside 0..3 publishes everything, the same as +// AGGREGATE_GROUP. const MgmtConverter ConnectionTracker::METRIC_ENABLED_CONV{ [](const void *data) -> MgmtInt { return static_cast(*static_cast(data)); }, [](void *data, MgmtInt i) -> void { @@ -188,7 +188,7 @@ Config_Update_Conntrack_Metric_Aggregate(const char * /* name ATS_UNUSED */, Rec if (RECD_INT == dtype) { auto level = std::clamp(static_cast(data.rec_int), static_cast(ConnectionTracker::AGGREGATE_NONE), - static_cast(ConnectionTracker::AGGREGATE_ONLY)); + static_cast(ConnectionTracker::AGGREGATE_SUM)); config->metric_aggregate = static_cast(level); return true; } @@ -486,45 +486,65 @@ ConnectionTracker::Group::Group(DirectionType direction, Key const &key, std::st std::string _host_metric_name = host_metric_name(key, fqdn, _global_config->metric_prefix); bool const has_aggregate = !_host_metric_name.empty(); - if (has_aggregate && metric_aggregate != AGGREGATE_NONE) { - Metrics::Derived::add_source("proxy.process.http.per_server.current_connection." + _host_metric_name, - Metrics::MetricType::GAUGE, _count_metric, Metrics::Derived::Op::SUM); - Metrics::Derived::add_source("proxy.process.http.per_server.total_connection." + _host_metric_name, - Metrics::MetricType::COUNTER, _count_total_metric, Metrics::Derived::Op::SUM); - Metrics::Derived::add_source("proxy.process.http.per_server.blocked_connection." + _host_metric_name, - Metrics::MetricType::COUNTER, _blocked_metric, Metrics::Derived::Op::SUM); - // The largest current count among this hostname's groups, sampled. Deliberately taken over - // the instantaneous gauge rather than each group's all time peak, so the value falls again - // and a maximum over time can be computed by whatever scrapes it. - Metrics::Derived::add_source("proxy.process.http.per_server.current_connection.max." + _host_metric_name, - Metrics::MetricType::GAUGE, _count_metric, Metrics::Derived::Op::MAX); + // A plugin can set an out of range value through the overridable config, see + // METRIC_AGGREGATE_CONV. Anything unrecognized publishes everything. + if (metric_aggregate < AGGREGATE_NONE || metric_aggregate > AGGREGATE_SUM) { + metric_aggregate = AGGREGATE_GROUP; } - std::array const published_names{ + // See MetricAggregate for the table these three implement. A group with no hostname to + // aggregate under keeps its own metrics whatever the setting says, since suppressing them would + // report nothing at all for that upstream. + bool const publish_sums = has_aggregate && (metric_aggregate == AGGREGATE_GROUP || metric_aggregate == AGGREGATE_SUM); + bool const publish_max = has_aggregate && metric_aggregate != AGGREGATE_NONE; + bool const publish_group = !has_aggregate || metric_aggregate == AGGREGATE_NONE || metric_aggregate == AGGREGATE_GROUP; + + std::array const sum_names{ + "proxy.process.http.per_server.current_connection." + _host_metric_name, + "proxy.process.http.per_server.total_connection." + _host_metric_name, + "proxy.process.http.per_server.blocked_connection." + _host_metric_name, + }; + std::array const group_names{ "proxy.process.http.per_server.current_connection." + _metric_name, "proxy.process.http.per_server.total_connection." + _metric_name, "proxy.process.http.per_server.blocked_connection." + _metric_name, }; + std::string const max_name = "proxy.process.http.per_server.current_connection.max." + _host_metric_name; + + auto &metrics = Metrics::instance(); + + // metric_aggregate is dynamic and overridable, so this group may well have published a name + // under an earlier value. A published name is never removed from the store, so without + // withdrawing it here it would report for the life of the process no matter what the setting + // says. Re-registering a source republishes it if the setting changes back. + if (publish_sums) { + Metrics::Derived::add_source(sum_names[0], Metrics::MetricType::GAUGE, _count_metric, Metrics::Derived::Op::SUM); + Metrics::Derived::add_source(sum_names[1], Metrics::MetricType::COUNTER, _count_total_metric, Metrics::Derived::Op::SUM); + Metrics::Derived::add_source(sum_names[2], Metrics::MetricType::COUNTER, _blocked_metric, Metrics::Derived::Op::SUM); + } else if (has_aggregate) { + for (auto const &name : sum_names) { + metrics.unlist(name); + } + } - // AGGREGATE_ONLY suppresses the per group metrics to keep the published count proportional to - // hostnames. Without an aggregate to stand in for them there would be nothing at all reported - // for this group, so in that case publish them regardless. - if (metric_aggregate != AGGREGATE_ONLY || !has_aggregate) { + if (publish_max) { + // The largest current count among this hostname's groups, sampled. Deliberately taken over + // the instantaneous gauge rather than each group's all time peak, so the value falls again + // and a maximum over time can be computed by whatever scrapes it. + Metrics::Derived::add_source(max_name, Metrics::MetricType::GAUGE, _count_metric, Metrics::Derived::Op::MAX); + } else if (has_aggregate) { + metrics.unlist(max_name); + } + + if (publish_group) { // 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. - Metrics::Derived::add_source(published_names[0], Metrics::MetricType::GAUGE, _count_metric, Metrics::Derived::Op::SUM); - Metrics::Derived::add_source(published_names[1], Metrics::MetricType::COUNTER, _count_total_metric, - Metrics::Derived::Op::SUM); - Metrics::Derived::add_source(published_names[2], Metrics::MetricType::COUNTER, _blocked_metric, Metrics::Derived::Op::SUM); + Metrics::Derived::add_source(group_names[0], Metrics::MetricType::GAUGE, _count_metric, Metrics::Derived::Op::SUM); + Metrics::Derived::add_source(group_names[1], Metrics::MetricType::COUNTER, _count_total_metric, Metrics::Derived::Op::SUM); + Metrics::Derived::add_source(group_names[2], Metrics::MetricType::COUNTER, _blocked_metric, Metrics::Derived::Op::SUM); } else { - // metric_aggregate is dynamic and overridable, so this group may well have published these - // names under an earlier value. A published name is never removed from the store, so without - // withdrawing them here they would report for the life of the process no matter what the - // setting says. The add_source calls above republish them if the setting changes back. - auto &metrics = Metrics::instance(); - - for (auto const &name : published_names) { + for (auto const &name : group_names) { metrics.unlist(name); } } diff --git a/src/iocore/net/unit_tests/test_ConnectionTracker.cc b/src/iocore/net/unit_tests/test_ConnectionTracker.cc index f26f0e484c0..e5f13a67723 100644 --- a/src/iocore/net/unit_tests/test_ConnectionTracker.cc +++ b/src/iocore/net/unit_tests/test_ConnectionTracker.cc @@ -148,7 +148,7 @@ TEST_CASE("ConnectionTracker aggregate metric publication", "[libinknet][Connect const std::string total_group = group_metric("total_connection", "10.9.8.7:443"); const std::string blocked_group = group_metric("blocked_connection", "10.9.8.7:443"); - SECTION("AGGREGATE_NONE publishes the per group metrics") + SECTION("AGGREGATE_NONE publishes the per group metrics and no aggregate") { txn.metric_aggregate = ConnectionTracker::AGGREGATE_NONE; open_and_close_connection(txn, addr); @@ -159,31 +159,99 @@ TEST_CASE("ConnectionTracker aggregate metric publication", "[libinknet][Connect CHECK_FALSE(is_published(host_metric("current_connection.max"))); } - SECTION("switching to AGGREGATE_ONLY retracts an already published per group metric") + SECTION("AGGREGATE_GROUP publishes the per group metrics, the sums and the max") { - // This is 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_GROUP; + open_and_close_connection(txn, addr); + + CHECK(is_published(current_group)); + CHECK(is_published(host_metric("current_connection"))); + CHECK(is_published(host_metric("total_connection"))); + CHECK(is_published(host_metric("blocked_connection"))); + CHECK(is_published(host_metric("current_connection.max"))); + } + + SECTION("AGGREGATE_MAX publishes the max and nothing else") + { + txn.metric_aggregate = ConnectionTracker::AGGREGATE_MAX; + open_and_close_connection(txn, addr); + + CHECK(is_published(host_metric("current_connection.max"))); + + CHECK_FALSE(is_published(host_metric("current_connection"))); + CHECK_FALSE(is_published(host_metric("total_connection"))); + CHECK_FALSE(is_published(host_metric("blocked_connection"))); + CHECK_FALSE(is_published(current_group)); + CHECK_FALSE(is_published(total_group)); + CHECK_FALSE(is_published(blocked_group)); + } + + SECTION("AGGREGATE_SUM publishes the sums and the max, but not the per group metrics") + { + txn.metric_aggregate = ConnectionTracker::AGGREGATE_SUM; + open_and_close_connection(txn, addr); + + CHECK(is_published(host_metric("current_connection"))); + CHECK(is_published(host_metric("total_connection"))); + CHECK(is_published(host_metric("blocked_connection"))); + CHECK(is_published(host_metric("current_connection.max"))); + + CHECK_FALSE(is_published(current_group)); + CHECK_FALSE(is_published(total_group)); + CHECK_FALSE(is_published(blocked_group)); + } + + 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_ONLY; + 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"))); + } + + SECTION("switching from AGGREGATE_SUM to AGGREGATE_MAX retracts the sums") + { + // The sums are aggregates rather than per group names, but they are published the same way and + // so need withdrawing the same way when the setting stops asking for them. + txn.metric_aggregate = ConnectionTracker::AGGREGATE_SUM; + open_and_close_connection(txn, addr); + REQUIRE(is_published(host_metric("current_connection"))); + + txn.metric_aggregate = ConnectionTracker::AGGREGATE_MAX; + open_and_close_connection(txn, addr); + + CHECK_FALSE(is_published(host_metric("current_connection"))); + CHECK_FALSE(is_published(host_metric("total_connection"))); + CHECK_FALSE(is_published(host_metric("blocked_connection"))); + CHECK(is_published(host_metric("current_connection.max"))); + } + + SECTION("switching from AGGREGATE_MAX to AGGREGATE_SUM republishes the sums") + { + txn.metric_aggregate = ConnectionTracker::AGGREGATE_MAX; + open_and_close_connection(txn, addr); + REQUIRE_FALSE(is_published(host_metric("total_connection"))); + + txn.metric_aggregate = ConnectionTracker::AGGREGATE_SUM; + open_and_close_connection(txn, addr); - // The aggregate stands in for them. CHECK(is_published(host_metric("current_connection"))); CHECK(is_published(host_metric("total_connection"))); CHECK(is_published(host_metric("blocked_connection"))); - CHECK(is_published(host_metric("current_connection.max"))); } - SECTION("switching back to AGGREGATE_GROUP republishes them") + SECTION("switching back to AGGREGATE_GROUP republishes the per group metrics") { - txn.metric_aggregate = ConnectionTracker::AGGREGATE_ONLY; + txn.metric_aggregate = ConnectionTracker::AGGREGATE_MAX; open_and_close_connection(txn, addr); REQUIRE_FALSE(is_published(current_group)); @@ -194,17 +262,19 @@ TEST_CASE("ConnectionTracker aggregate metric publication", "[libinknet][Connect CHECK(is_published(host_metric("current_connection"))); } - SECTION("AGGREGATE_ONLY still publishes a group that has no aggregate to stand in for it") + SECTION("a group with no aggregate keeps its own metrics whatever the setting") { // Only MATCH_BOTH yields a hostname to gather under, so a MATCH_PORT group has no aggregate. // Suppressing it would report nothing at all for that upstream. - txn.server_match = ConnectionTracker::MATCH_PORT; - txn.metric_aggregate = ConnectionTracker::AGGREGATE_ONLY; + txn.server_match = ConnectionTracker::MATCH_PORT; IpEndpoint port_addr; REQUIRE(ats_ip_pton("10.9.8.6:80", &port_addr) == 0); - open_and_close_connection(txn, port_addr); - CHECK(is_published("proxy.process.http.per_server.current_connection.10.9.8.6:80")); + for (auto level : {ConnectionTracker::AGGREGATE_MAX, ConnectionTracker::AGGREGATE_SUM}) { + txn.metric_aggregate = level; + open_and_close_connection(txn, port_addr); + CHECK(is_published("proxy.process.http.per_server.current_connection.10.9.8.6:80")); + } } } diff --git a/src/records/RecordsConfig.cc b/src/records/RecordsConfig.cc index 1e7384fa746..4e079d98371 100644 --- a/src/records/RecordsConfig.cc +++ b/src/records/RecordsConfig.cc @@ -406,7 +406,7 @@ static constexpr RecordElement RecordsConfig[] = , {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} + {RECT_CONFIG, "proxy.config.http.per_server.connection.metric_aggregate", RECD_INT, "0", RECU_DYNAMIC, RR_NULL, RECC_STR, "^[0-3]$", RECA_NULL} , {RECT_CONFIG, "proxy.config.http.per_server.connection.metric_prefix", RECD_STRING, "", RECU_DYNAMIC, RR_NULL, RECC_NULL, nullptr, RECA_NULL} , diff --git a/tests/gold_tests/origin_connection/per_server_connection_max.test.py b/tests/gold_tests/origin_connection/per_server_connection_max.test.py index 1b8f97ec8af..ea5fc51183e 100644 --- a/tests/gold_tests/origin_connection/per_server_connection_max.test.py +++ b/tests/gold_tests/origin_connection/per_server_connection_max.test.py @@ -156,11 +156,11 @@ def run(self) -> None: class ConnectMethodTest: """Test our max origin connection behavior with CONNECT traffic. - Also covers the two aggregate-publishing modes of + Also covers two of the aggregate-publishing modes of proxy.config.http.per_server.connection.metric_aggregate: - - 2 (AGGREGATE_ONLY): only the per hostname aggregate is published; the per group metrics - stay hidden and are visible only with --include-hidden. - - 1 (AGGREGATE_GROUP): the per hostname aggregate is published, and the per group metrics + - 3 (AGGREGATE_SUM): the per hostname sums and max are published; the per group metrics stay + hidden and are visible only with --include-hidden. + - 1 (AGGREGATE_GROUP): the per hostname sums and max are published, and the per group metrics are also mirrored into the published store. The match here defaults to 'both' and there is exactly one group for this hostname, so the @@ -171,7 +171,7 @@ class ConnectMethodTest: _process_counter: int = 0 _client_counter: int = 0 - def __init__(self, max_conn, metric_aggregate=2) -> None: + def __init__(self, max_conn, metric_aggregate=3) -> None: """Configure the server processes in preparation for the TestRun.""" self._metric_aggregate = metric_aggregate self._configure_dns() @@ -238,7 +238,7 @@ def _test_metrics(self, blocked) -> None: tr.Processes.Default.Streams.All += Testers.ContainsExpression( f'per_server.total_connection.{group_name} 5', 'The per group metric should be published at AGGREGATE_GROUP.') else: - # AGGREGATE_ONLY keeps the per group metrics hidden, so none of the three per group + # AGGREGATE_SUM keeps the per group metrics hidden, so none of the three per group # names may appear in a normal query. current_connection.max is not among them: it only # ever exists as a hostname aggregate, never per group. for counter in ('current_connection', 'total_connection', 'blocked_connection'): @@ -345,9 +345,9 @@ def _configure_trafficserver(self) -> None: 'proxy.config.diags.debug.enabled': 1, 'proxy.config.diags.debug.tags': 'http|dns|hostdb|conn_track', 'proxy.config.http.per_server.connection.metric_enabled': 1, - # Aggregates only: the per group metrics stay hidden, which is what this test is - # about reading through the aggregate. - 'proxy.config.http.per_server.connection.metric_aggregate': 2, + # Sums and max, per group metrics hidden: this test is about reading the group + # behavior through the hostname aggregate. + 'proxy.config.http.per_server.connection.metric_aggregate': 3, 'proxy.config.http.per_server.connection.match': 'both', }) self._ts.Disk.remap_config.AddLines( @@ -394,8 +394,8 @@ def _test_metrics_while_held(self) -> None: # which cannot catch a metric that should not be there at all. tr.Processes.Default.Streams.All += Testers.ExcludesExpression( r'per_server\.\w+_connection\.multi\.origin\.com\.\d', - 'At metric_aggregate 2 the per group metrics must stay hidden, leaving only the ' - 'hostname aggregate published.') + 'At metric_aggregate 3 the per group metrics must stay hidden, leaving only the ' + 'hostname aggregates published.') def _test_metrics_after_drain(self) -> None: """After traffic drains and a further sync tick passes, both live gauges must read 0. @@ -525,15 +525,15 @@ def run(self) -> None: class AggregateOnlyWithoutHostAggregateTest: """Verify metric_aggregate 2 still publishes per group metrics when there is no aggregate. - metric_aggregate 2 (AGGREGATE_ONLY) normally leaves the per group metrics hidden and publishes - only the per hostname aggregate. That aggregate exists only under match 'both', which is the - only match type with more than one group per hostname (Group::host_metric_name returns empty - for the others). With match 'port' there is therefore nothing for the aggregate to stand in - for, so the per group metrics have to be published regardless, or level 2 would report nothing - at all for this group. + metric_aggregate 2 (AGGREGATE_MAX) normally leaves the per group metrics hidden and publishes + only the per hostname max. That aggregate exists only under match 'both', which is the only + match type with more than one group per hostname (Group::host_metric_name returns empty for + the others). With match 'port' there is therefore nothing for the aggregate to stand in for, + so the per group metrics have to be published regardless, or level 2 would report nothing at + all for this group. - Every other test in this file that sets metric_aggregate 2 uses match 'both', so without this - case a regression that dropped the fallback would leave the suite green. + Every other test in this file that suppresses the per group metrics uses match 'both', so + without this case a regression that dropped the fallback would leave the suite green. """ def __init__(self) -> None: @@ -595,6 +595,9 @@ def run(self) -> None: class AggregateRetractionTest: """Verify that raising metric_aggregate to 2 withdraws already published per group metrics. + metric_aggregate 2 (AGGREGATE_MAX) publishes the per hostname max and nothing else, so this + also covers that the hostname sums are not published at that level. + metric_aggregate is dynamic, but the publication decision is made in the ConnectionTracker Group constructor, and a published metric name is never removed from the metric store. Before the store grew a tombstone, a name published while the setting was 0 kept reporting for the @@ -694,12 +697,18 @@ def run(self) -> None: 'no longer be published, even though they were published earlier in this process.') tr.Processes.Default.Streams.All += Testers.ContainsExpression( r'per_server\.current_connection\.max\.retract\.origin\.com', - 'The hostname aggregate stands in for the withdrawn per group metrics.') + 'The hostname max stands in for the withdrawn per group metrics.') + # metric_aggregate 2 is the max and nothing else, so the hostname sums must not appear + # either. '\.com ' with the trailing space matches the aggregate names, whose value follows + # the hostname directly; the max is 'current_connection.max.' and does not match. + tr.Processes.Default.Streams.All += Testers.ExcludesExpression( + r'per_server\.\w+_connection\.retract\.origin\.com ', + 'At metric_aggregate 2 only the max is published: the hostname sums must be absent.') tr.StillRunningAfter = self._ts PerServerConnectionMaxTest().run() -ConnectMethodTest(3, metric_aggregate=2).run(blocked=2, gold_file="gold/two_503_congested.gold") +ConnectMethodTest(3, metric_aggregate=3).run(blocked=2, gold_file="gold/two_503_congested.gold") ConnectMethodTest(0, metric_aggregate=1).run(blocked=0, gold_file="gold/two_200_ok.gold") MultiGroupAggregateTest().run() MetricOverrideTest().run() From 38c5ea2563a59a0bf821c0da783c82751f1efe7a Mon Sep 17 00:00:00 2001 From: Chris McFarlen Date: Wed, 16 Sep 2026 12:52:15 -0700 Subject: [PATCH 04/11] Include for std::forward for_each forwards its callable but the header got only through another include. It compiles today; that is not a property this header controls. --- include/tsutil/Metrics.h | 1 + 1 file changed, 1 insertion(+) diff --git a/include/tsutil/Metrics.h b/include/tsutil/Metrics.h index 344b3cd892b..a0b34753c86 100644 --- a/include/tsutil/Metrics.h +++ b/include/tsutil/Metrics.h @@ -33,6 +33,7 @@ #include #include #include +#include #include #include From d878d93c31e5bf41fcacc8c42d76b74e4eeaf409 Mon Sep 17 00:00:00 2001 From: Chris McFarlen Date: Wed, 16 Sep 2026 12:52:27 -0700 Subject: [PATCH 05/11] Note the Metrics API removals for v11 Metrics.h is installed, so dropping the iterator, find(), createSpan() and rename() breaks downstream plugins even though nothing in tree used them. Record the removals and the for_each replacement where upgraders will look. --- doc/release-notes/upgrading.en.rst | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/doc/release-notes/upgrading.en.rst b/doc/release-notes/upgrading.en.rst index fd9c145f7e7..bec15027abc 100644 --- a/doc/release-notes/upgrading.en.rst +++ b/doc/release-notes/upgrading.en.rst @@ -30,6 +30,25 @@ with :cpp:func:`TSPortDescriptorDestroy`. The descriptor can be destroyed immediately after :cpp:func:`TSPortDescriptorAccept` returns because the listener does not retain it. +``ts::Metrics``, in the installed ``tsutil/Metrics.h``, no longer has an +iterator. ``Metrics::iterator``, ``begin()``, ``end()`` and ``find()`` are +removed, and enumeration is now ``Metrics::for_each(func)``, which invokes +``func(name, type, value)`` for each metric: + +.. code-block:: cpp + + ts::Metrics::instance().for_each([](std::string_view name, ts::Metrics::MetricType type, int64_t value) { + // ... + }); + +Handing out a position let a caller name a slot the store was free to change +underneath them, which is what the iterator could not be made safe against. +Reaching a single metric by name is ``lookup()``. + +``Metrics::Storage::createSpan()`` and ``Metrics::rename()`` are also removed. +Spans handed out unnamed slots that only ``rename()`` could name, and +``rename()`` mutated a name that the lock free readers hand out views of. + Upgrading to ATS v10.x ====================== From 3854c1989aa9301d6e89fbe2ee376243f536b87b Mon Sep 17 00:00:00 2001 From: Chris McFarlen Date: Wed, 16 Sep 2026 12:53:59 -0700 Subject: [PATCH 06/11] Format test_ConnectionTracker.cc clang-format only, from merging this file with the one master added. --- src/iocore/net/unit_tests/test_ConnectionTracker.cc | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/iocore/net/unit_tests/test_ConnectionTracker.cc b/src/iocore/net/unit_tests/test_ConnectionTracker.cc index e5f13a67723..0b1f8f12ff4 100644 --- a/src/iocore/net/unit_tests/test_ConnectionTracker.cc +++ b/src/iocore/net/unit_tests/test_ConnectionTracker.cc @@ -79,9 +79,8 @@ is_published(std::string_view metric_name) { bool found = false; - ts::Metrics::instance().for_each([&](std::string_view name, ts::Metrics::MetricType, int64_t) { - found |= (name == metric_name); - }); + ts::Metrics::instance().for_each( + [&](std::string_view name, ts::Metrics::MetricType, int64_t) { found |= (name == metric_name); }); return found; } From 2c37e2a5aa2f55fba441e2d096c404dc59c6b1d4 Mon Sep 17 00:00:00 2001 From: Chris McFarlen Date: Wed, 16 Sep 2026 17:46:37 -0700 Subject: [PATCH 07/11] Drop the duplicate test source entry The rebase added test_ConnectionTracker.cc to test_net again; master already listed it when it added its own tests to that file. --- src/iocore/net/CMakeLists.txt | 1 - 1 file changed, 1 deletion(-) diff --git a/src/iocore/net/CMakeLists.txt b/src/iocore/net/CMakeLists.txt index c84ed8becfd..5de6afc44dc 100644 --- a/src/iocore/net/CMakeLists.txt +++ b/src/iocore/net/CMakeLists.txt @@ -146,7 +146,6 @@ if(BUILD_TESTING) NetVCTest.cc unit_tests/test_ConnectionTracker.cc unit_tests/test_NetHandler.cc - unit_tests/test_ConnectionTracker.cc unit_tests/test_ProxyProtocol.cc unit_tests/test_SSLCertLookup.cc unit_tests/test_SSLNetVConnectionAsyncEp.cc From 366d7f0d6f8a3070dc3c3f32d48a5839bda487cd Mon Sep 17 00:00:00 2001 From: Chris McFarlen Date: Wed, 16 Sep 2026 17:46:38 -0700 Subject: [PATCH 08/11] Name the mode under test in the assertion message It said AGGREGATE_ONLY, which no longer exists, so a failure pointed at the wrong configuration. Interpolate the configured value instead. --- .../origin_connection/per_server_connection_max.test.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/tests/gold_tests/origin_connection/per_server_connection_max.test.py b/tests/gold_tests/origin_connection/per_server_connection_max.test.py index ea5fc51183e..1724a23b40e 100644 --- a/tests/gold_tests/origin_connection/per_server_connection_max.test.py +++ b/tests/gold_tests/origin_connection/per_server_connection_max.test.py @@ -243,7 +243,8 @@ def _test_metrics(self, blocked) -> None: # ever exists as a hostname aggregate, never per group. for counter in ('current_connection', 'total_connection', 'blocked_connection'): tr.Processes.Default.Streams.All += Testers.ExcludesExpression( - f'per_server.{counter}.{group_name} ', f'per_server.{counter}.{group_name} must stay hidden at AGGREGATE_ONLY.') + f'per_server.{counter}.{group_name} ', + f'per_server.{counter}.{group_name} must stay hidden at metric_aggregate {self._metric_aggregate}.') # The per group metrics must be visible with --include-hidden at either level. This is also # the end to end test for that traffic_ctl option. From 61d9751174e7df299a0112c99cf2b33e11f0b184 Mon Sep 17 00:00:00 2001 From: Chris McFarlen Date: Wed, 16 Sep 2026 17:50:30 -0700 Subject: [PATCH 09/11] Add Metrics::Derived::remove_source A derived metric is shared by its sources, so a contributor that goes away cannot unlist it: another source may still be publishing through that name. remove_source drops one source and unlists the name only when the last one goes, and re-adding relists it. --- include/tsutil/Metrics.h | 12 +++++ src/tsutil/Metrics.cc | 43 ++++++++++++++++ src/tsutil/unit_tests/test_Metrics.cc | 73 +++++++++++++++++++++++++++ 3 files changed, 128 insertions(+) diff --git a/include/tsutil/Metrics.h b/include/tsutil/Metrics.h index a0b34753c86..17ccb822dce 100644 --- a/include/tsutil/Metrics.h +++ b/include/tsutil/Metrics.h @@ -719,6 +719,18 @@ class Metrics */ static void add_source(std::string_view derived_name, Metrics::MetricType type, Metrics::AtomicType *source, Op op = Op::SUM); + /** Stop @a source contributing to a derived metric. + * + * The counterpart to @c add_source, for a contributor that goes away or stops wanting the + * aggregate published. A derived metric is shared by its sources, so this does not unlist it + * while any remain; when the last one is removed there is nothing left to report and the name + * is unlisted. Re-adding a source relists it. + * + * A source that is not registered for @a derived_name, or a name with no derived metric, is a + * no-op. + */ + static void remove_source(std::string_view derived_name, Metrics::AtomicType *source); + /** * Update derived metrics. * diff --git a/src/tsutil/Metrics.cc b/src/tsutil/Metrics.cc index 83c4c8d19a3..447c0039890 100644 --- a/src/tsutil/Metrics.cc +++ b/src/tsutil/Metrics.cc @@ -300,6 +300,34 @@ namespace details } } + /// @return @c true if @a id has no sources left, so the caller can unlist it. + bool + remove_source(Metrics::IdType id, Metrics::AtomicType *source) + { + if (!source) { + return false; + } + + std::lock_guard l(metrics_lock); + auto it = std::find_if(metrics.begin(), metrics.end(), [id](DerivedMetric const &m) { return m.metric == id; }); + + if (it == metrics.end()) { + return false; + } + + auto src = std::find(it->derived_from.begin(), it->derived_from.end(), source); + + if (src == it->derived_from.end()) { + return false; // Not a source of this metric, so nothing about it changes. + } + + it->derived_from.erase(src); + + // The entry stays, holding no sources: update() skips those, and add_source finds it again if + // a contributor comes back. + return it->derived_from.empty(); + } + static DerivativeMetrics & instance() { @@ -358,6 +386,21 @@ Metrics::Derived::add_source(std::string_view derived_name, Metrics::MetricType details::DerivativeMetrics::instance().add_source(id, source, op); } +void +Metrics::Derived::remove_source(std::string_view derived_name, Metrics::AtomicType *source) +{ + auto &instance = Metrics::instance(); + auto id = instance.lookup(derived_name); + + if (id == Metrics::NOT_FOUND) { + return; + } + + if (details::DerivativeMetrics::instance().remove_source(id, source)) { + instance.unlist(id); + } +} + Metrics::StaticString & Metrics::StaticString::instance() { diff --git a/src/tsutil/unit_tests/test_Metrics.cc b/src/tsutil/unit_tests/test_Metrics.cc index ff737227ce6..3b0d4fe0b5d 100644 --- a/src/tsutil/unit_tests/test_Metrics.cc +++ b/src/tsutil/unit_tests/test_Metrics.cc @@ -299,6 +299,79 @@ TEST_CASE("Metrics derived ops", "[libtsapi][Metrics]") } } +TEST_CASE("Metrics derived remove_source", "[libtsapi][Metrics]") +{ + auto &m = Metrics::instance(); + + SECTION("a derived metric stays listed while any source remains") + { + auto a = Metrics::Gauge::createHiddenPtr("rm.a"); + auto b = Metrics::Gauge::createHiddenPtr("rm.b"); + + Metrics::Derived::add_source("rm.sum", Metrics::MetricType::GAUGE, a); + Metrics::Derived::add_source("rm.sum", Metrics::MetricType::GAUGE, b); + + Metrics::Gauge::store(a, 3); + Metrics::Gauge::store(b, 4); + Metrics::Derived::update_derived(); + + auto const id = m.lookup("rm.sum"); + REQUIRE(id != Metrics::NOT_FOUND); + REQUIRE(m[id].load() == 7); + REQUIRE(m.listed(id)); + + // One contributor drops out. The metric is still someone else's, so it stays listed and now + // reports only what is left. + Metrics::Derived::remove_source("rm.sum", b); + Metrics::Derived::update_derived(); + + CHECK(m.listed(id)); + CHECK(m[id].load() == 3); + + // The last one drops out, so nothing is contributing and the name goes out of the listing. + Metrics::Derived::remove_source("rm.sum", a); + + CHECK_FALSE(m.listed(id)); + } + + SECTION("re-adding a source relists it") + { + auto a = Metrics::Gauge::createHiddenPtr("rm.relist.a"); + + Metrics::Derived::add_source("rm.relist", Metrics::MetricType::GAUGE, a); + + auto const id = m.lookup("rm.relist"); + REQUIRE(id != Metrics::NOT_FOUND); + + Metrics::Derived::remove_source("rm.relist", a); + REQUIRE_FALSE(m.listed(id)); + + Metrics::Gauge::store(a, 11); + Metrics::Derived::add_source("rm.relist", Metrics::MetricType::GAUGE, a); + Metrics::Derived::update_derived(); + + CHECK(m.listed(id)); + CHECK(m[id].load() == 11); + } + + SECTION("removing an unknown source or name is harmless") + { + auto a = Metrics::Gauge::createHiddenPtr("rm.unknown.a"); + + Metrics::Derived::remove_source("rm.no.such.derived", a); + + Metrics::Derived::add_source("rm.unknown", Metrics::MetricType::GAUGE, a); + + auto const id = m.lookup("rm.unknown"); + auto other = Metrics::Gauge::createHiddenPtr("rm.unknown.other"); + + // Not a source of this metric, so it must not empty the list or unlist anything. + Metrics::Derived::remove_source("rm.unknown", other); + + CHECK(m.listed(id)); + } +} + TEST_CASE("Metrics derived add_source", "[libtsapi][Metrics]") { auto &m = Metrics::instance(); From 2af1eee7a154614519c444e60ee2435075c98c81 Mon Sep 17 00:00:00 2001 From: Chris McFarlen Date: Wed, 16 Sep 2026 17:50:30 -0700 Subject: [PATCH 10/11] Stop contributing to shared aggregate names instead of unlisting them The per hostname sums and max are named per hostname, so every group of that hostname shares them. A group built for a mode that does not publish them was unlisting names another group was still publishing: two mappings to one hostname with different metric_aggregate values, which is what overriding it is for, and the second group hid the first one's aggregate. Worse with mixed match types, where MATCH_HOST's own metric carries the same name as the MATCH_BOTH aggregate. Groups now remove their source instead. The per group names go the same way, though they have a single source, so the derived pass stops recomputing a value into a name that is no longer published. --- src/iocore/net/ConnectionTracker.cc | 21 +++++++++++-------- .../net/unit_tests/test_ConnectionTracker.cc | 19 +++++++++++++++++ 2 files changed, 31 insertions(+), 9 deletions(-) diff --git a/src/iocore/net/ConnectionTracker.cc b/src/iocore/net/ConnectionTracker.cc index fad0380f10d..49967a50717 100644 --- a/src/iocore/net/ConnectionTracker.cc +++ b/src/iocore/net/ConnectionTracker.cc @@ -511,8 +511,6 @@ ConnectionTracker::Group::Group(DirectionType direction, Key const &key, std::st }; std::string const max_name = "proxy.process.http.per_server.current_connection.max." + _host_metric_name; - auto &metrics = Metrics::instance(); - // metric_aggregate is dynamic and overridable, so this group may well have published a name // under an earlier value. A published name is never removed from the store, so without // withdrawing it here it would report for the life of the process no matter what the setting @@ -522,9 +520,11 @@ ConnectionTracker::Group::Group(DirectionType direction, Key const &key, std::st Metrics::Derived::add_source(sum_names[1], Metrics::MetricType::COUNTER, _count_total_metric, Metrics::Derived::Op::SUM); Metrics::Derived::add_source(sum_names[2], Metrics::MetricType::COUNTER, _blocked_metric, Metrics::Derived::Op::SUM); } else if (has_aggregate) { - for (auto const &name : sum_names) { - metrics.unlist(name); - } + // 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); } if (publish_max) { @@ -533,7 +533,7 @@ ConnectionTracker::Group::Group(DirectionType direction, Key const &key, std::st // and a maximum over time can be computed by whatever scrapes it. Metrics::Derived::add_source(max_name, Metrics::MetricType::GAUGE, _count_metric, Metrics::Derived::Op::MAX); } else if (has_aggregate) { - metrics.unlist(max_name); + Metrics::Derived::remove_source(max_name, _count_metric); } if (publish_group) { @@ -544,9 +544,12 @@ ConnectionTracker::Group::Group(DirectionType direction, Key const &key, std::st Metrics::Derived::add_source(group_names[1], Metrics::MetricType::COUNTER, _count_total_metric, Metrics::Derived::Op::SUM); Metrics::Derived::add_source(group_names[2], Metrics::MetricType::COUNTER, _blocked_metric, Metrics::Derived::Op::SUM); } else { - for (auto const &name : group_names) { - metrics.unlist(name); - } + // Same mechanism as the aggregates above, though these names have only this group as a + // source. It leaves nothing behind for the derived pass to keep recomputing into a name that + // is no longer published. + Metrics::Derived::remove_source(group_names[0], _count_metric); + Metrics::Derived::remove_source(group_names[1], _count_total_metric); + Metrics::Derived::remove_source(group_names[2], _blocked_metric); } if (dbg_ctl.on()) { diff --git a/src/iocore/net/unit_tests/test_ConnectionTracker.cc b/src/iocore/net/unit_tests/test_ConnectionTracker.cc index 0b1f8f12ff4..d2666a4908f 100644 --- a/src/iocore/net/unit_tests/test_ConnectionTracker.cc +++ b/src/iocore/net/unit_tests/test_ConnectionTracker.cc @@ -261,6 +261,25 @@ TEST_CASE("ConnectionTracker aggregate metric publication", "[libinknet][Connect CHECK(is_published(host_metric("current_connection"))); } + SECTION("one hostname's groups do not unlist each other's aggregate") + { + // metric_aggregate is overridable, so two mappings to one hostname can disagree. Both groups + // share the hostname's aggregate names, so a group that does not want them must stop + // contributing rather than unlist a name the other one is still publishing. + IpEndpoint other; + REQUIRE(ats_ip_pton("10.9.8.5:443", &other) == 0); + + txn.metric_aggregate = ConnectionTracker::AGGREGATE_SUM; + open_and_close_connection(txn, addr); + REQUIRE(is_published(host_metric("current_connection"))); + + txn.metric_aggregate = ConnectionTracker::AGGREGATE_MAX; + open_and_close_connection(txn, other); + + CHECK(is_published(host_metric("current_connection"))); + CHECK(is_published(host_metric("current_connection.max"))); + } + SECTION("a group with no aggregate keeps its own metrics whatever the setting") { // Only MATCH_BOTH yields a hostname to gather under, so a MATCH_PORT group has no aggregate. From 2ec417eee9d70c7104f6d9245b880dbac8eb09f4 Mon Sep 17 00:00:00 2001 From: Chris McFarlen Date: Wed, 16 Sep 2026 17:51:31 -0700 Subject: [PATCH 11/11] Document that shared aggregate names are not retracted by one group The per group and per hostname names now behave differently, and the earlier text said the last group rebuilt decides whether the sums are published, which was describing the defect. --- doc/admin-guide/files/records.yaml.en.rst | 7 +++++-- .../internal-libraries/Metrics.en.rst | 12 ++++++++++++ include/iocore/net/ConnectionTracker.h | 11 +++++++---- 3 files changed, 24 insertions(+), 6 deletions(-) diff --git a/doc/admin-guide/files/records.yaml.en.rst b/doc/admin-guide/files/records.yaml.en.rst index b05e6c85697..a24100bbc17 100644 --- a/doc/admin-guide/files/records.yaml.en.rst +++ b/doc/admin-guide/files/records.yaml.en.rst @@ -2093,8 +2093,11 @@ Origin Server Connect Attempts consumers; they are not destroyed, and moving back republishes them with their accumulated values intact. - Because the sums are named per hostname rather than per group, where the mappings for one - hostname disagree about this setting the last group rebuilt decides whether they are published. + 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 + cannot hide each other's aggregate. .. ts:cv:: CONFIG proxy.config.http.per_server.connection.metric_prefix STRING NULL :reloadable: diff --git a/doc/developer-guide/internal-libraries/Metrics.en.rst b/doc/developer-guide/internal-libraries/Metrics.en.rst index ba68d1a9df1..4f9c86b0dae 100644 --- a/doc/developer-guide/internal-libraries/Metrics.en.rst +++ b/doc/developer-guide/internal-libraries/Metrics.en.rst @@ -171,6 +171,18 @@ Adding a source that is already registered for that derived metric is a no-op, s re-register the same source, such as one recreating an object for the same key, need not track that itself. The ``type`` and ``op`` arguments are ignored if the derived metric already exists. +``ts::Metrics::Derived::remove_source()`` is the counterpart, for a contributor that goes away or +stops wanting the aggregate published: + +.. code-block:: cpp + + ts::Metrics::Derived::remove_source("proxy.process.example.total", per_thing_metric); + +A derived metric is shared by its sources, so this does not unlist it while any remain. Removing the +last source leaves nothing to report, so the name is unlisted; adding a source again relists it. +Removing a source that is not registered, or naming a derived metric that does not exist, is a +no-op. + A hidden source can feed a published aggregate: .. code-block:: cpp diff --git a/include/iocore/net/ConnectionTracker.h b/include/iocore/net/ConnectionTracker.h index 50bc75daa64..cc574d9846d 100644 --- a/include/iocore/net/ConnectionTracker.h +++ b/include/iocore/net/ConnectionTracker.h @@ -110,10 +110,13 @@ class ConnectionTracker * * 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. Retracting a published name relies on the metric store's - * listing, see @c ts::Metrics::unlist. The sums are named per hostname rather than per group, so - * where the mappings for one hostname disagree about this setting, the last group constructed - * decides whether they are published. + * in effect when it was created. + * + * The per group names belong to one group, so a group that stops publishing them retracts them, + * see @c ts::Metrics::unlist. The sums and the max are named per hostname and shared by its + * groups, so a group that stops publishing those only stops contributing to them, see + * @c ts::Metrics::Derived::remove_source; they are retracted once no group of that hostname + * publishes them. * * The records layer validates and clamps this to 0..3. A plugin setting the overridable config * directly is not clamped, see @c METRIC_AGGREGATE_CONV; any other value behaves as