Copilot commented on code in PR #13666:
URL: https://github.com/apache/trafficserver/pull/13666#discussion_r4040956416
##########
tests/gold_tests/origin_connection/per_server_connection_max.test.py:
##########
@@ -580,9 +593,125 @@ def run(self) -> None:
self._test_metrics()
+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
+ 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
Review Comment:
The origin server is started in the first test run, but this helper only
retains `self._ts`; `self._curl()` is invoked again at line 688 after several
intermediate runs that also retain only ATS. AuTest may stop a process at the
end of a run unless it is in `StillRunningAfter`, so the second curl can fail
before exercising metric retraction. Keep `self._server` alive in
`StillRunningAfter` for every run between the two curls (or start it again
before the second curl).
--
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]
