cmcfarlen opened a new pull request, #13666: URL: https://github.com/apache/trafficserver/pull/13666
Stacked on #13616, which adds `ts::Metrics::unlist`. Until that merges this branch carries its commit too, so review the last three commits here. Draft for that reason, not because the work is unfinished. ## 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. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
