diff --git a/doc/admin-guide/files/records.yaml.en.rst b/doc/admin-guide/files/records.yaml.en.rst index 5d75b68bafc..a24100bbc17 100644 --- a/doc/admin-guide/files/records.yaml.en.rst +++ b/doc/admin-guide/files/records.yaml.en.rst @@ -2033,13 +2033,15 @@ Origin Server Connect Attempts 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 *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 |TS| is restarted. This - affects only which metrics exist; enforcement of + connection count reaches zero, so a change is picked up the next time that upstream is reopened. + A group that never goes idle keeps whatever was in effect when it was created. + + Disabling this setting does not retire metrics that are already published: they stay in place, + frozen at their last sampled value, until |TS| is restarted. Changing + :ts:cv:`proxy.config.http.per_server.connection.metric_aggregate` does retract what it no longer + asks for, as each group is rebuilt. + + This affects only which metrics exist; enforcement of :ts:cv:`proxy.config.http.per_server.connection.max` uses the group's own connection count and is unaffected. @@ -2051,24 +2053,30 @@ Origin Server Connect Attempts :ts:cv:`proxy.config.http.per_server.connection.metric_enabled`. Has no effect when that setting is ``0``. - A per hostname aggregate sums a counter across every group belonging to that hostname that has - aggregation enabled, and exists only for + There are two kinds of per hostname aggregate. The *sums* add ``current_connection``, + ``total_connection`` and ``blocked_connection`` across every group belonging to that hostname + that has aggregation enabled. The *max* is ``current_connection.max``, the largest + ``current_connection`` among those groups, which is the one that answers how close the busiest + group is to :ts:cv:`proxy.config.http.per_server.connection.max`. Both exist only for :ts:cv:`match type ` ``both``, since that is the only match type whose group key carries the hostname. See :ref:`per-server-connection-metrics`. - ===== ====================================================================================== - Value Effect - ===== ====================================================================================== - ``0`` No aggregates. The per group metrics are published under their own names. - ``1`` Publish the per hostname aggregates and the per group metrics. - ``2`` Publish only the per hostname aggregates. The per group metrics from which they are - computed are collected but not published, which keeps the number of published metrics - proportional to hostnames rather than to groups. - ===== ====================================================================================== + ===== =========== ====== ===== + Value Per group Sums Max + ===== =========== ====== ===== + ``0`` published no no + ``1`` published yes yes + ``2`` hidden no yes + ``3`` hidden yes yes + ===== =========== ====== ===== + + ``2`` is the smallest useful configuration: one metric per hostname. ``3`` adds that hostname's + totals. Both keep the number of published metrics proportional to hostnames rather than to + groups. - With value ``2``, a group that has no aggregate to belong to -- any match type other than - ``both`` -- has its per group metrics published anyway, since otherwise nothing at all would be - reported for it. + With values ``2`` and ``3``, a group that has no aggregate to belong to -- any match type other + than ``both`` -- has its per group metrics published anyway, since otherwise nothing at all would + be reported for it. Values ``0`` and ``1`` can produce a very large number of metrics when the match type includes the address or port, since there is then one set per address and port rather than one per hostname. @@ -2079,12 +2087,17 @@ Origin Server Connect Attempts upstream had aggregation enabled, so mappings that disagree for one hostname produce an aggregate that covers only part of it. - The reload is one-directional for the same reason given under - :ts:cv:`proxy.config.http.per_server.connection.metric_enabled`. Raising the value takes effect - as upstreams reconnect, but moving to ``2`` does not hide per group metrics that are already - published, and moving from ``1`` to ``0`` does not stop the hostname aggregates from publishing. - Reducing the number of published metrics therefore requires a restart, which matters most for - ``2``, the value chosen specifically to bound that number. + A change in either direction takes effect as upstreams reconnect: a group publishes what the + new value asks for and withdraws what it does not, when that group is next rebuilt. Metrics + withdrawn this way stop appearing in :program:`traffic_ctl` output and in the other metric + consumers; they are not destroyed, and moving back republishes them with their accumulated + values intact. + + The per group metrics belong to a single group, so raising the value withdraws them as that group + is rebuilt. The sums and the max are named per hostname and shared by its groups, so a group + rebuilt for a value that does not publish them only stops contributing; they are withdrawn once + no group of that hostname publishes them. Mappings that disagree for one hostname therefore + cannot hide each other's aggregate. .. ts:cv:: CONFIG proxy.config.http.per_server.connection.metric_prefix STRING NULL :reloadable: diff --git a/doc/admin-guide/monitoring/statistics/core/http-connection.en.rst b/doc/admin-guide/monitoring/statistics/core/http-connection.en.rst index 47075ce6fa9..d5b438fcb01 100644 --- a/doc/admin-guide/monitoring/statistics/core/http-connection.en.rst +++ b/doc/admin-guide/monitoring/statistics/core/http-connection.en.rst @@ -234,21 +234,23 @@ 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, ```` is one of those three, each summed across the groups of that -hostname which have aggregation enabled, plus: +For a hostname aggregate there are two kinds. The *sums* are those same three counters, each added +across the groups of that hostname which have aggregation enabled, published at +:ts:cv:`metric_aggregate ` ``1`` and +``3``. The *max*, published at ``1``, ``2`` and ``3``, is: -current_connection_max +current_connection.max Gauge. The largest ``current_connection`` value among the groups of that hostname at the moment of sampling, so the maximum rather than the sum of the groups' current counts. This is useful because :ts:cv:`proxy.config.http.per_server.connection.max` is enforced per group rather than per hostname, so the busiest group is what determines whether connections are about to be blocked. Like ``current_connection`` it rises and falls with traffic and is not a high-water - mark. There is no per group ``current_connection_max``; it exists only as a hostname aggregate. + mark. There is no per group ``current_connection.max``; it exists only as a hostname aggregate. Because :ts:cv:`proxy.config.http.per_server.connection.metric_aggregate` is overridable, a group joins its hostname's aggregate only if the mapping that first opened that upstream had aggregation enabled. Mappings that disagree for one hostname therefore produce an aggregate over part of it: the -sums cover a subset of the groups and ``current_connection_max`` takes its maximum over that same +sums cover a subset of the groups and ``current_connection.max`` takes its maximum over that same subset, with nothing in the metric to indicate it. Keeping the setting uniform across the mappings for a hostname avoids this. @@ -261,13 +263,13 @@ Every published per server metric is recomputed periodically, currently every 5 on every connection event, so a reader sees a value up to that interval old. This is true of the hostname aggregates and of the published per group metrics alike: those are mirrored from the internal ones by the same periodic mechanism, not written as connections open and -close. It applies to ``current_connection_max`` too, which reports the maximum across groups as of +close. It applies to ``current_connection.max`` too, which reports the maximum across groups as of the last sample rather than a running peak. To obtain the peak over a longer window, compute a maximum over time from this gauge in the monitoring system. -At :ts:cv:`metric_aggregate ` value ``2`` -the per group metrics still exist internally, since the aggregates are computed from them, but are not -published. They can be listed with ``traffic_ctl metric match per_server --include-hidden``, which +At :ts:cv:`metric_aggregate ` values +``2`` and ``3`` the per group metrics still exist internally, since the aggregates are computed from +them, but are not published. They can be listed with ``traffic_ctl metric match per_server --include-hidden``, which reads them directly and so is not subject to the sampling delay above. That visibility is intended for debugging and is not a stable interface: the existence, granularity and naming of the per group metrics may change independently of the published aggregates. diff --git a/doc/developer-guide/internal-libraries/Metrics.en.rst b/doc/developer-guide/internal-libraries/Metrics.en.rst index ba68d1a9df1..4f9c86b0dae 100644 --- a/doc/developer-guide/internal-libraries/Metrics.en.rst +++ b/doc/developer-guide/internal-libraries/Metrics.en.rst @@ -171,6 +171,18 @@ Adding a source that is already registered for that derived metric is a no-op, s re-register the same source, such as one recreating an object for the same key, need not track that itself. The ``type`` and ``op`` arguments are ignored if the derived metric already exists. +``ts::Metrics::Derived::remove_source()`` is the counterpart, for a contributor that goes away or +stops wanting the aggregate published: + +.. code-block:: cpp + + ts::Metrics::Derived::remove_source("proxy.process.example.total", per_thing_metric); + +A derived metric is shared by its sources, so this does not unlist it while any remain. Removing the +last source leaves nothing to report, so the name is unlisted; adding a source again relists it. +Removing a source that is not registered, or naming a derived metric that does not exist, is a +no-op. + A hidden source can feed a published aggregate: .. code-block:: cpp diff --git a/doc/release-notes/upgrading.en.rst b/doc/release-notes/upgrading.en.rst index fd9c145f7e7..bec15027abc 100644 --- a/doc/release-notes/upgrading.en.rst +++ b/doc/release-notes/upgrading.en.rst @@ -30,6 +30,25 @@ with :cpp:func:`TSPortDescriptorDestroy`. The descriptor can be destroyed immediately after :cpp:func:`TSPortDescriptorAccept` returns because the listener does not retain it. +``ts::Metrics``, in the installed ``tsutil/Metrics.h``, no longer has an +iterator. ``Metrics::iterator``, ``begin()``, ``end()`` and ``find()`` are +removed, and enumeration is now ``Metrics::for_each(func)``, which invokes +``func(name, type, value)`` for each metric: + +.. code-block:: cpp + + ts::Metrics::instance().for_each([](std::string_view name, ts::Metrics::MetricType type, int64_t value) { + // ... + }); + +Handing out a position let a caller name a slot the store was free to change +underneath them, which is what the iterator could not be made safe against. +Reaching a single metric by name is ``lookup()``. + +``Metrics::Storage::createSpan()`` and ``Metrics::rename()`` are also removed. +Spans handed out unnamed slots that only ``rename()`` could name, and +``rename()`` mutated a name that the lock free readers hand out views of. + Upgrading to ATS v10.x ====================== diff --git a/include/iocore/net/ConnectionTracker.h b/include/iocore/net/ConnectionTracker.h index e18f6aa66c6..cc574d9846d 100644 --- a/include/iocore/net/ConnectionTracker.h +++ b/include/iocore/net/ConnectionTracker.h @@ -79,29 +79,54 @@ class ConnectionTracker * * This is independent of @c TxnConfig::metric_enabled, which decides only whether per server * metrics exist for a group at all. The per group metrics are always created in the hidden metric - * store; what varies here is what gets published from them: - * - @c AGGREGATE_NONE: no aggregate. The per group metrics are published under their own names. - * This is the default and matches the behavior of releases that had no aggregate support. - * - @c AGGREGATE_GROUP: the per hostname aggregates are published, and so are the per group - * metrics they are computed from. - * - @c AGGREGATE_ONLY: the per hostname aggregates are published and the per group metrics stay - * hidden, which keeps the published metric count proportional to hostnames rather than to - * groups. Where a group has no aggregate to belong to -- see @c Group::host_metric_name, which - * only yields a name for match type @c MATCH_BOTH -- the per group metrics are published - * anyway, since otherwise nothing at all would be reported for that group. + * store; what varies here is what gets published from them. + * + * Two kinds of per hostname aggregate exist. The *sums* are @c current_connection, + * @c total_connection and @c blocked_connection added across the groups of a hostname. The *max* + * is @c current_connection.max, the largest @c current_connection among those groups. Which of + * them are published, and whether the per group metrics are published alongside, is what this + * selects: + * + * | value | per group | sums | max | + * |----------------------|-----------|------|-----| + * | @c AGGREGATE_NONE | yes | no | no | + * | @c AGGREGATE_GROUP | yes | yes | yes | + * | @c AGGREGATE_MAX | no | no | yes | + * | @c AGGREGATE_SUM | no | yes | yes | + * + * @c AGGREGATE_NONE is the default and matches the behavior of releases that had no aggregate + * support. @c AGGREGATE_MAX is the smallest useful configuration: one metric per hostname, + * answering how close the busiest group is to @c per_server.connection.max. @c AGGREGATE_SUM adds + * the totals for that hostname. Both keep the published metric count proportional to hostnames + * rather than to groups. + * + * Where a group has no aggregate to belong to -- see @c Group::host_metric_name, which only + * yields a name for match type @c MATCH_BOTH -- the per group metrics are published whatever this + * says, since otherwise nothing at all would be reported for that group. * * Keeping the per group metrics in the hidden store in every case means changing this at runtime * is only a change of what is registered for publication, with no metric to migrate between the * two stores. * - * The records layer validates and clamps this to 0..2. A plugin setting the overridable config + * A change is applied per group, when that group is next constructed, which happens on the first + * connection after its count last fell to zero. A group that never goes idle keeps whatever was + * in effect when it was created. + * + * The per group names belong to one group, so a group that stops publishing them retracts them, + * see @c ts::Metrics::unlist. The sums and the max are named per hostname and shared by its + * groups, so a group that stops publishing those only stops contributing to them, see + * @c ts::Metrics::Derived::remove_source; they are retracted once no group of that hostname + * publishes them. + * + * The records layer validates and clamps this to 0..3. A plugin setting the overridable config * directly is not clamped, see @c METRIC_AGGREGATE_CONV; any other value behaves as - * @c AGGREGATE_GROUP, publishing both the aggregate and the per group metrics. + * @c AGGREGATE_GROUP, publishing everything. */ enum MetricAggregate : int { AGGREGATE_NONE = 0, ///< No hostname aggregate; the per group metrics are published. - AGGREGATE_GROUP = 1, ///< Hostname aggregates published, along with the per group metrics. - AGGREGATE_ONLY = 2, ///< Hostname aggregates published, per group metrics kept hidden. + AGGREGATE_GROUP = 1, ///< Hostname sums and max, published along with the per group metrics. + AGGREGATE_MAX = 2, ///< Hostname max only; the per group metrics stay hidden. + AGGREGATE_SUM = 3, ///< Hostname sums and max; the per group metrics stay hidden. }; /// Per transaction configuration values. diff --git a/include/tsutil/Metrics.h b/include/tsutil/Metrics.h index 344b3cd892b..17ccb822dce 100644 --- a/include/tsutil/Metrics.h +++ b/include/tsutil/Metrics.h @@ -33,6 +33,7 @@ #include #include #include +#include #include #include @@ -718,6 +719,18 @@ class Metrics */ static void add_source(std::string_view derived_name, Metrics::MetricType type, Metrics::AtomicType *source, Op op = Op::SUM); + /** Stop @a source contributing to a derived metric. + * + * The counterpart to @c add_source, for a contributor that goes away or stops wanting the + * aggregate published. A derived metric is shared by its sources, so this does not unlist it + * while any remain; when the last one is removed there is nothing left to report and the name + * is unlisted. Re-adding a source relists it. + * + * A source that is not registered for @a derived_name, or a name with no derived metric, is a + * no-op. + */ + static void remove_source(std::string_view derived_name, Metrics::AtomicType *source); + /** * Update derived metrics. * diff --git a/src/iocore/net/ConnectionTracker.cc b/src/iocore/net/ConnectionTracker.cc index 28b3c7fa9cd..49967a50717 100644 --- a/src/iocore/net/ConnectionTracker.cc +++ b/src/iocore/net/ConnectionTracker.cc @@ -27,6 +27,7 @@ #include "swoc/IPAddr.h" #include +#include using namespace std::literals; @@ -75,8 +76,8 @@ const MgmtConverter ConnectionTracker::SERVER_MATCH_CONV{ // 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. +// metrics, and any metric_aggregate outside 0..3 publishes everything, the same as +// AGGREGATE_GROUP. const MgmtConverter ConnectionTracker::METRIC_ENABLED_CONV{ [](const void *data) -> MgmtInt { return static_cast(*static_cast(data)); }, [](void *data, MgmtInt i) -> void { @@ -187,7 +188,7 @@ Config_Update_Conntrack_Metric_Aggregate(const char * /* name ATS_UNUSED */, Rec if (RECD_INT == dtype) { auto level = std::clamp(static_cast(data.rec_int), static_cast(ConnectionTracker::AGGREGATE_NONE), - static_cast(ConnectionTracker::AGGREGATE_ONLY)); + static_cast(ConnectionTracker::AGGREGATE_SUM)); config->metric_aggregate = static_cast(level); return true; } @@ -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 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 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; + + // 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) { + // Stop contributing rather than unlist: every group of this hostname shares these names, so + // one that does not want them must not remove a name another is still publishing. + Metrics::Derived::remove_source(sum_names[0], _count_metric); + Metrics::Derived::remove_source(sum_names[1], _count_total_metric); + Metrics::Derived::remove_source(sum_names[2], _blocked_metric); + } + + if (publish_max) { // 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); + Metrics::Derived::add_source(max_name, Metrics::MetricType::GAUGE, _count_metric, Metrics::Derived::Op::MAX); + } else if (has_aggregate) { + Metrics::Derived::remove_source(max_name, _count_metric); } - // 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) { + if (publish_group) { // 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. - Metrics::Derived::add_source("proxy.process.http.per_server.current_connection." + _metric_name, Metrics::MetricType::GAUGE, - _count_metric, Metrics::Derived::Op::SUM); - Metrics::Derived::add_source("proxy.process.http.per_server.total_connection." + _metric_name, Metrics::MetricType::COUNTER, - _count_total_metric, Metrics::Derived::Op::SUM); - Metrics::Derived::add_source("proxy.process.http.per_server.blocked_connection." + _metric_name, Metrics::MetricType::COUNTER, - _blocked_metric, Metrics::Derived::Op::SUM); + Metrics::Derived::add_source(group_names[0], Metrics::MetricType::GAUGE, _count_metric, Metrics::Derived::Op::SUM); + Metrics::Derived::add_source(group_names[1], Metrics::MetricType::COUNTER, _count_total_metric, Metrics::Derived::Op::SUM); + Metrics::Derived::add_source(group_names[2], Metrics::MetricType::COUNTER, _blocked_metric, Metrics::Derived::Op::SUM); + } else { + // Same mechanism as the aggregates above, though these names have only this group as a + // source. It leaves nothing behind for the derived pass to keep recomputing into a name that + // is no longer published. + Metrics::Derived::remove_source(group_names[0], _count_metric); + Metrics::Derived::remove_source(group_names[1], _count_total_metric); + Metrics::Derived::remove_source(group_names[2], _blocked_metric); } if (dbg_ctl.on()) { diff --git a/src/iocore/net/unit_tests/test_ConnectionTracker.cc b/src/iocore/net/unit_tests/test_ConnectionTracker.cc index ccd9d169d6f..d2666a4908f 100644 --- a/src/iocore/net/unit_tests/test_ConnectionTracker.cc +++ b/src/iocore/net/unit_tests/test_ConnectionTracker.cc @@ -22,10 +22,14 @@ */ #include "iocore/net/ConnectionTracker.h" +#include "iocore/net/Net.h" +#include "tscore/ink_inet.h" +#include "tsutil/Metrics.h" #include #include +#include TEST_CASE("Connection tracker server match conversion", "[libinknet][ConnectionTracker]") { @@ -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"))); + } + + SECTION("a group with no aggregate keeps its own metrics whatever the setting") + { + // Only MATCH_BOTH yields a hostname to gather under, so a MATCH_PORT group has no aggregate. + // Suppressing it would report nothing at all for that upstream. + txn.server_match = ConnectionTracker::MATCH_PORT; + + IpEndpoint port_addr; + REQUIRE(ats_ip_pton("10.9.8.6:80", &port_addr) == 0); + + for (auto level : {ConnectionTracker::AGGREGATE_MAX, ConnectionTracker::AGGREGATE_SUM}) { + txn.metric_aggregate = level; + open_and_close_connection(txn, port_addr); + CHECK(is_published("proxy.process.http.per_server.current_connection.10.9.8.6:80")); + } + } +} diff --git a/src/records/RecordsConfig.cc b/src/records/RecordsConfig.cc index 1e7384fa746..4e079d98371 100644 --- a/src/records/RecordsConfig.cc +++ b/src/records/RecordsConfig.cc @@ -406,7 +406,7 @@ static constexpr RecordElement RecordsConfig[] = , {RECT_CONFIG, "proxy.config.http.per_server.connection.metric_enabled", RECD_INT, "0", RECU_DYNAMIC, RR_NULL, RECC_STR, "^[0-1]$", RECA_NULL} , - {RECT_CONFIG, "proxy.config.http.per_server.connection.metric_aggregate", RECD_INT, "0", RECU_DYNAMIC, RR_NULL, RECC_STR, "^[0-2]$", RECA_NULL} + {RECT_CONFIG, "proxy.config.http.per_server.connection.metric_aggregate", RECD_INT, "0", RECU_DYNAMIC, RR_NULL, RECC_STR, "^[0-3]$", RECA_NULL} , {RECT_CONFIG, "proxy.config.http.per_server.connection.metric_prefix", RECD_STRING, "", RECU_DYNAMIC, RR_NULL, RECC_NULL, nullptr, RECA_NULL} , diff --git a/src/tsutil/Metrics.cc b/src/tsutil/Metrics.cc index 83c4c8d19a3..447c0039890 100644 --- a/src/tsutil/Metrics.cc +++ b/src/tsutil/Metrics.cc @@ -300,6 +300,34 @@ namespace details } } + /// @return @c true if @a id has no sources left, so the caller can unlist it. + bool + remove_source(Metrics::IdType id, Metrics::AtomicType *source) + { + if (!source) { + return false; + } + + std::lock_guard l(metrics_lock); + auto it = std::find_if(metrics.begin(), metrics.end(), [id](DerivedMetric const &m) { return m.metric == id; }); + + if (it == metrics.end()) { + return false; + } + + auto src = std::find(it->derived_from.begin(), it->derived_from.end(), source); + + if (src == it->derived_from.end()) { + return false; // Not a source of this metric, so nothing about it changes. + } + + it->derived_from.erase(src); + + // The entry stays, holding no sources: update() skips those, and add_source finds it again if + // a contributor comes back. + return it->derived_from.empty(); + } + static DerivativeMetrics & instance() { @@ -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); + } +} + Metrics::StaticString & Metrics::StaticString::instance() { diff --git a/src/tsutil/unit_tests/test_Metrics.cc b/src/tsutil/unit_tests/test_Metrics.cc index ff737227ce6..3b0d4fe0b5d 100644 --- a/src/tsutil/unit_tests/test_Metrics.cc +++ b/src/tsutil/unit_tests/test_Metrics.cc @@ -299,6 +299,79 @@ TEST_CASE("Metrics derived ops", "[libtsapi][Metrics]") } } +TEST_CASE("Metrics derived remove_source", "[libtsapi][Metrics]") +{ + auto &m = Metrics::instance(); + + SECTION("a derived metric stays listed while any source remains") + { + auto a = Metrics::Gauge::createHiddenPtr("rm.a"); + auto b = Metrics::Gauge::createHiddenPtr("rm.b"); + + Metrics::Derived::add_source("rm.sum", Metrics::MetricType::GAUGE, a); + Metrics::Derived::add_source("rm.sum", Metrics::MetricType::GAUGE, b); + + Metrics::Gauge::store(a, 3); + Metrics::Gauge::store(b, 4); + Metrics::Derived::update_derived(); + + auto const id = m.lookup("rm.sum"); + REQUIRE(id != Metrics::NOT_FOUND); + REQUIRE(m[id].load() == 7); + REQUIRE(m.listed(id)); + + // One contributor drops out. The metric is still someone else's, so it stays listed and now + // reports only what is left. + Metrics::Derived::remove_source("rm.sum", b); + Metrics::Derived::update_derived(); + + CHECK(m.listed(id)); + CHECK(m[id].load() == 3); + + // The last one drops out, so nothing is contributing and the name goes out of the listing. + Metrics::Derived::remove_source("rm.sum", a); + + CHECK_FALSE(m.listed(id)); + } + + SECTION("re-adding a source relists it") + { + auto a = Metrics::Gauge::createHiddenPtr("rm.relist.a"); + + Metrics::Derived::add_source("rm.relist", Metrics::MetricType::GAUGE, a); + + auto const id = m.lookup("rm.relist"); + REQUIRE(id != Metrics::NOT_FOUND); + + Metrics::Derived::remove_source("rm.relist", a); + REQUIRE_FALSE(m.listed(id)); + + Metrics::Gauge::store(a, 11); + Metrics::Derived::add_source("rm.relist", Metrics::MetricType::GAUGE, a); + Metrics::Derived::update_derived(); + + CHECK(m.listed(id)); + CHECK(m[id].load() == 11); + } + + SECTION("removing an unknown source or name is harmless") + { + auto a = Metrics::Gauge::createHiddenPtr("rm.unknown.a"); + + Metrics::Derived::remove_source("rm.no.such.derived", a); + + Metrics::Derived::add_source("rm.unknown", Metrics::MetricType::GAUGE, a); + + auto const id = m.lookup("rm.unknown"); + auto other = Metrics::Gauge::createHiddenPtr("rm.unknown.other"); + + // Not a source of this metric, so it must not empty the list or unlist anything. + Metrics::Derived::remove_source("rm.unknown", other); + + CHECK(m.listed(id)); + } +} + TEST_CASE("Metrics derived add_source", "[libtsapi][Metrics]") { auto &m = Metrics::instance(); diff --git a/tests/gold_tests/origin_connection/per_server_connection_max.test.py b/tests/gold_tests/origin_connection/per_server_connection_max.test.py index 05f2c5587c1..1724a23b40e 100644 --- a/tests/gold_tests/origin_connection/per_server_connection_max.test.py +++ b/tests/gold_tests/origin_connection/per_server_connection_max.test.py @@ -36,6 +36,11 @@ # scheduling jitter and the traffic_ctl round trip rather than racing the tick. _STAT_SYNC_WAIT_SECONDS: int = 2 +# How long to wait after changing an overridable record at runtime before driving traffic that +# should see the new value. http_config_cb schedules the reconfigure one second out, so a request +# made immediately after traffic_ctl returns is still served by the previous HttpConfigParams. +_CONFIG_APPLY_WAIT_SECONDS: int = 5 + # The records.yaml settings every ATS instance in this file needs for the waits above to hold. _STAT_SYNC_RECORDS: dict = { 'proxy.config.raw_stat_sync_interval_ms': _STAT_SYNC_INTERVAL_MS, @@ -134,7 +139,7 @@ def _test_metrics(self) -> None: # A 'port' match has one group per address:port and no hostname, so no aggregate should be # registered for it at all. tr.Processes.Default.Streams.All += Testers.ExcludesExpression( - 'per_server.current_connection_max.', 'A non-"both" match type must not register a hostname aggregate.') + 'per_server.current_connection.max.', 'A non-"both" match type must not register a hostname aggregate.') def run(self) -> None: """Configure the TestRun.""" @@ -151,11 +156,11 @@ def run(self) -> None: class ConnectMethodTest: """Test our max origin connection behavior with CONNECT traffic. - Also covers the two aggregate-publishing modes of + Also covers two of the aggregate-publishing modes of proxy.config.http.per_server.connection.metric_aggregate: - - 2 (AGGREGATE_ONLY): only the per hostname aggregate is published; the per group metrics - stay hidden and are visible only with --include-hidden. - - 1 (AGGREGATE_GROUP): the per hostname aggregate is published, and the per group metrics + - 3 (AGGREGATE_SUM): the per hostname sums and max are published; the per group metrics stay + hidden and are visible only with --include-hidden. + - 1 (AGGREGATE_GROUP): the per hostname sums and max are published, and the per group metrics are also mirrored into the published store. The match here defaults to 'both' and there is exactly one group for this hostname, so the @@ -166,7 +171,7 @@ class ConnectMethodTest: _process_counter: int = 0 _client_counter: int = 0 - def __init__(self, max_conn, metric_aggregate=2) -> None: + def __init__(self, max_conn, metric_aggregate=3) -> None: """Configure the server processes in preparation for the TestRun.""" self._metric_aggregate = metric_aggregate self._configure_dns() @@ -233,12 +238,13 @@ def _test_metrics(self, blocked) -> None: tr.Processes.Default.Streams.All += Testers.ContainsExpression( f'per_server.total_connection.{group_name} 5', 'The per group metric should be published at AGGREGATE_GROUP.') else: - # AGGREGATE_ONLY keeps the per group metrics hidden, so none of the three per group - # names may appear in a normal query. current_connection_max is not among them: it only + # AGGREGATE_SUM keeps the per group metrics hidden, so none of the three per group + # names may appear in a normal query. current_connection.max is not among them: it only # ever exists as a hostname aggregate, never per group. for counter in ('current_connection', 'total_connection', 'blocked_connection'): tr.Processes.Default.Streams.All += Testers.ExcludesExpression( - f'per_server.{counter}.{group_name} ', f'per_server.{counter}.{group_name} must stay hidden at AGGREGATE_ONLY.') + f'per_server.{counter}.{group_name} ', + f'per_server.{counter}.{group_name} must stay hidden at metric_aggregate {self._metric_aggregate}.') # The per group metrics must be visible with --include-hidden at either level. This is also # the end to end test for that traffic_ctl option. @@ -294,7 +300,7 @@ class MultiGroupAggregateTest: distinct groups sharing one host aggregate. The two groups are given different concurrency so the SUM and the MAX are distinguishable from each other. - current_connection and current_connection_max are instantaneous gauges recomputed from the live + current_connection and current_connection.max are instantaneous gauges recomputed from the live per group values every ~5s, so they rise and fall with traffic rather than remembering a peak. Observing a non-zero value therefore requires holding connections open across a sync tick. The most robust assertion, and the one that actually distinguishes this instantaneous behavior from @@ -340,9 +346,9 @@ def _configure_trafficserver(self) -> None: 'proxy.config.diags.debug.enabled': 1, 'proxy.config.diags.debug.tags': 'http|dns|hostdb|conn_track', 'proxy.config.http.per_server.connection.metric_enabled': 1, - # Aggregates only: the per group metrics stay hidden, which is what this test is - # about reading through the aggregate. - 'proxy.config.http.per_server.connection.metric_aggregate': 2, + # Sums and max, per group metrics hidden: this test is about reading the group + # behavior through the hostname aggregate. + 'proxy.config.http.per_server.connection.metric_aggregate': 3, 'proxy.config.http.per_server.connection.match': 'both', }) self._ts.Disk.remap_config.AddLines( @@ -381,9 +387,16 @@ def _test_metrics_while_held(self) -> None: 'While held open, the host aggregate current_connection should be the SUM of the ' 'currently open connections across both groups.') tr.Processes.Default.Streams.All += Testers.ContainsExpression( - f'per_server.current_connection_max.multi.origin.com {group_max}', - 'While held open, current_connection_max should be the largest single group current ' + f'per_server.current_connection.max.multi.origin.com {group_max}', + 'While held open, current_connection.max should be the largest single group current ' 'count (MAX), not the sum across the two groups.') + # The per group names end in the address, so anything matching this is a group metric and + # not the hostname aggregate. Every other assertion in this file is a ContainsExpression, + # which cannot catch a metric that should not be there at all. + tr.Processes.Default.Streams.All += Testers.ExcludesExpression( + r'per_server\.\w+_connection\.multi\.origin\.com\.\d', + 'At metric_aggregate 3 the per group metrics must stay hidden, leaving only the ' + 'hostname aggregates published.') def _test_metrics_after_drain(self) -> None: """After traffic drains and a further sync tick passes, both live gauges must read 0. @@ -403,8 +416,8 @@ def _test_metrics_after_drain(self) -> None: 'per_server.current_connection.multi.origin.com 0', 'Once all connections close, the host aggregate current_connection must drain to 0.') tr.Processes.Default.Streams.All += Testers.ContainsExpression( - 'per_server.current_connection_max.multi.origin.com 0', - 'Once all connections close, current_connection_max must also come back down to 0: it ' + 'per_server.current_connection.max.multi.origin.com 0', + 'Once all connections close, current_connection.max must also come back down to 0: it ' 'is a live gauge, not a monotone peak.') def run(self) -> None: @@ -513,15 +526,15 @@ def run(self) -> None: class AggregateOnlyWithoutHostAggregateTest: """Verify metric_aggregate 2 still publishes per group metrics when there is no aggregate. - metric_aggregate 2 (AGGREGATE_ONLY) normally leaves the per group metrics hidden and publishes - only the per hostname aggregate. That aggregate exists only under match 'both', which is the - only match type with more than one group per hostname (Group::host_metric_name returns empty - for the others). With match 'port' there is therefore nothing for the aggregate to stand in - for, so the per group metrics have to be published regardless, or level 2 would report nothing - at all for this group. + metric_aggregate 2 (AGGREGATE_MAX) normally leaves the per group metrics hidden and publishes + only the per hostname max. That aggregate exists only under match 'both', which is the only + match type with more than one group per hostname (Group::host_metric_name returns empty for + the others). With match 'port' there is therefore nothing for the aggregate to stand in for, + so the per group metrics have to be published regardless, or level 2 would report nothing at + all for this group. - Every other test in this file that sets metric_aggregate 2 uses match 'both', so without this - case a regression that dropped the fallback would leave the suite green. + Every other test in this file that suppresses the per group metrics uses match 'both', so + without this case a regression that dropped the fallback would leave the suite green. """ def __init__(self) -> None: @@ -580,9 +593,125 @@ def run(self) -> None: self._test_metrics() +class AggregateRetractionTest: + """Verify that raising metric_aggregate to 2 withdraws already published per group metrics. + + metric_aggregate 2 (AGGREGATE_MAX) publishes the per hostname max and nothing else, so this + also covers that the hostname sums are not published at that level. + + metric_aggregate is dynamic, but the publication decision is made in the ConnectionTracker + Group constructor, and a published metric name is never removed from the metric store. Before + the store grew a tombstone, a name published while the setting was 0 kept reporting for the + life of the process no matter what the setting was changed to, which is exactly what was seen + in production: per group and per hostname metrics side by side at metric_aggregate 2. + + Origin keep alive is disabled so each request opens and closes its own upstream connection. + That returns the group count to zero, which erases the group, so the next request constructs a + fresh one and re-evaluates the setting. A group that never goes idle would keep whatever was in + effect when it was created. + """ + + def __init__(self) -> None: + """Configure the processes for the test.""" + self._dns = _dns + self._server = Test.MakeHttpBinServer("retract_server") + self._configure_trafficserver() + + def _configure_trafficserver(self) -> None: + """Configure an ATS that starts out publishing the per group metrics.""" + self._ts = Test.MakeATSProcess("retract_ts") + self._ts.Disk.records_config.update( + { + **_STAT_SYNC_RECORDS, + 'proxy.config.dns.nameservers': f"127.0.0.1:{self._dns.Variables.Port}", + 'proxy.config.dns.resolv_conf': 'NULL', + 'proxy.config.http.per_server.connection.metric_enabled': 1, + # Start with the per group metrics published, then raise it at runtime below. + 'proxy.config.http.per_server.connection.metric_aggregate': 0, + 'proxy.config.http.per_server.connection.match': 'both', + # Force the upstream connection closed after each transaction so the group is + # erased and the next request rebuilds it. + 'proxy.config.http.keep_alive_enabled_out': 0, + }) + self._ts.Disk.remap_config.AddLine( + f"map http://retract.origin.com/ http://retract.origin.com:{self._server.Variables.Port}/") + + def _curl(self, tr) -> None: + """Drive one request through the remap rule.""" + tr.MakeCurlCommand(f"-v --fail -s -x 127.0.0.1:{self._ts.Variables.port} 'http://retract.origin.com/get'", ts=self._ts) + tr.Processes.Default.ReturnCode = 0 + tr.StillRunningAfter = self._ts + + def run(self) -> None: + """Publish the per group metrics, raise the setting, then verify they are withdrawn.""" + tr = Test.AddTestRun("Drive traffic with the per group metrics published") + _use_shared_dns(tr) + tr.Processes.Default.StartBefore(self._server) + tr.Processes.Default.StartBefore(self._ts) + self._curl(tr) + + tr = Test.AddTestRun("Verify the per group metrics are published at metric_aggregate 0") + tr.Processes.Default.Command = f'sleep {_STAT_SYNC_WAIT_SECONDS}; traffic_ctl metric match per_server' + tr.Processes.Default.ReturnCode = 0 + tr.Processes.Default.Env = self._ts.Env + tr.Processes.Default.TimeOut = _STAT_SYNC_WAIT_SECONDS + 30 + tr.Processes.Default.Streams.All = Testers.ContainsExpression( + r'per_server\.current_connection\.retract\.origin\.com\.\d', + 'At metric_aggregate 0 the per group metric is published under its own name. Without ' + 'this the retraction below would be vacuous.') + tr.StillRunningAfter = self._ts + + tr = Test.AddTestRun("Raise metric_aggregate to 2") + # http_config_cb schedules the reconfigure a second after the record changes + # (HttpConfig.cc), so the new HttpConfigParams is not in place the instant traffic_ctl + # returns. Without this wait the next request is served by the old configuration and + # rebuilds the group under the old setting, which looks exactly like a failure to retract. + tr.Processes.Default.Command = ( + 'traffic_ctl config set proxy.config.http.per_server.connection.metric_aggregate 2 && ' + 'traffic_ctl config reload && ' + f'sleep {_CONFIG_APPLY_WAIT_SECONDS}') + tr.Processes.Default.ReturnCode = 0 + tr.Processes.Default.Env = self._ts.Env + tr.Processes.Default.TimeOut = _CONFIG_APPLY_WAIT_SECONDS + 30 + tr.StillRunningAfter = self._ts + + tr = Test.AddTestRun("Verify the new metric_aggregate is in effect") + tr.Processes.Default.Command = 'traffic_ctl config get proxy.config.http.per_server.connection.metric_aggregate' + tr.Processes.Default.ReturnCode = 0 + tr.Processes.Default.Env = self._ts.Env + tr.Processes.Default.TimeOut = 30 + tr.Processes.Default.Streams.All = Testers.ContainsExpression( + r'metric_aggregate: 2', 'The record must carry the new value before behavior is asserted against it.') + tr.StillRunningAfter = self._ts + + tr = Test.AddTestRun("Drive traffic again so the group is rebuilt under the new setting") + self._curl(tr) + + tr = Test.AddTestRun("Verify the per group metrics were withdrawn") + tr.Processes.Default.Command = f'sleep {_STAT_SYNC_WAIT_SECONDS}; traffic_ctl metric match per_server' + tr.Processes.Default.ReturnCode = 0 + tr.Processes.Default.Env = self._ts.Env + tr.Processes.Default.TimeOut = _STAT_SYNC_WAIT_SECONDS + 30 + tr.Processes.Default.Streams.All = Testers.ExcludesExpression( + r'per_server\.\w+_connection\.retract\.origin\.com\.\d', + 'Once metric_aggregate is 2 and the group has been rebuilt, the per group metrics must ' + 'no longer be published, even though they were published earlier in this process.') + tr.Processes.Default.Streams.All += Testers.ContainsExpression( + r'per_server\.current_connection\.max\.retract\.origin\.com', + 'The hostname max stands in for the withdrawn per group metrics.') + # metric_aggregate 2 is the max and nothing else, so the hostname sums must not appear + # either. '\.com ' with the trailing space matches the aggregate names, whose value follows + # the hostname directly; the max is 'current_connection.max.' and does not match. + tr.Processes.Default.Streams.All += Testers.ExcludesExpression( + r'per_server\.\w+_connection\.retract\.origin\.com ', + 'At metric_aggregate 2 only the max is published: the hostname sums must be absent.') + tr.StillRunningAfter = self._ts + + PerServerConnectionMaxTest().run() -ConnectMethodTest(3, metric_aggregate=2).run(blocked=2, gold_file="gold/two_503_congested.gold") +ConnectMethodTest(3, metric_aggregate=3).run(blocked=2, gold_file="gold/two_503_congested.gold") ConnectMethodTest(0, metric_aggregate=1).run(blocked=0, gold_file="gold/two_200_ok.gold") MultiGroupAggregateTest().run() MetricOverrideTest().run() AggregateOnlyWithoutHostAggregateTest().run() +AggregateRetractionTest().run()