bryancall commented on code in PR #13506:
URL: https://github.com/apache/trafficserver/pull/13506#discussion_r3854592636
##########
doc/admin-guide/files/records.yaml.en.rst:
##########
@@ -2007,6 +2007,80 @@ Origin Server Connect Attempts
the connection. Useful when the origin supports keep-alive, removing the
time needed to set up a
new connection from the next request at the expense of added (inactive)
connections.
+.. ts:cv:: CONFIG proxy.config.http.per_server.connection.metric_enabled INT 0
+ :reloadable:
+ :overridable:
+
+ Enable per upstream server connection metrics. These metrics are
dynamically named, one set per
+ upstream server group, so the number of them scales with the number of
distinct upstream servers
+ seen. See :ref:`per-server-connection-metrics`.
+
+ =====
======================================================================================
+ Value Effect
+ =====
======================================================================================
+ ``0`` No per server connection metrics.
+ ``1`` Per server connection metrics are collected for each upstream server
group.
+ =====
======================================================================================
+
+ What is published from them is controlled separately by
+ :ts:cv:`proxy.config.http.per_server.connection.metric_aggregate`, which by
default publishes the
+ per group metrics themselves.
+
+ Because this is overridable, metrics can be enabled for the upstreams of
interest and left off
+ for the rest, for example with :ref:`admin-plugins-conf-remap` on a
specific mapping.
+
+ The value is applied when a connection group is created. Where two mappings
that disagree about
+ this setting resolve to the same group -- that is, the same key under
+ :ts:cv:`proxy.config.http.per_server.connection.match` -- the transaction
that creates the group
+ determines its metrics, and later transactions do not change them. A group
is discarded once its
+ connection count reaches zero, so the choice is made again the next time
that upstream is
+ reopened. This affects only which metrics exist; enforcement of
Review Comment:
This is the sentence I would change. "the choice is made again the next time
that upstream is reopened" reads as full reversibility, and it only holds for
increasing publication. Turning metrics off, or switching `metric_aggregate` to
`2`, leaves the already-published names in place for the life of the process.
```suggestion
determines its metrics, and later transactions do not change them. A
group is discarded once its
connection count reaches zero, so *raising* the level of publication is
picked up the next time
that upstream is reopened: enabling metrics, or enabling the aggregates,
takes effect as upstreams
reconnect. Lowering it does not. Metrics are never retired once
published, so disabling this
setting, or switching
:ts:cv:`proxy.config.http.per_server.connection.metric_aggregate` to
``2``, leaves the names that
are already published in place, frozen at their last sampled value, until
Traffic Server is
restarted. This affects only which metrics exist; enforcement of
```
The same caveat is needed on `metric_aggregate` below, which currently just
points back here with "the same consequence for mappings that disagree and
resolve to the same group". It is worth spelling out there too, since `2` is
the value an operator reaches for specifically to reduce a metric count, and
that is the one direction a reload cannot deliver.
##########
doc/admin-guide/monitoring/statistics/core/http-connection.en.rst:
##########
@@ -201,6 +201,64 @@ HTTP Connection
Current number of TCP connections for tunnels where the far end is the
server,
except for those counted by
``proxy.process.tunnel.current_server_connections_tls``
+.. _per-server-connection-metrics:
+
+Per Server Connection Metrics
+-----------------------------
+
+Unlike the metrics above these do not have fixed names. They are created
dynamically, one set per
+upstream server group as defined by
:ts:cv:`proxy.config.http.per_server.connection.match`, and,
+when the match type is ``both``, one aggregate set per hostname. Whether they
are collected at all is
+controlled by :ts:cv:`proxy.config.http.per_server.connection.metric_enabled`,
and which of them are
+published by
:ts:cv:`proxy.config.http.per_server.connection.metric_aggregate`. An optional
+:ts:cv:`proxy.config.http.per_server.connection.metric_prefix` can be inserted
into the names.
+
+Per group names are ``proxy.process.http.per_server.<counter>.<group>``, where
``<group>`` depends on
+the match type: an IP address, an ``address:port`` pair, a hostname, or, for
``both``,
+``<hostname>.<address:port>``. Per hostname names are
+``proxy.process.http.per_server.<counter>.<hostname>``. Aggregates exist only
for match type
+``both``, because that is the only match type with more than one group per
hostname; for match type
+``host`` the group name is already the bare hostname, so an aggregate would
carry the same name as
+the single group it summarises.
+
+For a group, ``<counter>`` is one of:
+
+current_connection
+ Gauge. The number of connections currently open to the group.
+
+total_connection
+ Counter. The total number of connections ever opened to the group. Never
decreases.
+
+blocked_connection
+ Counter. The total number of connection attempts to the group blocked by
+ :ts:cv:`proxy.config.http.per_server.connection.max`. Never decreases.
+
+For a hostname aggregate, ``<counter>`` is one of those three, each summed
across the groups of that
+hostname, plus:
Review Comment:
"each summed across the groups of that hostname" is what an operator will
rely on, and it can be false. Aggregate membership is registered per group,
gated on the `metric_aggregate` value of the transaction that created that
group, so under `match: both` a mapping with `metric_aggregate: 1` joins the
aggregate and a mapping with `0` for the same hostname does not. The sum then
covers a strict subset, and `current_connection_max` takes its maximum over
that subset, with nothing indicating it.
```suggestion
For a hostname aggregate, ``<counter>`` is one of those three, each summed
across the groups of that
hostname which have aggregation enabled, plus:
```
I would add a short paragraph after the `current_connection_max` entry as
well, along the lines of: because `metric_aggregate` is overridable, a group
joins its hostname's aggregate only if the mapping that first opened that
upstream had aggregation enabled, so mappings that disagree for one hostname
produce an aggregate over part of it. Making membership a per-hostname decision
is the better fix, but that can go with the `remove_source` follow-up.
##########
doc/admin-guide/monitoring/statistics/core/http-connection.en.rst:
##########
@@ -201,6 +201,64 @@ HTTP Connection
Current number of TCP connections for tunnels where the far end is the
server,
except for those counted by
``proxy.process.tunnel.current_server_connections_tls``
+.. _per-server-connection-metrics:
+
+Per Server Connection Metrics
+-----------------------------
+
+Unlike the metrics above these do not have fixed names. They are created
dynamically, one set per
+upstream server group as defined by
:ts:cv:`proxy.config.http.per_server.connection.match`, and,
+when the match type is ``both``, one aggregate set per hostname. Whether they
are collected at all is
+controlled by :ts:cv:`proxy.config.http.per_server.connection.metric_enabled`,
and which of them are
+published by
:ts:cv:`proxy.config.http.per_server.connection.metric_aggregate`. An optional
+:ts:cv:`proxy.config.http.per_server.connection.metric_prefix` can be inserted
into the names.
+
+Per group names are ``proxy.process.http.per_server.<counter>.<group>``, where
``<group>`` depends on
+the match type: an IP address, an ``address:port`` pair, a hostname, or, for
``both``,
+``<hostname>.<address:port>``. Per hostname names are
+``proxy.process.http.per_server.<counter>.<hostname>``. Aggregates exist only
for match type
+``both``, because that is the only match type with more than one group per
hostname; for match type
+``host`` the group name is already the bare hostname, so an aggregate would
carry the same name as
+the single group it summarises.
Review Comment:
"the only match type with more than one group per hostname" is not accurate,
and the same claim appears in `records.yaml.en.rst`, in the `MetricAggregate`
doc comment, and on `host_metric_name`. `Group::hash` and `Group::equal` key
`MATCH_IP` on the address alone, so one hostname with several A records already
yields several groups.
The real reason is narrower and, I think, more useful to a future reader:
`ip` and `port` keys carry no hostname at all, and one such group is shared by
every hostname that resolves to that address, so there is nothing to aggregate
it under. As written, someone will eventually relax the restriction on the
strength of a premise that is false.
```suggestion
``proxy.process.http.per_server.<counter>.<hostname>``. Aggregates exist
only for match type
``both``, because that is the only match type whose group key carries the
hostname. An ``ip`` or
``port`` group is keyed on the address alone and is shared by every hostname
that resolves to it, so
there is no single hostname to aggregate it under. For match type ``host``
the group name is already
the bare hostname, so an aggregate would carry the same name as the single
group it summarises.
```
##########
src/iocore/net/ConnectionTracker.cc:
##########
@@ -440,11 +475,45 @@ ConnectionTracker::Group::Group(DirectionType direction,
Key const &key, std::st
{
Metrics::Gauge::increment(net_rsb.connection_tracker_table_size);
// only add metrics for server connections
- if (_global_config->metric_enabled && direction == DirectionType::OUTBOUND) {
+ if (metric_enabled && direction == DirectionType::OUTBOUND) {
std::string _metric_name = metric_name(key, fqdn,
_global_config->metric_prefix);
- _count_metric =
Metrics::Gauge::createPtr("proxy.process.http.per_server.current_connection.",
_metric_name);
- _count_total_metric =
Metrics::Counter::createPtr("proxy.process.http.per_server.total_connection.",
_metric_name);
- _blocked_metric =
Metrics::Counter::createPtr("proxy.process.http.per_server.blocked_connection.",
_metric_name);
+ // Per group metrics always live in the hidden store. metric_aggregate
controls what is
+ // published from them (see MetricAggregate), not whether they exist.
+ _count_metric =
Metrics::Gauge::createHiddenPtr("proxy.process.http.per_server.current_connection.",
_metric_name);
+ _count_total_metric =
Metrics::Counter::createHiddenPtr("proxy.process.http.per_server.total_connection.",
_metric_name);
+ _blocked_metric =
Metrics::Counter::createHiddenPtr("proxy.process.http.per_server.blocked_connection.",
_metric_name);
+
+ // Only MATCH_BOTH groups have siblings sharing a hostname to aggregate
across.
+ 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);
+ // The largest current count among this hostname's groups, sampled.
Deliberately taken over
+ // the instantaneous gauge rather than each group's all time peak, so
the value falls again
+ // and a maximum over time can be computed by whatever scrapes it.
+
Metrics::Derived::add_source("proxy.process.http.per_server.current_connection_max."
+ _host_metric_name,
+ Metrics::MetricType::GAUGE, _count_metric,
Metrics::Derived::Op::MAX);
+ }
+
+ // AGGREGATE_ONLY suppresses the per group metrics to keep the published
count proportional to
+ // hostnames. Without an aggregate to stand in for them there would be
nothing at all reported
+ // for this group, so in that case publish them regardless.
+ if (metric_aggregate != AGGREGATE_ONLY || !has_aggregate) {
+ // Mirror the per group metrics into the published store under their own
name. A single
+ // source SUM is an identity: the published value always equals the
hidden source.
Review Comment:
"the published value always equals the hidden source" is the one claim in
this change that contradicts its own design. The published copy is stale
between derived ticks and reads 0 from creation until the first one, which is
precisely the property `http-connection.en.rst` takes care to explain. A
single-source SUM does not combine anything, but it is still a sample rather
than an identity.
```suggestion
// Mirror the per group metrics into the published store under their
own name. A single
// source SUM combines nothing, but the published value is still a
sample: it is whatever
// the last derived tick read, and it reads 0 from creation until that
first tick.
```
##########
src/iocore/net/ConnectionTracker.cc:
##########
@@ -70,10 +72,28 @@ const MgmtConverter ConnectionTracker::SERVER_MATCH_CONV{
}
}};
+// Neither of these clamps, for the same reason as SERVER_MATCH_CONV above:
the InkAPITest
+// regression test requires an arbitrary integer to round trip through the
setter and getter. The
+// records paths do the range checking instead -- records.yaml validates the
value and the reload
+// callbacks below clamp -- so an out of range value is only reachable by a
plugin that sets one
+// deliberately. Both settings degrade safely if that happens: any non-zero
metric_enabled enables
+// metrics, and any metric_aggregate outside 0..2 publishes both the aggregate
and the per group
+// metrics, the same as AGGREGATE_GROUP.
+const MgmtConverter ConnectionTracker::METRIC_ENABLED_CONV{
Review Comment:
Not blocking, and I accept the reasoning in the comment above for why
neither converter clamps. What I would still like is a diagnostic somewhere on
this path, because right now there is none anywhere in the new code.
`SERVER_MATCH_CONV`, three converters up, has a formatter and calls
`Warning_Bad_Match_Type`. Both new reload callbacks clamp silently, and their
`return false` on a wrong `dtype` is swallowed, since the `RecCore` lambda
returns `REC_ERR_OKAY` unconditionally and `traffic_ctl` reports success either
way. The sibling `Config_Update_Conntrack_Match` does at least emit
`Warning("Invalid type for '%s'")`. The net effect is that a plugin setting
`metric_aggregate = 7` reads back 7, behaves as `AGGREGATE_GROUP`, and leaves
no trace of either fact.
Related, and the same size of fix: the debug print at line 520 says
`Registered per_server_connection.{}` without saying what was actually
registered, hidden or published, aggregate or per group. That line is the first
place an operator will look to answer "why can I not see my metric", which is
going to be a common question given how many ways this feature can decline to
publish something.
##########
include/iocore/net/ConnectionTracker.h:
##########
@@ -171,6 +206,20 @@ class ConnectionTracker
std::time_t get_last_alert_epoch_time() const;
static std::string metric_name(const Key &key, std::string_view fqdn,
std::string metric_prefix);
+ /** Name of the metric which aggregates a value across all groups of a
hostname.
+ *
+ * Only @c MATCH_BOTH groups have more than one group per hostname. For @c
MATCH_HOST there is
+ * exactly one group per hostname, so an aggregate would be over a set of
one, and
+ * @c Group::metric_name already returns the FQDN alone for that match
type - identical to what
+ * this would return, so publishing both would collide on one name.
Review Comment:
Two problems in this paragraph. The first sentence repeats the inaccurate
reason described on the monitoring page. The second is that the collision
argument is right within one match type and does not hold across two, which
matters because `server_match` is itself overridable: foo.com can be
`MATCH_HOST` on one mapping and `MATCH_BOTH` on another, at which point
`metric_name` for the host group and `host_metric_name` for the aggregate
produce the identical string. `add_source` then finds the existing entry by id,
appends the source, and ignores the later caller's op, so one published name
silently carries the union of a per-group mirror and a cross-group aggregate.
```suggestion
* Only @c MATCH_BOTH keys carry both a hostname and an address, so it
is the only match type
* whose groups can be gathered by hostname at all. @c MATCH_IP and @c
MATCH_PORT key on the
* address alone and one such group is shared by every hostname
resolving to it, so there is no
* single hostname to aggregate it under. For @c MATCH_HOST there is
exactly one group per
* hostname, so an aggregate would be over a set of one, and @c
Group::metric_name already
* returns the FQDN alone for that match type - identical to what this
would return, so
* publishing both would collide on one name.
*
* Note that reasoning holds within a single match type. @c
TxnConfig::server_match is
* overridable, so one hostname can be @c MATCH_HOST on one mapping and
@c MATCH_BOTH on
* another, and then that group's own published name and this aggregate
name are the same
* string and are merged into one derived metric.
```
Documenting it is enough for this PR. Including the match type in the
aggregate name, or warning on the collision, is the actual fix and can go with
the follow-up.
##########
include/iocore/net/ConnectionTracker.h:
##########
@@ -155,8 +187,11 @@ class ConnectionTracker
* @param key A populated @c Key structure - values are copied to the @c
Group.
* @param fqdn The full FQDN.
* @param min_keep_alive The minimum number of origin keep alive
connections to maintain.
+ * @param metric_enabled Whether the transaction creating this group wants
per server metrics.
+ * @param metric_aggregate What that transaction wants published, see @c
MetricAggregate.
*/
- Group(DirectionType direction, Key const &key, std::string_view fqdn, int
min_keep_alive);
+ Group(DirectionType direction, Key const &key, std::string_view fqdn, int
min_keep_alive, int metric_enabled = 0,
+ MetricAggregate metric_aggregate = AGGREGATE_NONE);
Review Comment:
Suggestion only. I would drop both default arguments. There are two
construction sites, and only the INBOUND one 21 lines away in `obtain_inbound`
uses the defaults, so the defaults buy one short call and cost a silent failure
mode: a future outbound site that forgets these arguments compiles cleanly and
gets no metrics.
The sharper version is that `min_keep_alive` and `metric_enabled` are
adjacent, both `int`, so transposing them also compiles and produces a group
with metrics keyed off a keep-alive count. Taking `bool metric_enabled` would
remove that particular hazard, and it matches how the value is actually used
here, which is a plain truth test.
--
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]
