Copilot commented on code in PR #13666:
URL: https://github.com/apache/trafficserver/pull/13666#discussion_r4032123534
##########
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:
At line 399, the source-list mutation is protected by `metrics_lock`, but
`instance.unlist(id)` runs after that lock is released. `add_source()` relists
an existing name in `_create()` before taking the same lock, so another thread
can add a source between these operations and then have the metric unlisted
despite still having a contributor. Keep the source-list mutation and
relist/unlist transition coordinated so re-adding a source cannot leave it
hidden.
##########
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:
`current_connection.max.<host>` can collide with a valid MATCH_HOST
per-group name. A MATCH_HOST mapping for `max.<host>` publishes
`current_connection.max.<host>`, exactly the name used here for a MATCH_BOTH
`<host>` aggregate. Because `Derived::add_source` keeps the first operation for
an existing name, construction order makes this metric either a SUM or MAX, so
one of the two reported values is wrong. Please choose a collision-free/escaped
name or otherwise handle this mixed-match case.
##########
src/iocore/net/unit_tests/test_ConnectionTracker.cc:
##########
@@ -61,3 +65,234 @@ TEST_CASE("Connection tracker server match conversion",
"[libinknet][ConnectionT
CHECK(match == ConnectionTracker::MATCH_BOTH);
}
}
+
+namespace
+{
+
+constexpr std::string_view FQDN{"unit.test.origin"};
+
+// Whether the published store enumerates this name. Deliberately for_each
rather than lookup(),
+// because enumeration is what traffic_ctl, the JSONRPC record lookup and
stats_over_http walk, and
+// so is what "published" means to an operator.
+bool
+is_published(std::string_view metric_name)
+{
+ bool found = false;
+
+ ts::Metrics::instance().for_each(
+ [&](std::string_view name, ts::Metrics::MetricType, int64_t) { found |=
(name == metric_name); });
+
+ return found;
+}
+
+std::string
+group_metric(std::string_view stem, std::string_view addr)
+{
+ return
std::string("proxy.process.http.per_server.").append(stem).append(".").append(FQDN).append(".").append(addr);
+}
+
+std::string
+host_metric(std::string_view stem)
+{
+ return
std::string("proxy.process.http.per_server.").append(stem).append(".").append(FQDN);
+}
+
+// One upstream connection, opened and closed, following the same path as
production: HttpSM
+// reserves and then drops the group into the PoolableSession, and the session
releases it when the
+// connection closes. Group::release() is what erases the group at a zero
count, and only that makes
+// the next transaction to the same upstream construct a fresh Group and
re-evaluate
+// metric_aggregate. TxnState::release() alone decrements without erasing.
+void
+open_and_close_connection(ConnectionTracker::TxnConfig const &txn, IpEndpoint
const &addr)
+{
+ auto state = ConnectionTracker::obtain_outbound(txn, FQDN, addr);
+
+ REQUIRE(state.is_active());
+ state.reserve();
+
+ auto group = state.drop();
+ group->release();
+}
+
+ConnectionTracker::TxnConfig &
+test_config()
+{
+ // config_init keeps pointers to these for the records callbacks, so they
must outlive the test.
+ static ConnectionTracker::GlobalConfig global;
+ static ConnectionTracker::TxnConfig txn;
+ static bool initialized = false;
+
+ if (!initialized) {
+ ink_net_init(NET_SYSTEM_MODULE_PUBLIC_VERSION);
+ ConnectionTracker::config_init(&global, &txn, [](const char *, RecDataT,
RecData, void *) -> int { return REC_ERR_OKAY; });
+ initialized = true;
+ }
+
+ return txn;
+}
+
+} // namespace
+
+TEST_CASE("ConnectionTracker aggregate metric publication",
"[libinknet][ConnectionTracker]")
+{
+ auto &txn = test_config();
+
+ txn.metric_enabled = 1;
+ txn.server_match = ConnectionTracker::MATCH_BOTH;
+
+ IpEndpoint addr;
+ REQUIRE(ats_ip_pton("10.9.8.7:443", &addr) == 0);
+
+ const std::string current_group = group_metric("current_connection",
"10.9.8.7:443");
+ const std::string total_group = group_metric("total_connection",
"10.9.8.7:443");
+ const std::string blocked_group = group_metric("blocked_connection",
"10.9.8.7:443");
+
+ SECTION("AGGREGATE_NONE publishes the per group metrics and no aggregate")
+ {
+ txn.metric_aggregate = ConnectionTracker::AGGREGATE_NONE;
+ open_and_close_connection(txn, addr);
+
+ CHECK(is_published(current_group));
+ CHECK(is_published(total_group));
+ CHECK(is_published(blocked_group));
+ CHECK_FALSE(is_published(host_metric("current_connection.max")));
+ }
+
+ SECTION("AGGREGATE_GROUP publishes the per group metrics, the sums and the
max")
+ {
+ txn.metric_aggregate = ConnectionTracker::AGGREGATE_GROUP;
+ open_and_close_connection(txn, addr);
+
+ CHECK(is_published(current_group));
+ CHECK(is_published(host_metric("current_connection")));
+ CHECK(is_published(host_metric("total_connection")));
+ CHECK(is_published(host_metric("blocked_connection")));
+ CHECK(is_published(host_metric("current_connection.max")));
+ }
+
+ SECTION("AGGREGATE_MAX publishes the max and nothing else")
+ {
+ txn.metric_aggregate = ConnectionTracker::AGGREGATE_MAX;
+ open_and_close_connection(txn, addr);
+
+ CHECK(is_published(host_metric("current_connection.max")));
+
+ CHECK_FALSE(is_published(host_metric("current_connection")));
+ CHECK_FALSE(is_published(host_metric("total_connection")));
+ CHECK_FALSE(is_published(host_metric("blocked_connection")));
+ CHECK_FALSE(is_published(current_group));
+ CHECK_FALSE(is_published(total_group));
+ CHECK_FALSE(is_published(blocked_group));
+ }
+
+ SECTION("AGGREGATE_SUM publishes the sums and the max, but not the per group
metrics")
+ {
+ txn.metric_aggregate = ConnectionTracker::AGGREGATE_SUM;
+ open_and_close_connection(txn, addr);
+
+ CHECK(is_published(host_metric("current_connection")));
+ CHECK(is_published(host_metric("total_connection")));
+ CHECK(is_published(host_metric("blocked_connection")));
+ CHECK(is_published(host_metric("current_connection.max")));
+
+ CHECK_FALSE(is_published(current_group));
+ CHECK_FALSE(is_published(total_group));
+ CHECK_FALSE(is_published(blocked_group));
+ }
+
+ SECTION("switching to AGGREGATE_MAX retracts already published per group
metrics")
+ {
+ // The production sequence: run for a while with the per group metrics
published, then change
+ // the setting. Without a retraction the first set of names is published
forever.
+ txn.metric_aggregate = ConnectionTracker::AGGREGATE_NONE;
+ open_and_close_connection(txn, addr);
+ REQUIRE(is_published(current_group));
+
+ txn.metric_aggregate = ConnectionTracker::AGGREGATE_MAX;
+ open_and_close_connection(txn, addr);
+
+ CHECK_FALSE(is_published(current_group));
+ CHECK_FALSE(is_published(total_group));
+ CHECK_FALSE(is_published(blocked_group));
+ CHECK(is_published(host_metric("current_connection.max")));
+ }
+
+ SECTION("switching from AGGREGATE_SUM to AGGREGATE_MAX retracts the sums")
+ {
+ // The sums are aggregates rather than per group names, but they are
published the same way and
+ // so need withdrawing the same way when the setting stops asking for them.
+ txn.metric_aggregate = ConnectionTracker::AGGREGATE_SUM;
+ open_and_close_connection(txn, addr);
+ REQUIRE(is_published(host_metric("current_connection")));
+
+ txn.metric_aggregate = ConnectionTracker::AGGREGATE_MAX;
+ open_and_close_connection(txn, addr);
+
+ CHECK_FALSE(is_published(host_metric("current_connection")));
+ CHECK_FALSE(is_published(host_metric("total_connection")));
+ CHECK_FALSE(is_published(host_metric("blocked_connection")));
+ CHECK(is_published(host_metric("current_connection.max")));
+ }
+
+ SECTION("switching from AGGREGATE_MAX to AGGREGATE_SUM republishes the sums")
+ {
+ txn.metric_aggregate = ConnectionTracker::AGGREGATE_MAX;
+ open_and_close_connection(txn, addr);
+ REQUIRE_FALSE(is_published(host_metric("total_connection")));
+
+ txn.metric_aggregate = ConnectionTracker::AGGREGATE_SUM;
+ open_and_close_connection(txn, addr);
+
+ CHECK(is_published(host_metric("current_connection")));
+ CHECK(is_published(host_metric("total_connection")));
+ CHECK(is_published(host_metric("blocked_connection")));
+ }
+
+ SECTION("switching back to AGGREGATE_GROUP republishes the per group
metrics")
+ {
+ txn.metric_aggregate = ConnectionTracker::AGGREGATE_MAX;
+ open_and_close_connection(txn, addr);
+ REQUIRE_FALSE(is_published(current_group));
+
+ txn.metric_aggregate = ConnectionTracker::AGGREGATE_GROUP;
+ open_and_close_connection(txn, addr);
+
+ CHECK(is_published(current_group));
+ CHECK(is_published(host_metric("current_connection")));
+ }
+
+ SECTION("one hostname's groups do not unlist each other's aggregate")
+ {
+ // metric_aggregate is overridable, so two mappings to one hostname can
disagree. Both groups
+ // share the hostname's aggregate names, so a group that does not want
them must stop
+ // contributing rather than unlist a name the other one is still
publishing.
+ IpEndpoint other;
+ REQUIRE(ats_ip_pton("10.9.8.5:443", &other) == 0);
+
+ txn.metric_aggregate = ConnectionTracker::AGGREGATE_SUM;
+ open_and_close_connection(txn, addr);
+ REQUIRE(is_published(host_metric("current_connection")));
+
+ txn.metric_aggregate = ConnectionTracker::AGGREGATE_MAX;
+ open_and_close_connection(txn, other);
+
+ CHECK(is_published(host_metric("current_connection")));
+ CHECK(is_published(host_metric("current_connection.max")));
+ }
Review Comment:
This new sharing test covers two MATCH_BOTH groups only; it does not
exercise the source-ownership guard for the real name collision between a
MATCH_HOST group's own `stem.<fqdn>` metric and a MATCH_BOTH aggregate. Keep a
MATCH_HOST group alive while constructing a MATCH_BOTH group at
`AGGREGATE_NONE` and `AGGREGATE_MAX`, then assert the three shared names remain
listed, so a future name-only unlist cannot hide metrics still owned by the
host group.
--
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]
