Copilot commented on code in PR #13666:
URL: https://github.com/apache/trafficserver/pull/13666#discussion_r4030340767
##########
src/iocore/net/ConnectionTracker.cc:
##########
@@ -485,33 +486,67 @@ 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);
+ // 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;
+ }
+
+ // 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<std::string, 3> 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<std::string, 3> 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);
+ }
Review Comment:
These names are not guaranteed to be aggregate-only: a `MATCH_HOST` group
publishes its own metrics under the same `<stem>.<fqdn>` names, and the header
documents that mixed `MATCH_HOST`/`MATCH_BOTH` groups merge those names.
Building a `MATCH_BOTH` group with mode 0 or 2 therefore unlists the live
`MATCH_HOST` metrics here, even though there may be no aggregate to retract;
avoid unlisting a name still owned by that group.
##########
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
Review Comment:
This source is already listed at line 147, so adding it again duplicates the
same translation unit in `test_net`. Remove the second entry; otherwise
generators/linkers may carry a duplicate object reference, and the target
source list is redundant even where it is deduplicated.
##########
tests/gold_tests/origin_connection/per_server_connection_max.test.py:
##########
@@ -233,8 +238,8 @@ 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
- # names may appear in a normal query. current_connection_max is
not among them: it only
+ # 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'):
tr.Processes.Default.Streams.All += Testers.ExcludesExpression(
Review Comment:
This branch now covers metric_aggregate 3, but the assertion message on line
246 still says `AGGREGATE_ONLY`, which was removed and misidentifies the
configuration under test. Update the message to `AGGREGATE_SUM` (or use the
configured value) so a failing autest points to the current mode.
--
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]
