diff --git a/doc/admin-guide/files/records.yaml.en.rst b/doc/admin-guide/files/records.yaml.en.rst index 5d75b68bafc..b05e6c85697 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 + ===== =========== ====== ===== - 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. + ``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 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,14 @@ 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. + + Because the sums are named per hostname rather than per group, where the mappings for one + hostname disagree about this setting the last group rebuilt decides whether they are published. .. 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 4822170546e..f78af61a40f 100644 --- a/doc/developer-guide/internal-libraries/Metrics.en.rst +++ b/doc/developer-guide/internal-libraries/Metrics.en.rst @@ -188,6 +188,46 @@ sampling point*, not the true peak. There are two ways to arrange this, with dif Which is appropriate depends on whether the consumer needs to aggregate over time downstream. +Unlisting a metric +================== + +A metric can be taken out of the store's listing after the fact. An unlisted metric is skipped by +iteration, so it disappears from ``traffic_ctl metric match``, the JSONRPC record lookup and +``stats_over_http``, without either of those consumers needing to know about it: + +.. code-block:: cpp + + auto &m = ts::Metrics::instance(); + + m.unlist(id); // by id + m.unlist("proxy.process.example"); // or by name + + m.relist(id); // put it back + +The slot, the name and the atomic all survive: an unlisted number that still rings. An unlisted +metric still resolves through ``lookup``, so an exact name query, a logging field reference and +``TSStatFindName`` all continue to work, and its value may still be read and written. Creating the +same name again relists it and returns the same id with its accumulated value intact, so a metric +that comes and goes with a configuration setting costs nothing to bring back. + +``find`` is the exception: it returns ``end()`` for an unlisted metric. Iteration never visits an +unlisted slot, so an iterator pointing at one would be a range bound that a walk steps straight over +and never reaches. Use ``lookup`` to read an unlisted metric. + +This exists because the decision to publish a name is otherwise made once, when the metric is first +created, and can never be revisited. Any metric whose name or publication policy depends on a +runtime changeable setting needs a way to retract a name it has already published. + +.. important:: + + Unlisting hides; it does not free. The slot and the name remain allocated against the storage + limit below. Unlisting does not make an unbounded naming scheme safe. + +.. note:: + + Iteration is a snapshot taken when the iterator is created. A metric created after ``begin()`` + is not visited by that iterator. + Storage limits ============== diff --git a/include/iocore/net/ConnectionTracker.h b/include/iocore/net/ConnectionTracker.h index e18f6aa66c6..50bc75daa64 100644 --- a/include/iocore/net/ConnectionTracker.h +++ b/include/iocore/net/ConnectionTracker.h @@ -79,29 +79,51 @@ 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. Retracting a published name relies on the metric store's + * listing, see @c ts::Metrics::unlist. The sums are named per hostname rather than per group, so + * where the mappings for one hostname disagree about this setting, the last group constructed + * decides whether they are published. + * + * 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 741b1e069a5..73453ab7670 100644 --- a/include/tsutil/Metrics.h +++ b/include/tsutil/Metrics.h @@ -94,13 +94,20 @@ class Metrics static constexpr int METRIC_TYPE_MASK = 0x1FFF; private: - using NameAndId = std::tuple; - using LookupTable = std::unordered_map; - using NameStorage = std::array; - using AtomicStorage = std::array; - using NamesAndAtomics = std::tuple; + using NameAndId = std::tuple; + using LookupTable = std::unordered_map; + using NameStorage = std::array; + using AtomicStorage = std::array; + /// Per slot flag bits, see @c UNLISTED. A parallel array rather than a member of @c NameAndId + /// because an atomic member would make that tuple neither copyable nor movable, and the slot is + /// written there with a tuple assignment. + using FlagStorage = std::array, MAX_SIZE>; + using NamesAndAtomics = std::tuple; using BlobStorage = std::array, MAX_BLOBS>; + /// The slot exists and is still resolvable by name or id, but is skipped by iteration. + static constexpr uint8_t UNLISTED = 0x01; + public: Metrics(const self_type &) = delete; self_type &operator=(const self_type &) = delete; @@ -145,6 +152,57 @@ class Metrics { return _storage->lookup(id, out_name, type); } + + /** Take @a id out of the store's listing. + * + * An unlisted metric keeps its slot, its name and its atomic. It is skipped by iteration, so it + * vanishes from everything that enumerates the store, but it still resolves through @c lookup and + * its value may still be read and written -- an unlisted number that still rings. Creating the + * same name again relists it and returns the same id. + * + * @return @c false if @a id does not name an allocated slot. + */ + bool + unlist(IdType id) + { + return _storage->set_listed(id, false); + } + + /// Put @a id back in the listing. @see unlist + bool + relist(IdType id) + { + return _storage->set_listed(id, true); + } + + /** Whether @a id is enumerated. + * + * @return @c false for an unlisted metric, and also for an id that names no allocated slot -- + * neither appears in iteration. + */ + bool + listed(IdType id) const + { + return _storage->listed(id); + } + + /// Convenience for callers that publish by name and do not retain the id. @see unlist + bool + unlist(std::string_view name) + { + auto id = lookup(name); + + return id != NOT_FOUND && unlist(id); + } + + /// Convenience for callers that publish by name and do not retain the id. @see relist + bool + relist(std::string_view name) + { + auto id = lookup(name); + + return id != NOT_FOUND && relist(id); + } AtomicType & operator[](IdType id) { @@ -194,6 +252,18 @@ class Metrics // Static methods to encapsulate access to the atomic's class iterator { + friend class Metrics; + + /// Tag for the end sentinel, which has no position and reads no storage. + struct end_tag { + }; + + // Only Metrics hands these out, through begin(), end() and find(). A caller that could name an + // arbitrary position could name an unlisted one, which iteration must never visit. + explicit iterator(const Metrics &m); + iterator(const Metrics &m, IdType pos); + iterator(const Metrics &m, end_tag); + public: using iterator_category = std::input_iterator_tag; using value_type = std::tuple; @@ -201,8 +271,6 @@ class Metrics using pointer = value_type *; using reference = value_type &; - iterator(const Metrics &m, IdType pos) : _metrics(m), _it(pos) {} - iterator & operator++() { @@ -231,35 +299,70 @@ class Metrics return std::make_tuple(name, type, metric->_value.load()); } + /** Equality. + * + * Three way rather than a plain position compare: any exhausted iterator equals the end + * sentinel, and equals any other exhausted iterator, since two of them may have skipped a + * different number of unlisted slots. Two live iterators still compare by position. + * + * Two positional iterators may hold different snapshots, so exhaustion between them is judged + * against the earlier bound. Otherwise a walk could pass its own bound while a stop iterator + * made later was still live: they would never compare equal and @c operator++ could not make + * progress. The sentinel keeps its own answer, since its bound is meaningless. + * + * @note A snapshot is the sequence: iterators from different ones are no more comparable than + * iterators into different containers, and mixing them is unspecified. Within one snapshot + * equality is the equivalence relation an input iterator requires. The rule above keeps the + * unspecified case terminating rather than hanging. + */ bool operator==(const iterator &o) const { - return _it == o._it && std::addressof(_metrics) == std::addressof(o._metrics); - } + if (std::addressof(_metrics) != std::addressof(o._metrics)) { + return false; + } - bool - operator!=(const iterator &o) const - { - return _it != o._it || std::addressof(_metrics) != std::addressof(o._metrics); + if (_end || o._end) { + return at_end() == o.at_end(); + } + + auto const bound = _bound < o._bound ? _bound : o._bound; + bool const a = _it >= bound, b = o._it >= bound; + + if (a || b) { + return a && b; + } + return _it == o._it; } private: void next(); + void advance(); + void skip_unlisted(); + + bool + at_end() const + { + return _end || _it >= _bound; + } const Metrics &_metrics; - Metrics::IdType _it; + Metrics::IdType _it{0}; + /// One past the last slot allocated when this iterator was made. Iteration is a snapshot. + Metrics::IdType _bound{0}; + bool _end{false}; }; iterator begin() const { - return iterator(*this, 0); + return iterator(*this); } iterator end() const { - return iterator(*this, _storage->next_free_id()); + return iterator(*this, iterator::end_tag{}); } iterator @@ -267,7 +370,9 @@ class Metrics { auto id = lookup(name); - if (id == NOT_FOUND) { + // An unlisted slot is never visited by iteration, so handing out an iterator to one would + // produce a bound that a skipping walk steps straight over. Reach it with lookup() instead. + if (id == NOT_FOUND || !listed(id)) { return end(); } else { return iterator(*this, id); @@ -349,6 +454,8 @@ class Metrics AtomicType *lookup(Metrics::IdType id, std::string_view *out_name = nullptr, MetricType *out_type = nullptr) const; std::string_view name(IdType id) const; MetricType type(IdType id) const; + bool set_listed(IdType id, bool listed); + bool listed(IdType id) const; /// The id the next slot will get, which is also iteration's exclusive bound. IdType diff --git a/src/iocore/net/CMakeLists.txt b/src/iocore/net/CMakeLists.txt index fbffe744194..62c869586fe 100644 --- a/src/iocore/net/CMakeLists.txt +++ b/src/iocore/net/CMakeLists.txt @@ -145,6 +145,7 @@ if(BUILD_TESTING) libinknet_stub.cc NetVCTest.cc unit_tests/test_NetHandler.cc + unit_tests/test_ConnectionTracker.cc unit_tests/test_ProxyProtocol.cc unit_tests/test_SSLCertLookup.cc unit_tests/test_SSLNetVConnectionAsyncEp.cc diff --git a/src/iocore/net/ConnectionTracker.cc b/src/iocore/net/ConnectionTracker.cc index e8e56cabd12..024e8a34ec2 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; @@ -77,8 +78,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 { @@ -189,7 +190,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; } @@ -487,33 +488,67 @@ 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; + + auto &metrics = Metrics::instance(); + + // 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) { + for (auto const &name : sum_names) { + metrics.unlist(name); + } + } + + 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.unlist(max_name); } - // 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 { + for (auto const &name : group_names) { + metrics.unlist(name); + } } if (dbg_ctl.on()) { diff --git a/src/iocore/net/unit_tests/test_ConnectionTracker.cc b/src/iocore/net/unit_tests/test_ConnectionTracker.cc new file mode 100644 index 00000000000..c33246d23c9 --- /dev/null +++ b/src/iocore/net/unit_tests/test_ConnectionTracker.cc @@ -0,0 +1,240 @@ +/** @file + + Catch based unit tests for per upstream server metric publication. + + @section license License + + Licensed to the Apache Software Foundation (ASF) under one or more contributor license agreements. + See the NOTICE file distributed with this work for additional information regarding copyright + ownership. The ASF licenses this file to you under the Apache License, Version 2.0 (the + "License"); you may not use this file except in compliance with the License. You may obtain a + copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + + Unless required by applicable law or agreed to in writing, software distributed under the License + is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express + or implied. See the License for the specific language governing permissions and limitations under + the License. + */ + +#include + +#include + +#include "iocore/net/ConnectionTracker.h" +#include "iocore/net/Net.h" +#include "tsutil/Metrics.h" +#include "tscore/ink_inet.h" + +namespace +{ + +constexpr std::string_view FQDN{"unit.test.origin"}; + +// Whether the published store enumerates this name. Deliberately iteration 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) +{ + for (auto &&[name, type, value] : ts::Metrics::instance()) { + if (name == metric_name) { + return true; + } + } + return false; +} + +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", "[net][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("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 e429ac4680b..47b726e817f 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/records/unit_tests/test_RecHiddenMetricLookup.cc b/src/records/unit_tests/test_RecHiddenMetricLookup.cc index d1e2d4c4fcb..c4f6e2e3b16 100644 --- a/src/records/unit_tests/test_RecHiddenMetricLookup.cc +++ b/src/records/unit_tests/test_RecHiddenMetricLookup.cc @@ -115,3 +115,59 @@ TEST_CASE("RecLookupMatchingRecords - hidden metrics", "[librecords][RecLookup][ } } } + +TEST_CASE("RecLookupMatchingRecords - unlisted metrics", "[librecords][RecLookup][unlisted]") +{ + const std::string name = "proxy.test.lookup.unlisted_gauge"; + auto *m = ts::Metrics::Gauge::createPtr(name); + + REQUIRE(m != nullptr); + m->store(7); + + auto &metrics = ts::Metrics::instance(); + auto id = metrics.lookup(name); + + REQUIRE(id != ts::Metrics::NOT_FOUND); + REQUIRE(metrics.unlist(id)); + + SECTION("an unlisted metric is not enumerated") + { + std::vector entries; + + REQUIRE(RecLookupMatchingRecords(RECT_ALL, name.c_str(), collect, &entries) == REC_ERR_OKAY); + + for (const auto &e : entries) { + CHECK(e.name != name); + } + } + + SECTION("an unlisted metric is still found by exact name") + { + // RecLookupRecord resolves through Metrics::lookup() rather than iteration, which is what keeps + // logging fields and TSStatFindName working across an unlisting. + std::vector entries; + + REQUIRE(RecLookupRecord(name.c_str(), collect, &entries) == REC_ERR_OKAY); + REQUIRE(entries.size() == 1); + CHECK(entries[0].name == name); + CHECK(entries[0].int_value == 7); + } + + SECTION("relisting puts it back in enumeration") + { + REQUIRE(metrics.relist(id)); + + std::vector entries; + bool found = false; + + REQUIRE(RecLookupMatchingRecords(RECT_ALL, name.c_str(), collect, &entries) == REC_ERR_OKAY); + for (const auto &e : entries) { + if (e.name == name) { + found = true; + CHECK(e.int_value == 7); + } + } + + REQUIRE(found); + } +} diff --git a/src/tsutil/Metrics.cc b/src/tsutil/Metrics.cc index 0b9ef8dc5ac..a4eea599402 100644 --- a/src/tsutil/Metrics.cc +++ b/src/tsutil/Metrics.cc @@ -77,6 +77,10 @@ Metrics::Storage::create(std::string_view name, const MetricType type) auto it = _lookups.find(name); if (it != _lookups.end()) { + // Re-creating a name relists it: same slot, same atomic, and whatever value it accumulated + // while it was out of the listing. A name in _lookups always names an allocated slot. + set_listed(it->second, true); + return it->second; } @@ -190,9 +194,61 @@ Metrics::Storage::type(IdType id) const return _extractType(id); } +bool +Metrics::Storage::set_listed(Metrics::IdType id, bool listed) +{ + if (!_is_allocated(id)) { + return false; + } + + auto [blob_ix, offset] = _splitID(id); + Metrics::NamesAndAtomics *blob = _blobs[blob_ix].get(); + + // Only this bit, so a flag added later is not clobbered by unlisting or relisting. + if (listed) { + std::get<2>(*blob)[offset].fetch_and(static_cast(~UNLISTED), MEMORY_ORDER); + } else { + std::get<2>(*blob)[offset].fetch_or(UNLISTED, MEMORY_ORDER); + } + + return true; +} + +bool +Metrics::Storage::listed(Metrics::IdType id) const +{ + if (!_is_allocated(id)) { + return false; + } + + auto [blob_ix, offset] = _splitID(id); + Metrics::NamesAndAtomics *blob = _blobs[blob_ix].get(); + + return (std::get<2>(*blob)[offset].load(MEMORY_ORDER) & UNLISTED) == 0; +} + // Iterator implementation +Metrics::iterator::iterator(const Metrics &m) : _metrics(m), _it(0), _bound(m._storage->next_free_id()) +{ + skip_unlisted(); +} + +Metrics::iterator::iterator(const Metrics &m, IdType pos) : _metrics(m), _bound(m._storage->next_free_id()) +{ + // A metric id carries its type at METRIC_TYPE_BITS, but positions are compared numerically + // against a bound with no type bits. Keep only the blob and offset, as advance() does, or a GAUGE + // id would compare past the end of the store and the iterator would look exhausted. + auto [blob, offset] = _metrics._splitID(pos); + + _it = _makeId(blob, offset, MetricType::COUNTER); + + skip_unlisted(); +} + +Metrics::iterator::iterator(const Metrics &m, end_tag) : _metrics(m), _end(true) {} + void -Metrics::iterator::next() +Metrics::iterator::advance() { auto [blob, offset] = _metrics._splitID(_it); @@ -204,6 +260,23 @@ Metrics::iterator::next() _it = _makeId(blob, offset, MetricType::COUNTER); } +void +Metrics::iterator::skip_unlisted() +{ + // Bounded by the snapshot so a slot created and unlisted after this iterator was made cannot draw + // the scan past the end of what this iterator agreed to visit. + while (!at_end() && !_metrics._storage->listed(_it)) { + advance(); + } +} + +void +Metrics::iterator::next() +{ + advance(); + skip_unlisted(); +} + namespace details { struct DerivedMetric { diff --git a/src/tsutil/unit_tests/test_Metrics.cc b/src/tsutil/unit_tests/test_Metrics.cc index f267097807f..9751c119215 100644 --- a/src/tsutil/unit_tests/test_Metrics.cc +++ b/src/tsutil/unit_tests/test_Metrics.cc @@ -707,3 +707,278 @@ TEST_CASE("Metrics id lookup is safe against concurrent creation", "[libtsapi][M // would mean the sweep above never left the first one. REQUIRE(hi - lo > Metrics::MAX_SIZE); } + +TEST_CASE("Metrics unlisting", "[libtsapi][Metrics]") +{ + auto &m = Metrics::instance(); + + SECTION("an unlisted metric is skipped by iteration") + { + Metrics::Counter::create("unlisted.iter.before"); + auto target = Metrics::Counter::create("unlisted.iter.target"); + Metrics::Counter::create("unlisted.iter.after"); + + REQUIRE(m.unlist(target)); + + bool saw_before = false, saw_target = false, saw_after = false; + + for (auto &&[name, type, value] : m) { + saw_before |= (name == "unlisted.iter.before"); + saw_target |= (name == "unlisted.iter.target"); + saw_after |= (name == "unlisted.iter.after"); + } + + REQUIRE(saw_before); + REQUIRE_FALSE(saw_target); + REQUIRE(saw_after); + } + + SECTION("creating an unlisted name again relists it") + { + auto p = Metrics::Counter::createPtr("unlisted.resurrect"); + auto id = m.lookup("unlisted.resurrect"); + + Metrics::Counter::increment(p, 5); + REQUIRE(m.unlist(id)); + REQUIRE_FALSE(m.listed(id)); + + // Same name, same id, same atomic, and the mark is gone. + auto p2 = Metrics::Counter::createPtr("unlisted.resurrect"); + REQUIRE(p2 == p); + REQUIRE(m.lookup("unlisted.resurrect") == id); + REQUIRE(m.listed(id)); + + // Visible again, with its value intact. + bool found = false; + for (auto &&[name, type, value] : m) { + if (name == "unlisted.resurrect") { + found = true; + REQUIRE(value == 5); + } + } + REQUIRE(found); + } + + SECTION("unlist and relist by name") + { + auto id = Metrics::Counter::create("unlisted.byname"); + + REQUIRE(m.unlist("unlisted.byname")); + REQUIRE_FALSE(m.listed(id)); + + REQUIRE(m.relist("unlisted.byname")); + REQUIRE(m.listed(id)); + + bool found = false; + for (auto &&[name, type, value] : m) { + found |= (name == "unlisted.byname"); + } + REQUIRE(found); + + // A name that was never created cannot be marked. + REQUIRE_FALSE(m.unlist("unlisted.byname.never.created")); + } + + SECTION("an unlisted metric is still resolvable and still counts") + { + auto p = Metrics::Counter::createPtr("unlisted.resolvable"); + auto id = m.lookup("unlisted.resolvable"); + + REQUIRE(m.unlist(id)); + + // Hidden from enumeration is not gone: by name, by id, and through the atomic it is unchanged. + REQUIRE(m.lookup("unlisted.resolvable") == id); + REQUIRE(m.lookup(id) == p); + REQUIRE(m.valid(id)); + REQUIRE(m.name(id) == "unlisted.resolvable"); + REQUIRE(m.type(id) == Metrics::MetricType::COUNTER); + + Metrics::Counter::increment(p, 3); + REQUIRE(Metrics::Counter::load(p) == 3); + } + + SECTION("begin() skips an unlisted first slot") + { + // Slot 0 is the reserved bad_id and is what begin() would otherwise return. + auto bad_id = m.lookup("proxy.process.api.metrics.bad_id"); + REQUIRE(bad_id == 0); + + REQUIRE(m.unlist(bad_id)); + REQUIRE(std::get<0>(*m.begin()) != "proxy.process.api.metrics.bad_id"); + + REQUIRE(m.relist(bad_id)); + REQUIRE(std::get<0>(*m.begin()) == "proxy.process.api.metrics.bad_id"); + } + + SECTION("an unlisted run at the end of the store terminates iteration") + { + // Skipping the last slots in the store is the case where the skip loop has nothing unmarked + // left to land on. The anchor is a listed metric of this section's own, so the loop below is + // known to have run without depending on what other sections left in the shared store. + constexpr int COUNT = 8; + std::vector names; + + Metrics::Counter::create("unlisted.tail.anchor"); + + names.reserve(COUNT); + for (int i = 0; i < COUNT; ++i) { + names.push_back("unlisted.tail." + std::to_string(i)); + REQUIRE(m.unlist(Metrics::Counter::create(names[i]))); + } + + bool saw_anchor = false; + + for (auto &&[name, type, value] : m) { + saw_anchor |= (name == "unlisted.tail.anchor"); + for (auto const &n : names) { + REQUIRE(name != n); + } + } + + REQUIRE(saw_anchor); + } + + SECTION("iterator comparison") + { + auto a = m.begin(); + auto b = m.begin(); + auto e = m.end(); + + REQUIRE(a == b); + + ++a; + REQUIRE(a != b); // two live iterators still compare by position + + while (a != e) { + ++a; + } + REQUIRE(a == e); // exhausted equals the sentinel + + while (b != e) { + ++b; + } + REQUIRE(b == a); // and equals another exhausted iterator + } + + SECTION("iterating to a bound that is not end()") + { + // A sub-range delimited by a positional iterator has to terminate even when marked slots fall + // inside it. Both ends skip by the same rule, so the walk still lands exactly on the bound. + auto first = Metrics::Counter::create("unlisted.range.1"); + auto skip1 = Metrics::Counter::create("unlisted.range.2"); + auto skip2 = Metrics::Counter::create("unlisted.range.3"); + Metrics::Counter::create("unlisted.range.4"); + Metrics::Counter::create("unlisted.range.5"); + + REQUIRE(m.unlist(skip1)); + REQUIRE(m.unlist(skip2)); + + auto stop = m.find("unlisted.range.5"); + REQUIRE(stop != m.end()); + + std::vector seen; + + for (auto it = m.find("unlisted.range.1"); it != stop; ++it) { + seen.push_back(std::string(std::get<0>(*it))); + REQUIRE(seen.size() <= 4); // do not spin if the bound is never reached + } + + REQUIRE(seen == std::vector{"unlisted.range.1", "unlisted.range.4"}); + REQUIRE(first != Metrics::NOT_FOUND); + } + + SECTION("a subrange from iterators made at different times terminates") + { + // Each iterator snapshots its own bound at construction. If exhaustion is judged against each + // one's own bound, the walk can pass its own end while the stop iterator, made later and so + // holding a larger bound, is still live -- they never compare equal and ++ makes no progress. + Metrics::Counter::create("unlisted.snap.start"); + + auto start = m.find("unlisted.snap.start"); + REQUIRE(start != m.end()); + + Metrics::Counter::create("unlisted.snap.stop"); + + auto stop = m.find("unlisted.snap.stop"); + REQUIRE(stop != m.end()); + + int steps = 0; + + for (auto it = start; it != stop; ++it) { + REQUIRE(++steps < 64); // fails rather than spinning if the two never meet + } + } + + SECTION("find() works for a gauge, whose id carries type bits") + { + // A metric id encodes its type at METRIC_TYPE_BITS, while the iteration bound is built with + // COUNTER type bits. Comparing a GAUGE id against that bound numerically makes it look past + // the end of the store. + Metrics::Gauge::createPtr("unlisted.typed.gauge"); + Metrics::Counter::createPtr("unlisted.typed.counter"); + + auto g = m.find("unlisted.typed.gauge"); + REQUIRE(g != m.end()); + REQUIRE(std::get<0>(*g) == "unlisted.typed.gauge"); + REQUIRE(std::get<1>(*g) == Metrics::MetricType::GAUGE); + + auto c = m.find("unlisted.typed.counter"); + REQUIRE(c != m.end()); + REQUIRE(std::get<0>(*c) == "unlisted.typed.counter"); + } + + SECTION("find() on an unlisted metric yields end()") + { + // Iteration never visits a marked slot, so there must be no way to get an iterator that points + // at one. Otherwise using it as a range bound is a walk that never terminates: the skipping + // iterator steps straight over the bound and runs off the end of the store. + auto id = Metrics::Counter::create("unlisted.unfindable"); + + REQUIRE(m.find("unlisted.unfindable") != m.end()); + REQUIRE(m.unlist(id)); + REQUIRE(m.find("unlisted.unfindable") == m.end()); + + // lookup() is the supported way to reach a unlisted metric, and is unaffected. + REQUIRE(m.lookup("unlisted.unfindable") == id); + } + + SECTION("an id that names no allocated slot is neither listed nor unlistable") + { + // Storage::_is_allocated is the gate; this only checks that unlist and listed go through it. + // Blob 100 was never allocated, the largest id names an offset past MAX_SIZE, and create() + // advances after writing so the id one past the last one created is not allocated yet. + auto last = Metrics::Counter::create("unlisted.next.free"); + + for (auto id : {Metrics::IdType{100 << 16}, std::numeric_limits::max(), last + 1}) { + CHECK_FALSE(m.unlist(id)); + CHECK_FALSE(m.listed(id)); + } + } + + SECTION("the hidden store unlists independently") + { + auto &h = Metrics::hidden_instance(); + + Metrics::Counter::createPtr("unlisted.dual"); + Metrics::Counter::createHiddenPtr("unlisted.dual"); + + auto pub_id = m.lookup("unlisted.dual"); + auto hid_id = h.lookup("unlisted.dual"); + + REQUIRE(h.unlist(hid_id)); + REQUIRE_FALSE(h.listed(hid_id)); + REQUIRE(m.listed(pub_id)); + + bool in_published = false, in_hidden = false; + + for (auto &&[name, type, value] : m) { + in_published |= (name == "unlisted.dual"); + } + for (auto &&[name, type, value] : h) { + in_hidden |= (name == "unlisted.dual"); + } + + REQUIRE(in_published); + REQUIRE_FALSE(in_hidden); + } +} 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..ea5fc51183e 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,8 +238,8 @@ 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( @@ -294,7 +299,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 +345,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 +386,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 +415,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 +525,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 +592,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()