bneradt commented on code in PR #13666:
URL: https://github.com/apache/trafficserver/pull/13666#discussion_r4030659785
##########
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:
[P2] Preserve metrics still published by a MATCH_HOST group
Confirmed by tracing the constructor and name generation. This does not
require conflicting aggregate settings: keep a MATCH_HOST connection for
`example.test` alive with metric_enabled=1 and metric_aggregate=0, then
construct a MATCH_BOTH group for the same hostname with those same metric
settings. The first group publishes `current_connection.example.test`,
`total_connection.example.test`, and `blocked_connection.example.test`; the
second reaches this branch and unlists all three. No hostname aggregate ever
needed retracting in this sequence. Subsequent connections reusing the live
MATCH_HOST group do not run its constructor, so its metrics remain absent from
enumeration despite continuing to update.
Please distinguish aggregate publication from the MATCH_HOST group's
publication requirement before withdrawing a shared name. Add coverage that
keeps the MATCH_HOST group reserved while constructing a MATCH_BOTH group at
mode 0 (and mode 2), then verifies all three MATCH_HOST metrics remain listed.
The current fallback coverage only tests MATCH_PORT, which cannot expose this
name collision.
--
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]
