bneradt commented on code in PR #13666:
URL: https://github.com/apache/trafficserver/pull/13666#discussion_r4041532675
##########
src/iocore/net/ConnectionTracker.cc:
##########
@@ -485,33 +486,70 @@ 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;
Review Comment:
Reconsidering severity after discussion: the collision is real, but requires
mixed MATCH_HOST/MATCH_BOTH configurations and the related hostnames
example.test and max.example.test. I have no evidence that this is common, and
calling it a merge blocker overstated its practical impact. Treating this as a
non-blocking naming follow-up; it does not prevent my approval.
##########
src/tsutil/Metrics.cc:
##########
@@ -358,6 +386,21 @@ Metrics::Derived::add_source(std::string_view
derived_name, Metrics::MetricType
details::DerivativeMetrics::instance().add_source(id, source, op);
}
+void
+Metrics::Derived::remove_source(std::string_view derived_name,
Metrics::AtomicType *source)
+{
+ auto &instance = Metrics::instance();
+ auto id = instance.lookup(derived_name);
+
+ if (id == Metrics::NOT_FOUND) {
+ return;
+ }
+
+ if (details::DerivativeMetrics::instance().remove_source(id, source)) {
+ instance.unlist(id);
Review Comment:
Addressed in c525ced3e49b4c56f2d2dbb39035f2fc754c718c: remove_source now
unlists under metrics_lock, and add_source relists under that same lock after
registering its source. This covers both interleavings raised above. The new
concurrent test checks the resulting source/listing invariant after each racing
pair.
--
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]
