cmcfarlen commented on PR #13506: URL: https://github.com/apache/trafficserver/pull/13506#issuecomment-5413723690
Thanks Bryan — this is a genuinely useful review, and the two corrections to our own claims are the valuable part. Pushed `4e57a3e183` and `8038a05aa9`. ## Applied **1. Reload directionality.** Took your wording on `metric_enabled` and spelled the caveat out on `metric_aggregate` rather than leaving it as a back-reference, since as you say `2` is the value someone reaches for specifically to cut cardinality and it's the one direction a reload can't deliver. Kept `:reloadable:`. **2. The wrong premise.** You're right, and I verified it: `Group::equal` keys `MATCH_IP` on the address alone, so one hostname with several A records already yields several groups. Worth flagging that the claim had spread to **four** places, not one — `http-connection.en.rst:220`, `records.yaml.en.rst:2051`, and both `ConnectionTracker.h:211` and `:491` — so I fixed all of them with your reasoning: `ip`/`port` keys carry no hostname and are shared by every hostname resolving to that address. Good catch on "someone will eventually relax the restriction on the strength of a premise that is false"; that's exactly the failure mode. **3. Subset aggregates and the cross-match-type collision.** Both documented, on the monitoring page and on `host_metric_name`. Confirmed `server_match` is overridable at `OverridableConfigDefs.h:229`, so the `MATCH_HOST`/`MATCH_BOTH` collision is reachable by configuration, and `add_source` does keep the first caller's op. Agreed that including the match type in the aggregate name, and deciding membership per hostname, are the real fixes and belong with the follow-up. **4. `per_server_metric_enabled.test.py`.** Fixed — it now sets `raw_stat_sync_interval_ms` the same way the other file does. Your analysis was right: a 6 second wait against a 5000ms default is at least one second of margin and never more. **7 (partial).** Corrected the "single source SUM is an identity" comment. You're right that it contradicts the design it sits in: a single-source SUM combines nothing, but the published value is still a sample, and it reads 0 from creation until the first tick. ## One correction back The unanchored-pattern note isn't right as stated — though I suspect it's a holdover from reviewing the first iteration of this work, which did define `metric_enabled` as a three-valued `0/1/2` level before we split it into two settings for backward compatibility. Against the pattern that's actually on master, a `records.yaml` carrying `metric_enabled: 2` did **not** load: the pre-PR pattern was unanchored `[0-1]`, and `recordRegexCheck` searching `"2"` for `[0-1]` finds no match, so validation failed then too. What actually regresses is multi-character values containing a `0` or `1` — `10`, `01`, `12` — which used to match on a substring and now don't. Real incompatibility, different trigger, and vanishingly unlikely in a real config, so I'd rather not put a note in the PR body implying `2` used to be accepted. Happy to add one about the substring case if you think it earns the space. ## Deferred, with reasons - **5, upgrading notes.** Skipping this one here. `upgrading.en.rst` is still headed "Upgrading to ATS v10.x" and this change is on 11.x-dev, so starting that section is a bigger editorial call than this PR should make. The sampling-latency note lands with the follow-up, which also has to document the retirement behaviour, and the monitoring page already covers the mechanism in the meantime. - **6, `metric_prefix` global + unsynchronised write.** Agreed on both halves. The race is pre-existing and the fix wants to be the same change that makes registration lifetime-aware, so it goes with the follow-up. - **7, remaining items.** The diagnostics on the converters and reload callbacks and the more informative `Registered per_server_connection` debug line are worth doing — your point that "why can I not see my metric" will be the common question is well taken, given how many ways this feature can decline to publish. Dropping the `Group` constructor defaults and taking `bool metric_enabled` is a good call too; the adjacent-`int` transposition hazard is real. All code changes, none of them blocking, and I'd rather not reopen the platform builds on this PR for them. - **`MultiGroupAggregateTest` hold time.** Fair, and widening it costs nothing at a 500ms interval. Also right that the comment's reasoning is wrong — go-httpbin returns 400 above its cap and `--fail` makes that a hard failure, not a clamp. Folding both into the follow-up unless you'd rather see it here. Following up separately on the `MatchType` `static_cast` UB you spotted — you're right that it's a real find independent of this change, and it deserves its own issue rather than being buried in this thread. -- 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]
