bneradt commented on code in PR #13666:
URL: https://github.com/apache/trafficserver/pull/13666#discussion_r4039843745
##########
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:
[P2] Confirmed from the name builders and Derived::add_source: with an empty
metric_prefix, MATCH_HOST for max.example.test and MATCH_BOTH for example.test
both register
proxy.process.http.per_server.current_connection.max.example.test. If the
MATCH_HOST source (value 7) registers first, the entry retains SUM; adding two
MATCH_BOTH sources with values 2 and 3 produces 12 instead of the expected
hostname max of 3. In the reverse construction order it retains MAX and the
unrelated host source can inflate that max to 7. Please make the max aggregate
namespace distinct from valid per-group hostname names and cover both
construction orders. This remains a correctness blocker.
##########
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:
[P2] Confirmed the interleaving in the new public Derived API: A removes the
last source and returns true from the helper, releasing metrics_lock; B
completes add_source for the same name (including relisting it and registering
its source); A then calls unlist. The resulting metric has an active source and
keeps being recomputed, but is absent from enumeration. Moving only unlist
inside the lock is insufficient because add_source currently relists before
acquiring that lock: both source membership and the listing transition need
coordinated ordering. Please add a concurrent regression test.
ConnectionTracker constructors are serialized by the outbound table lock, so
that particular caller is protected; the new public Metrics API itself is not.
--
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]
