diff --git a/doc/admin-guide/files/records.yaml.en.rst b/doc/admin-guide/files/records.yaml.en.rst index a34387bb4d3..4ba1e11e31f 100644 --- a/doc/admin-guide/files/records.yaml.en.rst +++ b/doc/admin-guide/files/records.yaml.en.rst @@ -2007,6 +2007,35 @@ Origin Server Connect Attempts the connection. Useful when the origin supports keep-alive, removing the time needed to set up a new connection from the next request at the expense of added (inactive) connections. +.. ts:cv:: CONFIG proxy.config.http.per_server.connection.metric_enabled INT 0 + :reloadable: + + Publish per upstream server connection metrics. These metrics are dynamically named, one set per + upstream server group or hostname, so the number of them scales with the number of distinct + upstream servers seen. See :ref:`per-server-connection-metrics`. + + ===== ====================================================================================== + Value Effect + ===== ====================================================================================== + ``0`` No per server connection metrics. + ``1`` Publish only the per hostname aggregate metrics. The per group metrics from which the + aggregates are computed exist internally but are not published. + ``2`` Publish the per hostname aggregates and the per group metrics. + ===== ====================================================================================== + + Level ``2`` can produce a very large number of metrics when the + :ts:cv:`match type ` includes the address or + port, since there is then one set per address and port rather than one per hostname. + +.. ts:cv:: CONFIG proxy.config.http.per_server.connection.metric_prefix STRING NULL + :reloadable: + + An optional prefix inserted into the per server connection metric names, between the fixed + ``proxy.process.http.per_server..`` portion of the name and the upstream server group + or hostname. Useful to distinguish metrics from separate + :ts:cv:`match ` configurations sharing the same + upstream. See :ref:`per-server-connection-metrics`. + .. ts:cv:: CONFIG proxy.config.http.connect_attempts_rr_retries INT 3 :reloadable: :overridable: 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 7b3b23940f0..78a75e11f8f 100644 --- a/doc/admin-guide/monitoring/statistics/core/http-connection.en.rst +++ b/doc/admin-guide/monitoring/statistics/core/http-connection.en.rst @@ -201,6 +201,64 @@ HTTP Connection Current number of TCP connections for tunnels where the far end is the server, except for those counted by ``proxy.process.tunnel.current_server_connections_tls`` +.. _per-server-connection-metrics: + +Per Server Connection Metrics +----------------------------- + +Unlike the metrics above these do not have fixed names. They are created dynamically, one set per +upstream server group as defined by :ts:cv:`proxy.config.http.per_server.connection.match`, and, +when the match type is ``both``, one aggregate set per hostname. Whether any of them are published, +and at what granularity, is controlled by +:ts:cv:`proxy.config.http.per_server.connection.metric_enabled`. An optional +:ts:cv:`proxy.config.http.per_server.connection.metric_prefix` can be inserted into the names. + +Per group names are ``proxy.process.http.per_server..``, where ```` depends on +the match type: an IP address, an ``address:port`` pair, a hostname, or, for ``both``, +``.``. Per hostname names are +``proxy.process.http.per_server..``. Aggregates exist only for match type +``both``, because that is the only match type with more than one group per hostname; for match type +``host`` the group name is already the bare hostname, so an aggregate would carry the same name as +the single group it summarises. + +For a group, ```` is one of: + +current_connection + Gauge. The number of connections currently open to the group. + +total_connection + Counter. The total number of connections ever opened to the group. Never decreases. + +blocked_connection + Counter. The total number of connection attempts to the group blocked by + :ts:cv:`proxy.config.http.per_server.connection.max`. Never decreases. + +For a hostname aggregate, ```` is one of those three, each summed across the groups of that +hostname, plus: + +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. + +Every published per server metric is recomputed periodically, currently every 5 seconds, rather than +on every connection event, so a reader sees a value up to that interval old. This is true of the +hostname aggregates and, at level ``2``, of the published per group metrics as well: 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 +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_enabled ` level ``1`` 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. + HTTP/2 ------ diff --git a/doc/appendices/command-line/traffic_ctl.en.rst b/doc/appendices/command-line/traffic_ctl.en.rst index 6d50dce4cac..6b454240929 100644 --- a/doc/appendices/command-line/traffic_ctl.en.rst +++ b/doc/appendices/command-line/traffic_ctl.en.rst @@ -949,13 +949,20 @@ traffic_ctl metric Display the current value of the specified statistics. .. program:: traffic_ctl metric -.. option:: match REGEX [REGEX...] +.. option:: match [--include-hidden] REGEX [REGEX...] :ref:`admin_lookup_records` Display the current values of all statistics whose names match the given regular expression. +.. option:: --include-hidden + + Also match hidden metrics. Hidden metrics are internal metrics that are stored but never + published through the normal metrics registry; they are not part of the stable metric + contract and may be added, changed, or removed between releases without notice. This + option is intended for debugging. + .. program:: traffic_ctl metric .. option:: describe RECORD [RECORD...] diff --git a/doc/developer-guide/internal-libraries/Metrics.en.rst b/doc/developer-guide/internal-libraries/Metrics.en.rst new file mode 100644 index 00000000000..4822170546e --- /dev/null +++ b/doc/developer-guide/internal-libraries/Metrics.en.rst @@ -0,0 +1,198 @@ +.. 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:: ../../common.defs + +Metrics +******* + +Synopsis +======== + +.. code-block:: cpp + + #include "tsutil/Metrics.h" + +``ts::Metrics`` is the metrics registry. A metric is a named ``int64_t`` counter or gauge, +reached either by an integer id or by a pointer to its underlying atomic. This page covers two +facilities layered on top of it: a separate store for metrics that should not be published, and +derived metrics that aggregate other metrics. + +Metric types +============ + +Every metric has a ``ts::Metrics::MetricType``, either ``COUNTER`` (monotonically increasing) +or ``GAUGE`` (rises and falls). The type is chosen by the facade used to create the metric, +``ts::Metrics::Counter`` or ``ts::Metrics::Gauge``, and is encoded into the metric id. + +.. code-block:: cpp + + auto *hits = ts::Metrics::Counter::createPtr("proxy.process.example.hits"); + auto *live = ts::Metrics::Gauge::createPtr("proxy.process.example.live"); + + ts::Metrics::Counter::increment(hits); + ts::Metrics::Gauge::store(live, 5); + +The two stores +============== + +There are two entirely separate stores: + +``ts::Metrics::instance()`` + The published store. Everything here is visible to :program:`traffic_ctl`, the JSONRPC API and + ``stats_over_http``. + +``ts::Metrics::hidden_instance()`` + The hidden store. Metrics here are recorded normally but are never published. + +Hidden metrics exist for high cardinality intermediate values, where the individual values are not +useful to publish but an aggregate over them is. A separate store is used rather than a +"hidden" flag on each metric so that hidden metrics are *structurally* unreachable from the +published store: no consumer can expose one by forgetting to check a flag. + +Create a hidden metric with ``createHiddenPtr`` on either facade: + +.. code-block:: cpp + + auto *g = ts::Metrics::Gauge::createHiddenPtr("proxy.process.example.per_thing.", thing_name); + + // The ordinary typed mutators work unchanged on a hidden metric. + ts::Metrics::Gauge::increment(g); + ts::Metrics::Gauge::decrement(g); + +``createHiddenPtr`` returns the same correctly typed pointer as ``createPtr``, so a hidden metric is +read and written with the normal mutators and no cast is needed at the call site. There are two +overloads on each facade, one taking a name and one taking a prefix and a name. + +.. important:: + + An id from one store is meaningless in the other. Both stores number their metrics from zero, so + passing a hidden id to the published store silently reads a different metric, with no error and + no crash. Prefer ``createHiddenPtr``, which returns a pointer and never hands out an id. + +Inspecting hidden metrics +------------------------- + +Because hidden metrics are invisible to normal queries, they can be listed explicitly with +``traffic_ctl metric match --include-hidden``. This sets an additional record type bit which +is deliberately outside ``RECT_ALL``, so hidden metrics are returned only when asked for by name and +never as a side effect of a broad query. + +.. note:: + + Hidden metrics are internal. They are not part of the stable metric contract and may be added, + renamed or removed between releases without notice. Do not build monitoring on them; use the + published aggregate instead. + +Derived metrics +=============== + +A derived metric is a published metric whose value is computed from other metrics, its *sources*. A +source may live in either store, which is the point of the facility: high cardinality sources stay +hidden while only the aggregate is published. + +Sources are combined with one of three operations, ``ts::Metrics::Derived::Op``: + +``SUM`` + Add the sources together. This is the default. + +``MAX`` + The largest source value. + +``MIN`` + The smallest source value. + +Declaring aggregates up front +----------------------------- + +``ts::Metrics::Derived::derive()`` takes a list of specifications and is meant for aggregates whose +sources are all known at startup. Each source may be given as a pointer, an id or a name: + +.. code-block:: cpp + + ts::Metrics::Derived::derive({ + {"proxy.process.example.total", ts::Metrics::MetricType::COUNTER, {a, b, c}}, + {"proxy.process.example.peak", ts::Metrics::MetricType::GAUGE, {a, b, c}, + ts::Metrics::Derived::Op::MAX}, + }); + +A source that does not resolve, because the name or id is unknown, is skipped. + +Building aggregates at runtime +------------------------------ + +``ts::Metrics::Derived::add_source()`` adds a single source to a derived metric, creating the +derived metric if it does not exist yet. Use it when sources are discovered as the process runs, for +example one per upstream server as traffic arrives: + +.. code-block:: cpp + + ts::Metrics::Derived::add_source("proxy.process.example.total", ts::Metrics::MetricType::COUNTER, + per_thing_metric); + +Repeatedly calling ``ts::Metrics::Derived::derive()`` for the same derived name does **not** work +for this: each call appends a separate entry targeting the same metric, so every update overwrites +the others with its own subset of sources and the last one to run silently wins. +``ts::Metrics::Derived::add_source()`` accumulates into a single entry instead. + +Adding a source that is already registered for that derived metric is a no-op, so a caller which may +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. + +A hidden source can feed a published aggregate: + +.. code-block:: cpp + + auto *hidden = ts::Metrics::Gauge::createHiddenPtr("per_thing.", name); + + ts::Metrics::Derived::add_source("proxy.process.example.live", ts::Metrics::MetricType::GAUGE, + hidden, ts::Metrics::Derived::Op::SUM); + +When derived values update +-------------------------- + +Derived metrics are not recomputed when a source changes. They are recalculated by +``ts::Metrics::Derived::update_derived()``, which runs on an ``ET_TASK`` thread every +``REC_RAW_STAT_SYNC_INTERVAL_MS``, currently 5000 ms. Consequences: + +* A derived value lags its sources by up to one interval. +* Reading a derived metric immediately after changing a source returns the previous value. Unit + tests must call ``ts::Metrics::Derived::update_derived()`` directly. +* The cost of the pass is proportional to the total number of registered sources, and it runs + single threaded while holding a lock. Registering very large numbers of sources is therefore not + free, even though registration itself is rare. + +Because the pass samples its sources, a derived ``MAX`` reports the largest value *observed at a +sampling point*, not the true peak. There are two ways to arrange this, with different tradeoffs: + +* A ``MAX`` over instantaneous gauges is sampled, so a brief spike occurring between two samples is + not observed. The value rises and falls with the sources, so a monitoring system that scrapes it + can compute a maximum over any time window. +* A ``MAX`` over monotonically increasing sources, such as each source's own all-time peak, is exact + and never misses a spike. It also never decreases, so the time dimension is lost: the value + reports only that a peak occurred at some point, not when. + +Which is appropriate depends on whether the consumer needs to aggregate over time downstream. + +Storage limits +============== + +Metrics are allocated from fixed size blobs, ``MAX_BLOBS`` of ``MAX_SIZE`` entries each, for a +maximum of about 8M metrics per store. Creating a metric when the store is full returns the reserved +``bad_id`` rather than growing past the end, so an exhausted store degrades to writing into a +throwaway slot instead of corrupting memory. Reaching this limit means the naming scheme is +unbounded, and hidden metrics with per-connection or per-URL names are the likely cause. diff --git a/doc/developer-guide/internal-libraries/index.en.rst b/doc/developer-guide/internal-libraries/index.en.rst index 0dc9820afdd..6c1574e4efd 100644 --- a/doc/developer-guide/internal-libraries/index.en.rst +++ b/doc/developer-guide/internal-libraries/index.en.rst @@ -31,6 +31,7 @@ development team. ArgParser.en MemArena.en MemSpan.en + Metrics.en TextView.en buffer-writer.en scalar.en diff --git a/include/iocore/net/ConnectionTracker.h b/include/iocore/net/ConnectionTracker.h index ae3691fdbe2..5041c278274 100644 --- a/include/iocore/net/ConnectionTracker.h +++ b/include/iocore/net/ConnectionTracker.h @@ -82,16 +82,36 @@ class ConnectionTracker MatchType server_match{MATCH_IP}; ///< Server match type. }; + /** Levels for the @c metric_enabled configuration. + * + * The per group metrics are always created, in the hidden metric store, whenever metrics are + * enabled at all. What varies by level is what gets published: + * - @c METRIC_LEVEL_NONE: no per group metrics are created and nothing is published. + * - @c METRIC_LEVEL_HOST: the per group metrics stay hidden; the per hostname aggregates + * (computed across the groups of a hostname by a @c Derived metric) are published. + * - @c METRIC_LEVEL_GROUP: as above, plus the per group metrics are also mirrored into the + * published store. + * + * Keeping the per group metrics in the hidden store at every level means changing the level at + * runtime is only a change of what is registered for publication, with no metric to migrate + * between the two stores. + */ + enum MetricLevel { + METRIC_LEVEL_NONE = 0, ///< No per server metrics. + METRIC_LEVEL_HOST = 1, ///< Only the per hostname aggregate metrics are published. + METRIC_LEVEL_GROUP = 2, ///< The per hostname aggregates and the per group metrics are published. + }; + /** Static configuration values. */ struct GlobalConfig { GlobalConfig() = default; GlobalConfig(GlobalConfig const &); GlobalConfig &operator=(GlobalConfig const &); - std::chrono::seconds client_alert_delay{60}; ///< Alert delay in seconds. - std::chrono::seconds server_alert_delay{60}; ///< Alert delay in seconds. - bool metric_enabled{false}; ///< Enabling per server metrics. - std::string metric_prefix; ///< Per server metric prefix. + std::chrono::seconds client_alert_delay{60}; ///< Alert delay in seconds. + std::chrono::seconds server_alert_delay{60}; ///< Alert delay in seconds. + MetricLevel metric_enabled{METRIC_LEVEL_NONE}; ///< Which per server metrics to publish. + std::string metric_prefix; ///< Per server metric prefix. swoc::IPRangeSet client_exempt_list; ///< The set of IP addresses to not block due client connection counting. mutable ts::bravo::shared_mutex client_exempt_list_mutex; ///< Protects client_exempt_list from concurrent access. }; @@ -145,7 +165,8 @@ class ConnectionTracker std::atomic _in_queue{0}; ///< # of connections queued, waiting for a connection. std::atomic _last_alert{0}; ///< Absolute time of the last alert. - // Recording data as metrics + // Recording data as metrics. These are always in the hidden metric store when created; see + // @c MetricLevel for how they are published. ts::Metrics::Gauge::AtomicType *_count_metric = nullptr; ts::Metrics::Counter::AtomicType *_count_total_metric = nullptr; ts::Metrics::Counter::AtomicType *_blocked_metric = nullptr; @@ -171,6 +192,20 @@ class ConnectionTracker std::time_t get_last_alert_epoch_time() const; static std::string metric_name(const Key &key, std::string_view fqdn, std::string metric_prefix); + /** Name of the metric which aggregates a value across all groups of a hostname. + * + * Only @c MATCH_BOTH groups have more than one group per hostname. For @c MATCH_HOST there is + * exactly one group per hostname, so an aggregate would be over a set of one, and + * @c Group::metric_name already returns the FQDN alone for that match type - identical to what + * this would return, so publishing both would collide on one name. + * + * @param key The group key. + * @param fqdn The full FQDN. + * @param metric_prefix The configured metric prefix. + * @return The metric name, or an empty string if @a key is not @c MATCH_BOTH. + */ + static std::string host_metric_name(const Key &key, std::string_view fqdn, std::string metric_prefix); + /// Release the reference count to this group and remove it from the /// group table if it is no longer referenced. void release(); @@ -433,6 +468,15 @@ ConnectionTracker::Group::metric_name(const Key &key, std::string_view fqdn, std return metric_prefix.empty() ? std::move(metric_name) : metric_prefix + "." + metric_name; } +inline std::string +ConnectionTracker::Group::host_metric_name(const Key &key, std::string_view fqdn, std::string metric_prefix) +{ + if (MATCH_BOTH != key._match_type) { + return {}; // Only MATCH_BOTH has more than one group per hostname to aggregate across. + } + return metric_prefix.empty() ? std::string(fqdn) : metric_prefix + "." + std::string(fqdn); +} + inline bool ConnectionTracker::TxnState::is_active() const { @@ -489,9 +533,13 @@ ConnectionTracker::TxnState::clear() inline void ConnectionTracker::TxnState::update_max_count(int count) { - auto cmax = _g->_count_max.load(); - if (count > cmax) { - _g->_count_max.compare_exchange_weak(cmax, count); + auto cmax = _g->_count_max.load(std::memory_order_relaxed); + + while (count > cmax) { + if (_g->_count_max.compare_exchange_weak(cmax, count, std::memory_order_relaxed, std::memory_order_relaxed)) { + break; + } + // cmax was reloaded by the failed exchange; retry if we are still larger. } } diff --git a/include/records/RecDefs.h b/include/records/RecDefs.h index 8befda0d895..2935f528f72 100644 --- a/include/records/RecDefs.h +++ b/include/records/RecDefs.h @@ -57,7 +57,10 @@ enum RecT { RECT_NODE = 0x04, RECT_LOCAL = 0x10, RECT_PLUGIN = 0x20, - RECT_ALL = 0x3F + RECT_ALL = 0x3F, + /// Hidden metrics. Deliberately outside RECT_ALL so they are only ever returned when + /// explicitly requested. See ts::Metrics::hidden_instance(). + RECT_HIDDEN_METRIC = 0x40 }; enum RecDataT { diff --git a/include/shared/rpc/RPCRequests.h b/include/shared/rpc/RPCRequests.h index 0a047d529dd..b7c58793583 100644 --- a/include/shared/rpc/RPCRequests.h +++ b/include/shared/rpc/RPCRequests.h @@ -117,6 +117,8 @@ struct ClientRequestNotification : JSONRPCRequest { // handy definitions. static const std::vector CONFIG_REC_TYPES = {1, 16}; static const std::vector METRIC_REC_TYPES = {2, 4, 32}; +// Same as METRIC_REC_TYPES, plus 64 (RECT_HIDDEN_METRIC, see RecDefs.h) to also match hidden metrics. +static const std::vector METRIC_REC_TYPES_INCLUDE_HIDDEN = {2, 4, 32, 64}; static constexpr bool NOT_REGEX{false}; static constexpr bool REGEX{true}; diff --git a/include/tsutil/Metrics.h b/include/tsutil/Metrics.h index 4f47fd5485f..a1d4d2fe2b6 100644 --- a/include/tsutil/Metrics.h +++ b/include/tsutil/Metrics.h @@ -115,6 +115,22 @@ class Metrics // The singleton instance, owned by the Metrics class static Metrics &instance(); + /** The hidden metrics instance. + * + * A completely separate storage from @c instance(). Metrics here are stored but never + * published - they are structurally unreachable from the published store, so no consumer + * (traffic_ctl, JSONRPC, stats_over_http) can expose them by omission. + * + * Intended for high cardinality intermediate values which feed @c Derived aggregates. + * + * @note An @c IdType from this instance is NOT interchangeable with one from @c instance(). + * Ids are meaningful only relative to their store: passing a hidden id to the published + * store yields a silently wrong metric, with no error and no crash, since @c valid() will + * accept it. Prefer @c Gauge::createHiddenPtr / @c Counter::createHiddenPtr, which return + * correctly typed pointers and never hand out an id. + */ + static Metrics &hidden_instance(); + // Yes, we don't return objects here, but rather ID's and atomic's directly. Treat // the std::atomic as the underlying class for a single metric, and be happy. IdType @@ -417,6 +433,27 @@ class Metrics return reinterpret_cast(instance.lookup(instance._create(tmpname, MetricType::GAUGE))); } + /** Create a metric which is stored but never published. + * + * @see Metrics::hidden_instance() + */ + static AtomicType * + createHiddenPtr(const std::string_view name) + { + auto &instance = Metrics::hidden_instance(); + + return reinterpret_cast(instance.lookup(instance._create(name, MetricType::GAUGE))); + } + + static AtomicType * + createHiddenPtr(const std::string_view prefix, const std::string_view name) + { + auto &instance = Metrics::hidden_instance(); + std::string tmpname = std::string(prefix) + std::string(name); + + return reinterpret_cast(instance.lookup(instance._create(tmpname, MetricType::GAUGE))); + } + static Metrics::Gauge::SpanType createSpan(size_t size, IdType *id = nullptr) { @@ -514,6 +551,27 @@ class Metrics return reinterpret_cast(instance.lookup(instance._create(tmpname, MetricType::COUNTER))); } + /** Create a metric which is stored but never published. + * + * @see Metrics::hidden_instance() + */ + static AtomicType * + createHiddenPtr(const std::string_view name) + { + auto &instance = Metrics::hidden_instance(); + + return reinterpret_cast(instance.lookup(instance._create(name, MetricType::COUNTER))); + } + + static AtomicType * + createHiddenPtr(const std::string_view prefix, const std::string_view name) + { + auto &instance = Metrics::hidden_instance(); + std::string tmpname = std::string(prefix) + std::string(name); + + return reinterpret_cast(instance.lookup(instance._create(tmpname, MetricType::COUNTER))); + } + static Metrics::Counter::SpanType createSpan(size_t size, IdType *id = nullptr) { @@ -587,11 +645,15 @@ class Metrics class Derived { public: + /// How the sources of a derived metric are combined into its value. + enum class Op { SUM, MAX, MIN }; + struct DerivedMetricSpec { using MetricSpec = std::variant; std::string_view derived_name; Metrics::MetricType derived_type; std::initializer_list derived_from; + Op op{Op::SUM}; }; /** @@ -602,6 +664,21 @@ class Metrics */ static void derive(const std::initializer_list &metrics); + /** Add a source to a derived metric, creating the derived metric if needed. + * + * Unlike @c derive this may be called at any time, so aggregates can be built up as their + * sources are discovered at runtime. + * + * @param derived_name Name of the derived metric, in the published store. + * @param type Type of the derived metric. Ignored if the derived metric already exists. + * @param source The source metric. May come from either the published or the hidden store. + * @param op How to combine the sources. Ignored if the derived metric already exists. + * + * Adding a source which is already registered for @a derived_name is a no-op, so callers + * which may re-register (e.g. an object recreated for the same key) need not track this. + */ + static void add_source(std::string_view derived_name, Metrics::MetricType type, Metrics::AtomicType *source, Op op = Op::SUM); + /** * Update derived metrics. * diff --git a/src/iocore/net/ConnectionTracker.cc b/src/iocore/net/ConnectionTracker.cc index 45ce7e60f07..e8d4fc1eee5 100644 --- a/src/iocore/net/ConnectionTracker.cc +++ b/src/iocore/net/ConnectionTracker.cc @@ -26,6 +26,8 @@ #include "records/RecCore.h" #include "swoc/IPAddr.h" +#include + using namespace std::literals; ConnectionTracker::TableSingleton ConnectionTracker::_inbound_table; @@ -156,7 +158,9 @@ Config_Update_Conntrack_Metric_Enabled(const char * /* name ATS_UNUSED */, RecDa auto config = static_cast(cookie); if (RECD_INT == dtype) { - config->metric_enabled = data.rec_int; + auto level = std::clamp(static_cast(data.rec_int), static_cast(ConnectionTracker::METRIC_LEVEL_NONE), + static_cast(ConnectionTracker::METRIC_LEVEL_GROUP)); + config->metric_enabled = static_cast(level); return true; } return false; @@ -440,11 +444,40 @@ ConnectionTracker::Group::Group(DirectionType direction, Key const &key, std::st { Metrics::Gauge::increment(net_rsb.connection_tracker_table_size); // only add metrics for server connections - if (_global_config->metric_enabled && direction == DirectionType::OUTBOUND) { + if (_global_config->metric_enabled != METRIC_LEVEL_NONE && direction == DirectionType::OUTBOUND) { std::string _metric_name = metric_name(key, fqdn, _global_config->metric_prefix); - _count_metric = Metrics::Gauge::createPtr("proxy.process.http.per_server.current_connection.", _metric_name); - _count_total_metric = Metrics::Counter::createPtr("proxy.process.http.per_server.total_connection.", _metric_name); - _blocked_metric = Metrics::Counter::createPtr("proxy.process.http.per_server.blocked_connection.", _metric_name); + // Per group metrics always live in the hidden store. metric_enabled controls what is published + // from them (see MetricLevel), not whether they exist. + _count_metric = Metrics::Gauge::createHiddenPtr("proxy.process.http.per_server.current_connection.", _metric_name); + _count_total_metric = Metrics::Counter::createHiddenPtr("proxy.process.http.per_server.total_connection.", _metric_name); + _blocked_metric = Metrics::Counter::createHiddenPtr("proxy.process.http.per_server.blocked_connection.", _metric_name); + + // Only MATCH_BOTH groups have siblings sharing a hostname to aggregate across. + std::string _host_metric_name = host_metric_name(key, fqdn, _global_config->metric_prefix); + if (!_host_metric_name.empty()) { + Metrics::Derived::add_source("proxy.process.http.per_server.current_connection." + _host_metric_name, + Metrics::MetricType::GAUGE, _count_metric, Metrics::Derived::Op::SUM); + Metrics::Derived::add_source("proxy.process.http.per_server.total_connection." + _host_metric_name, + Metrics::MetricType::COUNTER, _count_total_metric, Metrics::Derived::Op::SUM); + Metrics::Derived::add_source("proxy.process.http.per_server.blocked_connection." + _host_metric_name, + Metrics::MetricType::COUNTER, _blocked_metric, Metrics::Derived::Op::SUM); + // The largest current count among this hostname's groups, sampled. Deliberately taken over + // the instantaneous gauge rather than each group's all time peak, so the value falls again + // and a maximum over time can be computed by whatever scrapes it. + Metrics::Derived::add_source("proxy.process.http.per_server.current_connection_max." + _host_metric_name, + Metrics::MetricType::GAUGE, _count_metric, Metrics::Derived::Op::MAX); + } + + if (_global_config->metric_enabled >= METRIC_LEVEL_GROUP) { + // Mirror the per group metrics into the published store under their own name. A single + // source SUM is an identity: the published value always equals the hidden source. + 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); + } if (dbg_ctl.on()) { swoc::LocalBufferWriter<256> w; diff --git a/src/mgmt/rpc/handlers/records/Records.cc b/src/mgmt/rpc/handlers/records/Records.cc index e2bb3f0cb27..2cbb1ae4d46 100644 --- a/src/mgmt/rpc/handlers/records/Records.cc +++ b/src/mgmt/rpc/handlers/records/Records.cc @@ -104,6 +104,7 @@ template <> struct convert { case RECT_LOCAL: case RECT_PLUGIN: case RECT_ALL: + case RECT_HIDDEN_METRIC: // Opt-in only, deliberately not part of RECT_ALL, see RecDefs.h. info.recTypes.push_back(rt); break; default: diff --git a/src/proxy/http/HttpSM.cc b/src/proxy/http/HttpSM.cc index 64173d0f3c9..d89ef892b4d 100644 --- a/src/proxy/http/HttpSM.cc +++ b/src/proxy/http/HttpSM.cc @@ -5866,7 +5866,9 @@ HttpSM::do_http_server_open(bool raw, bool only_direct) } else if (t_state.txn_conf->connection_tracker_config.server_min > 0 || t_state.http_config_param->global_connection_tracker_config.metric_enabled) { auto &ct_state = t_state.outbound_conn_track_state; - ct_state.reserve(); + // Feed the count through as well, otherwise the group's peak stays at zero whenever metrics + // are enabled without a configured maximum. + ct_state.update_max_count(ct_state.reserve()); } // We did not manage to get an existing session and need to open a new connection diff --git a/src/records/RecCore.cc b/src/records/RecCore.cc index c5daa449bf6..cd8f3146f40 100644 --- a/src/records/RecCore.cc +++ b/src/records/RecCore.cc @@ -612,6 +612,22 @@ RecLookupMatchingRecords(unsigned rec_type, const char *match, void (*callback)( }); } + if (rec_type & RECT_HIDDEN_METRIC) { + // Opt-in only: hidden metrics are never reachable through RECT_ALL, see RecDefs.h. + for (auto &&[name, type, val] : ts::Metrics::hidden_instance()) { + if (regex.exec(name.data())) { + RecRecord tmp; + + tmp.rec_type = RECT_PROCESS; + + tmp.name = name.data(); + tmp.data_type = type == ts::Metrics::MetricType::COUNTER ? RECD_COUNTER : RECD_INT; + tmp.data.rec_int = val; + callback(&tmp, data); + } + } + } + int num_records = g_num_records; for (int i = 0; i < num_records; i++) { RecRecord *r = &(g_records[i]); diff --git a/src/records/RecordsConfig.cc b/src/records/RecordsConfig.cc index 3ad853798e4..c52253a0a34 100644 --- a/src/records/RecordsConfig.cc +++ b/src/records/RecordsConfig.cc @@ -395,7 +395,7 @@ static constexpr RecordElement RecordsConfig[] = , {RECT_CONFIG, "proxy.config.http.per_server.connection.min", RECD_INT, "0", RECU_DYNAMIC, RR_NULL, RECC_STR, "^[0-9]+$", RECA_NULL} , - {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_enabled", RECD_INT, "0", RECU_DYNAMIC, RR_NULL, RECC_STR, "^[0-2]$", 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/traffic_ctl/CtrlCommands.cc b/src/traffic_ctl/CtrlCommands.cc index 514c5d17098..88d29637ece 100644 --- a/src/traffic_ctl/CtrlCommands.cc +++ b/src/traffic_ctl/CtrlCommands.cc @@ -224,12 +224,12 @@ ConfigCommand::ConfigCommand(ts::Arguments *args) : RecordCommand(args) } shared::rpc::JSONRPCResponse -RecordCommand::record_fetch(ts::ArgumentData argData, bool isRegex, RecordQueryType recQueryType) +RecordCommand::record_fetch(ts::ArgumentData argData, bool isRegex, RecordQueryType recQueryType, bool includeHidden) { shared::rpc::RecordLookupRequest request; + auto const &metricTypes = includeHidden ? shared::rpc::METRIC_REC_TYPES_INCLUDE_HIDDEN : shared::rpc::METRIC_REC_TYPES; for (auto &&it : argData) { - request.emplace_rec(it, isRegex, - recQueryType == RecordQueryType::CONFIG ? shared::rpc::CONFIG_REC_TYPES : shared::rpc::METRIC_REC_TYPES); + request.emplace_rec(it, isRegex, recQueryType == RecordQueryType::CONFIG ? shared::rpc::CONFIG_REC_TYPES : metricTypes); } return invoke_rpc(request); } @@ -784,7 +784,9 @@ MetricCommand::metric_get() void MetricCommand::metric_match() { - _printer->write_output(record_fetch(get_parsed_arguments()->get(MATCH_STR), shared::rpc::REGEX, RecordQueryType::METRIC)); + bool const include_hidden = get_parsed_arguments()->get(INCLUDE_HIDDEN_STR); + _printer->write_output( + record_fetch(get_parsed_arguments()->get(MATCH_STR), shared::rpc::REGEX, RecordQueryType::METRIC, include_hidden)); } void diff --git a/src/traffic_ctl/CtrlCommands.h b/src/traffic_ctl/CtrlCommands.h index 0958fb70697..4a022d15211 100644 --- a/src/traffic_ctl/CtrlCommands.h +++ b/src/traffic_ctl/CtrlCommands.h @@ -120,7 +120,9 @@ class RecordCommand : public CtrlCommand /// @param argData argument's data. /// @param isRegex if the request should be done by regex or name. /// @param recQueryType Config or Metric. - shared::rpc::JSONRPCResponse record_fetch(ts::ArgumentData argData, bool isRegex, RecordQueryType recQueryType); + /// @param includeHidden if true, also match hidden (internal, normally unpublished) metrics. Only meaningful for METRIC. + shared::rpc::JSONRPCResponse record_fetch(ts::ArgumentData argData, bool isRegex, RecordQueryType recQueryType, + bool includeHidden = false); }; // ----------------------------------------------------------------------------------------------------------------------------------- class ConfigCommand : public RecordCommand @@ -174,6 +176,7 @@ class CacheCommand : public CtrlCommand class MetricCommand : public RecordCommand { static inline const std::string MONITOR_STR{"monitor"}; + static inline const std::string INCLUDE_HIDDEN_STR{"include-hidden"}; void metric_get(); void metric_match(); diff --git a/src/traffic_ctl/traffic_ctl.cc b/src/traffic_ctl/traffic_ctl.cc index f39759e2f2f..a9d450c80a3 100644 --- a/src/traffic_ctl/traffic_ctl.cc +++ b/src/traffic_ctl/traffic_ctl.cc @@ -249,7 +249,9 @@ main([[maybe_unused]] int argc, const char **argv) .add_example_usage("traffic_ctl metric get METRIC [METRIC ...]"); metric_command.add_command("describe", "Show detailed information about one or more metric values", "", MORE_THAN_ONE_ARG_N, Command_Execute); // not implemented - metric_command.add_command("match", "Get metrics matching a regular expression", "", MORE_THAN_ZERO_ARG_N, Command_Execute); + metric_command.add_command("match", "Get metrics matching a regular expression", "", MORE_THAN_ZERO_ARG_N, Command_Execute) + .add_option("--include-hidden", "", "Also match hidden (internal, normally unpublished) metrics") + .add_example_usage("traffic_ctl metric match METRIC [--include-hidden]"); metric_command .add_command( "monitor", diff --git a/src/tsutil/Metrics.cc b/src/tsutil/Metrics.cc index 92bcb3bf6b0..6f078726b50 100644 --- a/src/tsutil/Metrics.cc +++ b/src/tsutil/Metrics.cc @@ -22,6 +22,7 @@ */ #include "tsutil/Assert.h" +#include #include #include #include @@ -42,13 +43,24 @@ Metrics::instance() return _instance; } +Metrics & +Metrics::hidden_instance() +{ + // Separate storage from instance(). Hidden metrics are never published. + static std::shared_ptr _hidden_store = std::make_shared(); + thread_local Metrics _instance(_hidden_store); + + return _instance; +} + void Metrics::Storage::addBlob() // The mutex must be held before calling this! { auto blob = std::make_unique(); debug_assert(blob); - debug_assert(_cur_blob < MAX_BLOBS); + // The write below is to _blobs[_cur_blob + 1], so the last usable blob index is MAX_BLOBS - 1. + release_assert(_cur_blob < MAX_BLOBS - 1); _blobs[++_cur_blob] = std::move(blob); _cur_off = 0; @@ -64,6 +76,13 @@ Metrics::Storage::create(std::string_view name, const MetricType type) return it->second; } + // The slot is written below and the bookkeeping only then advances, calling addBlob() once + // _cur_off reaches MAX_SIZE. Refusing the final slot of the final blob keeps addBlob() from + // ever being reached in an exhausted store, at a cost of one slot out of MAX_BLOBS * MAX_SIZE. + if (_cur_blob >= MAX_BLOBS - 1 && _cur_off >= MAX_SIZE - 1) { + return 0; // Slot 0 is the reserved bad_id. Cannot grow further. + } + Metrics::IdType id = _makeId(_cur_blob, _cur_off, type); Metrics::NamesAndAtomics *blob = _blobs[_cur_blob].get(); Metrics::NameStorage &names = std::get<0>(*blob); @@ -228,6 +247,7 @@ namespace details struct DerivedMetric { Metrics::IdType metric; std::vector derived_from; + Metrics::Derived::Op op{Metrics::Derived::Op::SUM}; }; struct DerivativeMetrics { @@ -241,12 +261,30 @@ namespace details std::lock_guard l(metrics_lock); for (auto &m : metrics) { - int64_t sum = 0; + if (m.derived_from.empty()) { + continue; + } - for (auto d : m.derived_from) { - sum += d->load(); + // Seeded from the first source rather than from zero: a zero seed is correct only for + // SUM, and would clamp every MIN result to <= 0. + int64_t value = m.derived_from.front()->load(); + + for (auto it = m.derived_from.begin() + 1; it != m.derived_from.end(); ++it) { + int64_t const v = (*it)->load(); + + switch (m.op) { + case Metrics::Derived::Op::SUM: + value += v; + break; + case Metrics::Derived::Op::MAX: + value = std::max(value, v); + break; + case Metrics::Derived::Op::MIN: + value = std::min(value, v); + break; + } } - instance[m.metric].store(sum); + instance[m.metric].store(value); } } @@ -257,6 +295,26 @@ namespace details metrics.push_back(std::move(m)); } + void + add_source(Metrics::IdType id, Metrics::AtomicType *source, Metrics::Derived::Op op) + { + if (!source) { + return; + } + + 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()) { + metrics.push_back(DerivedMetric{id, {source}, op}); + return; + } + // Already registered sources are skipped so repeated registration is harmless. + if (std::find(it->derived_from.begin(), it->derived_from.end(), source) == it->derived_from.end()) { + it->derived_from.push_back(source); + } + } + static DerivativeMetrics & instance() { @@ -275,14 +333,25 @@ Metrics::Derived::derive(const std::initializer_list(d)) { - dm.derived_from.push_back(std::get(d)); + ptr = std::get(d); } else if (std::holds_alternative(d)) { - dm.derived_from.push_back(instance.lookup(std::get(d))); - } else if (std::holds_alternative(d)) { - dm.derived_from.push_back(instance.lookup(instance.lookup(std::get(d)))); + auto id = std::get(d); + ptr = instance.valid(id) ? instance.lookup(id) : nullptr; + } else { + auto id = instance.lookup(std::get(d)); + ptr = (id != Metrics::NOT_FOUND) ? instance.lookup(id) : nullptr; + } + + // A source that does not resolve is skipped. Passing an unresolved id to lookup() would + // silently land on the reserved bad_id slot and contribute its value to the aggregate. + if (ptr) { + dm.derived_from.push_back(ptr); } } details::DerivativeMetrics::instance().push_back(dm); @@ -295,6 +364,15 @@ Metrics::Derived::update_derived() details::DerivativeMetrics::instance().update(); } +void +Metrics::Derived::add_source(std::string_view derived_name, Metrics::MetricType type, Metrics::AtomicType *source, Op op) +{ + // Resolved here rather than in the helper because _create is private to Metrics. + auto id = Metrics::instance()._create(derived_name, type); + + details::DerivativeMetrics::instance().add_source(id, source, op); +} + Metrics::StaticString & Metrics::StaticString::instance() { diff --git a/src/tsutil/unit_tests/test_Metrics.cc b/src/tsutil/unit_tests/test_Metrics.cc index cc30cb79768..7dc8b749641 100644 --- a/src/tsutil/unit_tests/test_Metrics.cc +++ b/src/tsutil/unit_tests/test_Metrics.cc @@ -23,6 +23,14 @@ #include +#include +#include +#include +#include +#include +#include +#include + #include "tsutil/Metrics.h" using ts::Metrics; @@ -38,18 +46,35 @@ TEST_CASE("Metrics", "[libtsapi][Metrics]") REQUIRE(name == "proxy.process.api.metrics.bad_id"); REQUIRE(m.begin() != m.end()); - REQUIRE(++m.begin() == m.end()); + + // Other test cases share this process-wide store, so the number of metrics already present + // is not knowable here. Assert the delta from creating one metric instead of an absolute + // iterator position. + auto pre_count = std::distance(m.begin(), m.end()); + + Metrics::Counter::create("iterator.marker"); + REQUIRE(std::distance(m.begin(), m.end()) == pre_count + 1); auto it = m.begin(); - it++; + std::advance(it, pre_count); + REQUIRE(it != m.end()); + ++it; REQUIRE(it == m.end()); + + auto it2 = m.begin(); + std::advance(it2, pre_count); + it2++; + REQUIRE(it2 == m.end()); } SECTION("New metric") { auto fooid = Metrics::Counter::create("foo"); - REQUIRE(fooid == 1); + // Not an absolute id: that depends on how many metrics other test cases created first. + // Assert the id is valid and round-trips through lookup. + REQUIRE(fooid != ts::Metrics::NOT_FOUND); + REQUIRE(m.lookup("foo") == fooid); REQUIRE(m.name(fooid) == "foo"); REQUIRE(m.type(fooid) == Metrics::MetricType::COUNTER); @@ -75,8 +100,16 @@ TEST_CASE("Metrics", "[libtsapi][Metrics]") auto span = Metrics::Counter::createSpan(17, &span_id); REQUIRE(span.size() == 17); - REQUIRE(fooid == 1); - REQUIRE(span_id == 3); + // Not fixed offsets: those only hold against a virgin store. Assert instead that the span + // was allocated above the earlier metric and that every id in it is valid. Both ids are + // counters, so they are directly comparable -- ids encode the metric type, and so are not + // ordered across differing types. + REQUIRE(fooid != ts::Metrics::NOT_FOUND); + REQUIRE(span_id != ts::Metrics::NOT_FOUND); + REQUIRE(span_id > fooid); + for (size_t i = 0; i < span.size(); ++i) { + REQUIRE(m.valid(span_id + static_cast(i))); + } m.rename(span_id + 0, "span.0"); m.rename(span_id + 1, "span.1"); @@ -144,3 +177,433 @@ TEST_CASE("Metrics", "[libtsapi][Metrics]") REQUIRE(m[derivedce].load() == 10); } } + +TEST_CASE("Metrics derived ops", "[libtsapi][Metrics]") +{ + auto &m = Metrics::instance(); + + SECTION("max and min") + { + auto a = Metrics::Gauge::createPtr("op-a"); + auto b = Metrics::Gauge::createPtr("op-b"); + auto c = Metrics::Gauge::createPtr("op-c"); + + Metrics::Derived::derive({ + {"op-sum", Metrics::MetricType::GAUGE, {a, b, c}, Metrics::Derived::Op::SUM}, + {"op-max", Metrics::MetricType::GAUGE, {a, b, c}, Metrics::Derived::Op::MAX}, + {"op-min", Metrics::MetricType::GAUGE, {a, b, c}, Metrics::Derived::Op::MIN}, + }); + + Metrics::Gauge::store(a, 3); + Metrics::Gauge::store(b, 9); + Metrics::Gauge::store(c, 5); + + Metrics::Derived::update_derived(); + + REQUIRE(m[m.lookup("op-sum")].load() == 17); + REQUIRE(m[m.lookup("op-max")].load() == 9); + REQUIRE(m[m.lookup("op-min")].load() == 3); + } + + SECTION("min is not clamped by a zero seed") + { + // A zero-seeded accumulator is correct for SUM but wrong for MIN: it would report 0 here + // instead of the smallest source value. + auto a = Metrics::Gauge::createPtr("posmin-a"); + auto b = Metrics::Gauge::createPtr("posmin-b"); + + Metrics::Derived::derive({ + {"posmin", Metrics::MetricType::GAUGE, {a, b}, Metrics::Derived::Op::MIN}, + }); + + Metrics::Gauge::store(a, 7); + Metrics::Gauge::store(b, 12); + Metrics::Derived::update_derived(); + + REQUIRE(m[m.lookup("posmin")].load() == 7); + } + + SECTION("negative values aggregate correctly") + { + auto a = Metrics::Gauge::createPtr("neg-a"); + auto b = Metrics::Gauge::createPtr("neg-b"); + + Metrics::Derived::derive({ + {"neg-max", Metrics::MetricType::GAUGE, {a, b}, Metrics::Derived::Op::MAX}, + {"neg-min", Metrics::MetricType::GAUGE, {a, b}, Metrics::Derived::Op::MIN}, + }); + + Metrics::Gauge::store(a, -5); + Metrics::Gauge::store(b, -2); + Metrics::Derived::update_derived(); + + REQUIRE(m[m.lookup("neg-max")].load() == -2); + REQUIRE(m[m.lookup("neg-min")].load() == -5); + } + + SECTION("a single source works for every op") + { + auto a = Metrics::Gauge::createPtr("solo-a"); + + Metrics::Derived::derive({ + {"solo-sum", Metrics::MetricType::GAUGE, {a}, Metrics::Derived::Op::SUM}, + {"solo-max", Metrics::MetricType::GAUGE, {a}, Metrics::Derived::Op::MAX}, + {"solo-min", Metrics::MetricType::GAUGE, {a}, Metrics::Derived::Op::MIN}, + }); + + Metrics::Gauge::store(a, 42); + Metrics::Derived::update_derived(); + + REQUIRE(m[m.lookup("solo-sum")].load() == 42); + REQUIRE(m[m.lookup("solo-max")].load() == 42); + REQUIRE(m[m.lookup("solo-min")].load() == 42); + } + + // An unknown name resolves to NOT_FOUND, which _splitID masks to blob 0 / offset 0 -- the + // reserved bad_id slot, which holds 0. Under SUM that is invisible, so MIN is tested with + // strictly positive sources and MAX with strictly negative ones: in both of those an + // aliased-in 0 would become the winning value and change the result. They are separate + // sections so that one failure does not mask the other. + SECTION("an unresolvable source is skipped under MIN") + { + auto a = Metrics::Gauge::createPtr("guard-pos-a"); + auto b = Metrics::Gauge::createPtr("guard-pos-b"); + + Metrics::Derived::derive({ + {"guard-min", Metrics::MetricType::GAUGE, {a, "guard-does-not-exist", b}, Metrics::Derived::Op::MIN}, + }); + + Metrics::Gauge::store(a, 5); + Metrics::Gauge::store(b, 8); + Metrics::Derived::update_derived(); + + REQUIRE(m[m.lookup("guard-min")].load() == 5); // 0 if the unknown source were included + } + + SECTION("an unresolvable source is skipped under MAX") + { + auto a = Metrics::Gauge::createPtr("guard-neg-a"); + auto b = Metrics::Gauge::createPtr("guard-neg-b"); + + Metrics::Derived::derive({ + {"guard-max", Metrics::MetricType::GAUGE, {a, "guard-does-not-exist", b}, Metrics::Derived::Op::MAX}, + }); + + Metrics::Gauge::store(a, -9); + Metrics::Gauge::store(b, -4); + Metrics::Derived::update_derived(); + + REQUIRE(m[m.lookup("guard-max")].load() == -4); // 0 if the unknown source were included + } + + SECTION("an invalid source id is skipped") + { + auto a = Metrics::Gauge::createPtr("guardid-a"); + + Metrics::Derived::derive({ + {"guardid-max", Metrics::MetricType::GAUGE, {a, Metrics::NOT_FOUND}, Metrics::Derived::Op::MAX}, + }); + + Metrics::Gauge::store(a, -7); + Metrics::Derived::update_derived(); + + REQUIRE(m[m.lookup("guardid-max")].load() == -7); + } + + SECTION("op defaults to SUM for backward compatibility") + { + auto a = Metrics::Counter::createPtr("dflt-a"); + auto b = Metrics::Counter::createPtr("dflt-b"); + Metrics::Derived::derive({ + {"dflt-sum", Metrics::MetricType::COUNTER, {a, b}} + }); + Metrics::Counter::increment(a, 2); + Metrics::Counter::increment(b, 4); + Metrics::Derived::update_derived(); + REQUIRE(m[m.lookup("dflt-sum")].load() == 6); + } +} + +TEST_CASE("Metrics derived add_source", "[libtsapi][Metrics]") +{ + auto &m = Metrics::instance(); + + SECTION("sources registered one at a time accumulate into a single derived metric") + { + auto a = Metrics::Gauge::createPtr("inc-a"); + auto b = Metrics::Gauge::createPtr("inc-b"); + auto c = Metrics::Gauge::createPtr("inc-c"); + + // Registered separately, as sources are discovered at runtime. derive() cannot be used this + // way: it appends a new entry per call, so several entries would target the same id and each + // update would overwrite the others with its own subset. + Metrics::Derived::add_source("inc-max", Metrics::MetricType::GAUGE, a, Metrics::Derived::Op::MAX); + Metrics::Derived::add_source("inc-max", Metrics::MetricType::GAUGE, b, Metrics::Derived::Op::MAX); + Metrics::Derived::add_source("inc-max", Metrics::MetricType::GAUGE, c, Metrics::Derived::Op::MAX); + + Metrics::Gauge::store(a, 4); + Metrics::Gauge::store(b, 11); + Metrics::Gauge::store(c, 7); + + Metrics::Derived::update_derived(); + + REQUIRE(m[m.lookup("inc-max")].load() == 11); + } + + SECTION("a SUM aggregate sees every source, not just the last registered") + { + // This is what distinguishes add_source from repeated derive() calls: a SUM over all three + // sources rather than over whichever subset was registered last. + auto a = Metrics::Gauge::createPtr("incsum-a"); + auto b = Metrics::Gauge::createPtr("incsum-b"); + auto c = Metrics::Gauge::createPtr("incsum-c"); + + Metrics::Derived::add_source("incsum", Metrics::MetricType::GAUGE, a); + Metrics::Derived::add_source("incsum", Metrics::MetricType::GAUGE, b); + Metrics::Derived::add_source("incsum", Metrics::MetricType::GAUGE, c); + + Metrics::Gauge::store(a, 1); + Metrics::Gauge::store(b, 20); + Metrics::Gauge::store(c, 300); + Metrics::Derived::update_derived(); + + REQUIRE(m[m.lookup("incsum")].load() == 321); + } + + SECTION("add_source is idempotent for a repeated source") + { + auto a = Metrics::Gauge::createPtr("idem-a"); + + Metrics::Derived::add_source("idem-sum", Metrics::MetricType::GAUGE, a); + Metrics::Derived::add_source("idem-sum", Metrics::MetricType::GAUGE, a); + Metrics::Gauge::store(a, 6); + Metrics::Derived::update_derived(); + + REQUIRE(m[m.lookup("idem-sum")].load() == 6); // not 12 + } + + SECTION("a null source is ignored") + { + auto a = Metrics::Gauge::createPtr("null-a"); + + Metrics::Derived::add_source("null-sum", Metrics::MetricType::GAUGE, nullptr); + Metrics::Derived::add_source("null-sum", Metrics::MetricType::GAUGE, a); + Metrics::Gauge::store(a, 9); + Metrics::Derived::update_derived(); + + REQUIRE(m[m.lookup("null-sum")].load() == 9); + } + + SECTION("a hidden metric can feed a published derived metric") + { + // The whole point of the facility: high cardinality sources stay hidden while only the + // aggregate is published. + auto h1 = Metrics::Gauge::createHiddenPtr("hidden.src.", "one"); + auto h2 = Metrics::Gauge::createHiddenPtr("hidden.src.", "two"); + + Metrics::Derived::add_source("hidden-derived-sum", Metrics::MetricType::GAUGE, h1); + Metrics::Derived::add_source("hidden-derived-sum", Metrics::MetricType::GAUGE, h2); + Metrics::Gauge::store(h1, 21); + Metrics::Gauge::store(h2, 2); + Metrics::Derived::update_derived(); + + // The derived metric itself lives in the PUBLISHED store... + REQUIRE(m.lookup("hidden-derived-sum") != Metrics::NOT_FOUND); + REQUIRE(m[m.lookup("hidden-derived-sum")].load() == 23); + // ...while its sources remain absent from it. + REQUIRE(m.lookup("hidden.src.one") == Metrics::NOT_FOUND); + REQUIRE(m.lookup("hidden.src.two") == Metrics::NOT_FOUND); + } + + SECTION("one source can feed two derived metrics with different ops") + { + // Commit 9 relies on this: a per-group gauge feeds both a SUM and a MAX aggregate. + auto a = Metrics::Gauge::createHiddenPtr("dual.src.a"); + auto b = Metrics::Gauge::createHiddenPtr("dual.src.b"); + + Metrics::Derived::add_source("dual-sum", Metrics::MetricType::GAUGE, a, Metrics::Derived::Op::SUM); + Metrics::Derived::add_source("dual-sum", Metrics::MetricType::GAUGE, b, Metrics::Derived::Op::SUM); + Metrics::Derived::add_source("dual-max", Metrics::MetricType::GAUGE, a, Metrics::Derived::Op::MAX); + Metrics::Derived::add_source("dual-max", Metrics::MetricType::GAUGE, b, Metrics::Derived::Op::MAX); + + Metrics::Gauge::store(a, 2); + Metrics::Gauge::store(b, 3); + Metrics::Derived::update_derived(); + + REQUIRE(m[m.lookup("dual-sum")].load() == 5); + REQUIRE(m[m.lookup("dual-max")].load() == 3); + } + + SECTION("add_source works on a derived metric created by derive") + { + auto a = Metrics::Gauge::createPtr("mix-a"); + auto b = Metrics::Gauge::createPtr("mix-b"); + + Metrics::Derived::derive({ + {"mix-sum", Metrics::MetricType::GAUGE, {a}, Metrics::Derived::Op::SUM}, + }); + Metrics::Derived::add_source("mix-sum", Metrics::MetricType::GAUGE, b); + + Metrics::Gauge::store(a, 10); + Metrics::Gauge::store(b, 5); + Metrics::Derived::update_derived(); + + REQUIRE(m[m.lookup("mix-sum")].load() == 15); + } +} + +TEST_CASE("Metrics hidden store", "[libtsapi][Metrics]") +{ + auto &m = Metrics::instance(); + auto &h = Metrics::hidden_instance(); + + SECTION("stores are separate") + { + REQUIRE(std::addressof(m) != std::addressof(h)); + + auto hp = Metrics::Counter::createHiddenPtr("hidden.only"); + REQUIRE(hp != nullptr); + + // Not visible in the published store, by name or by iteration. + REQUIRE(m.lookup("hidden.only") == Metrics::NOT_FOUND); + for (auto &&[name, type, value] : m) { + REQUIRE(name != "hidden.only"); + } + + // Visible in the hidden store. + REQUIRE(h.lookup("hidden.only") != Metrics::NOT_FOUND); + bool found = false; + for (auto &&[name, type, value] : h) { + if (name == "hidden.only") { + found = true; + } + } + REQUIRE(found); + } + + SECTION("same name in both stores is independent") + { + auto pub = Metrics::Counter::createPtr("dual.name"); + auto hid = Metrics::Counter::createHiddenPtr("dual.name"); + REQUIRE(pub != hid); + + // Exercised through the typed facade, which is what proves no cast is needed at a call site + // holding a Counter::AtomicType *. + Metrics::Counter::increment(pub, 3); + Metrics::Counter::increment(hid, 7); + REQUIRE(Metrics::Counter::load(pub) == 3); + REQUIRE(Metrics::Counter::load(hid) == 7); + } + + SECTION("createHiddenPtr is idempotent by name") + { + auto a = Metrics::Counter::createHiddenPtr("hidden.idem"); + auto b = Metrics::Counter::createHiddenPtr("hidden.idem"); + REQUIRE(a == b); + } + + SECTION("prefixed create") + { + auto p = Metrics::Counter::createHiddenPtr("pfx.", "suffix"); + REQUIRE(p != nullptr); + REQUIRE(h.lookup("pfx.suffix") != Metrics::NOT_FOUND); + } + + SECTION("the metric type is recorded in the hidden store") + { + Metrics::IdType cid{}, gid{}; + + REQUIRE(Metrics::Counter::createHiddenPtr("hidden.typed.counter") != nullptr); + REQUIRE(Metrics::Gauge::createHiddenPtr("hidden.typed.gauge") != nullptr); + + cid = h.lookup("hidden.typed.counter"); + gid = h.lookup("hidden.typed.gauge"); + REQUIRE(cid != Metrics::NOT_FOUND); + REQUIRE(gid != Metrics::NOT_FOUND); + + REQUIRE(h.type(cid) == Metrics::MetricType::COUNTER); + REQUIRE(h.type(gid) == Metrics::MetricType::GAUGE); + } + + SECTION("hidden gauge works with the typed Gauge API") + { + auto g = Metrics::Gauge::createHiddenPtr("hidden.gauge.", "one"); + REQUIRE(g != nullptr); + Metrics::Gauge::store(g, 42); + REQUIRE(Metrics::Gauge::load(g) == 42); + Metrics::Gauge::increment(g); + REQUIRE(Metrics::Gauge::load(g) == 43); + Metrics::Gauge::decrement(g); + REQUIRE(Metrics::Gauge::load(g) == 42); + } + + SECTION("hidden metrics are shared across threads") + { + // Metrics is thread_local but Storage is shared, so the same name must resolve to the same + // atomic on every thread. This is how per-group counters are created from many event threads. + constexpr int N_THREADS = 4; + std::vector threads; + std::array ptrs{}; + + for (int i = 0; i < N_THREADS; ++i) { + threads.emplace_back([i, &ptrs]() { + auto p = Metrics::Counter::createHiddenPtr("hidden.threaded"); + ptrs[i] = p; + Metrics::Counter::increment(p, 10); + }); + } + for (auto &t : threads) { + t.join(); + } + + for (int i = 1; i < N_THREADS; ++i) { + REQUIRE(ptrs[i] == ptrs[0]); + } + REQUIRE(Metrics::Counter::load(ptrs[0]) == N_THREADS * 10); + } +} + +TEST_CASE("Metrics blob growth boundary", "[libtsapi][Metrics]") +{ + // Storage packs metrics into fixed-size blobs (MAX_SIZE entries each). Creating more than + // MAX_SIZE metrics forces at least one new blob to be allocated, which is exactly where an + // off-by-one in the blob/offset bookkeeping would corrupt or orphan entries. Use the hidden + // store so this doesn't dump thousands of names into the published store that other test cases + // iterate over. + auto &h = Metrics::hidden_instance(); + constexpr int COUNT = Metrics::MAX_SIZE + 100; + std::vector ptrs; + std::vector names; + + ptrs.reserve(COUNT); + names.reserve(COUNT); + + for (int i = 0; i < COUNT; ++i) { + names.push_back("blob.growth." + std::to_string(i)); + auto p = Metrics::Counter::createHiddenPtr(names[i]); + REQUIRE(p != nullptr); + ptrs.push_back(p); + Metrics::Counter::increment(p, i); + } + + for (int i = 0; i < COUNT; ++i) { + auto id = h.lookup(names[i]); + REQUIRE(id != Metrics::NOT_FOUND); + REQUIRE(h.valid(id)); + + // Re-creating by name must be idempotent and resolve to the exact same atomic: a blob + // boundary bug that aliases two entries onto the same slot, or orphans one behind the + // boundary, would fail this. + auto p2 = Metrics::Counter::createHiddenPtr(names[i]); + REQUIRE(p2 == ptrs[i]); + + // Distinct values catch aliasing: if two logically distinct entries were mapped to the same + // underlying atomic, this readback would not match the index written above. + REQUIRE(Metrics::Counter::load(ptrs[i]) == i); + } + + // Every pointer must be distinct: no two names should have been aliased onto the same atomic. + std::vector sorted_ptrs = ptrs; + std::sort(sorted_ptrs.begin(), sorted_ptrs.end()); + REQUIRE(std::adjacent_find(sorted_ptrs.begin(), sorted_ptrs.end()) == sorted_ptrs.end()); +} diff --git a/tests/gold_tests/jsonrpc/metric_match_include_hidden.test.py b/tests/gold_tests/jsonrpc/metric_match_include_hidden.test.py new file mode 100644 index 00000000000..9dd35c11982 --- /dev/null +++ b/tests/gold_tests/jsonrpc/metric_match_include_hidden.test.py @@ -0,0 +1,49 @@ +''' +Verify that "traffic_ctl metric match --include-hidden" is accepted end-to-end by the +JSONRPC server (i.e. the additional rec type is not rejected during request decoding) +and still returns normal, published metrics. +''' +# 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. + +Test.Summary = __doc__ + +ts = Test.MakeATSProcess("ts") + +tr = Test.AddTestRun("metric match --include-hidden is accepted and still returns published metrics") +tr.Processes.Default.Command = 'traffic_ctl metric match reconfigure_time --include-hidden' +tr.Processes.Default.Env = ts.Env +tr.Processes.Default.ReturnCode = 0 +tr.Processes.Default.StartBefore(ts) +# Proves the query actually ran against the server and matched a real, published metric. +tr.Processes.Default.Streams.All = Testers.ContainsExpression( + r'proxy\.process\.proxy\.reconfigure_time', 'Expected the published reconfigure_time metric to be present in the output.') +# Proves the new rec type bit was not rejected by the JSONRPC request decoder. +# NOTE: must be "+=", not "=". Assigning a stream tester replaces any previously assigned +# tester for that stream, which would silently drop the check above. +tr.Processes.Default.Streams.All += Testers.ExcludesExpression( + 'INVALID_INCOMING_DATA', 'The --include-hidden flag must not cause the JSONRPC request to be rejected as invalid.') +tr.StillRunningAfter = ts + +tr = Test.AddTestRun("a normal metric match must not return hidden metrics") +tr.Processes.Default.Command = 'traffic_ctl metric match reconfigure_time' +tr.Processes.Default.Env = ts.Env +tr.Processes.Default.ReturnCode = 0 +# RECT_HIDDEN_METRIC sits outside RECT_ALL, so a plain query still works and is unaffected. +tr.Processes.Default.Streams.All = Testers.ContainsExpression( + r'proxy\.process\.proxy\.reconfigure_time', 'Expected the published reconfigure_time metric without --include-hidden too.') +tr.Processes.Default.Streams.All += Testers.ExcludesExpression('INVALID_INCOMING_DATA', 'A plain metric match must remain valid.') +tr.StillRunningAfter = ts 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 e7bad788ab3..9dc9b12f204 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 @@ -1,5 +1,6 @@ ''' -Verify the behavior of proxy.config.http.per_server.connection.max. +Verify the behavior of proxy.config.http.per_server.connection.max and the per server +connection metrics (proxy.config.http.per_server.connection.metric_enabled). ''' # Licensed to the Apache Software Foundation (ASF) under one # or more contributor license agreements. See the NOTICE file @@ -22,6 +23,17 @@ Test.SkipIf(Condition.CurlUsingUnixDomainSocket()) +# The per hostname aggregates are derived metrics, recomputed once per +# REC_RAW_STAT_SYNC_INTERVAL_MS (5000ms, src/records/P_RecDefs.h) on an ET_TASK thread. Nothing in +# records.yaml drives that interval, so a test has to sleep comfortably longer than one sync period +# before reading an aggregate rather than trying to configure a faster one. Reading too early +# silently compares against zeros. +_STAT_SYNC_WAIT_SECONDS: int = 6 + +# NOTE: assigning to a Streams attribute REPLACES any tester already set for that stream +# (TesterSet.Assign), so every assertion after the first on the same stream must use '+=' or it +# silently discards the earlier ones. + class PerServerConnectionMaxTest: """Define an object to test our max origin connection behavior.""" @@ -54,7 +66,10 @@ def _configure_trafficserver(self) -> None: 'proxy.config.diags.debug.enabled': 1, 'proxy.config.diags.debug.tags': 'http|conn_track', 'proxy.config.http.per_server.connection.max': self._origin_max_connections, - 'proxy.config.http.per_server.connection.metric_enabled': 1, + # Level 2 (METRIC_LEVEL_GROUP): the match here is 'port', which never has more than + # one group per hostname, so there is no aggregate to read and the per group + # metrics themselves have to be published to be checked below. + 'proxy.config.http.per_server.connection.metric_enabled': 2, 'proxy.config.http.per_server.connection.metric_prefix': 'foo', 'proxy.config.http.per_server.connection.match': 'port', }) @@ -64,16 +79,26 @@ def _configure_trafficserver(self) -> None: def _test_metrics(self) -> None: """Use traffic_ctl to test metrics.""" + group_name = f'foo.127.0.0.1:{self._server.Variables.http_port}' + tr = Test.AddTestRun("Check connection metrics") - tr.Processes.Default.Command = 'traffic_ctl metric match per_server' + # At level 2 the per group metrics are published by mirroring the hidden ones through a + # derived metric, so a sync tick has to pass before they carry a value. + 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( - f'per_server.total_connection.foo.127.0.0.1:{self._server.Variables.http_port} 4', - 'incorrect statistic return, or possible error.') - tr.Processes.Default.Streams.All = Testers.ContainsExpression( - f'per_server.blocked_connection.foo.127.0.0.1:{self._server.Variables.http_port} 1', - 'incorrect statistic return, or possible error.') + f'per_server.total_connection.{group_name} 4', 'incorrect statistic return, or possible error.') + tr.Processes.Default.Streams.All += Testers.ExcludesExpression( + 'INVALID_INCOMING_DATA', 'The metric query must not be rejected.') + tr.Processes.Default.Streams.All += Testers.ContainsExpression( + f'per_server.blocked_connection.{group_name} 1', 'incorrect statistic return, or possible error.') + + # 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.') def run(self) -> None: """Configure the TestRun.""" @@ -88,16 +113,28 @@ def run(self) -> None: class ConnectMethodTest: - """Test our max origin connection behavior with CONNECT traffic.""" + """Test our max origin connection behavior with CONNECT traffic. + + Also covers the two publication levels of proxy.config.http.per_server.connection.metric_enabled: + - 1 (METRIC_LEVEL_HOST): only the per hostname aggregate is published; the per group metrics + stay hidden and are visible only with --include-hidden. + - 2 (METRIC_LEVEL_GROUP): the per hostname aggregate is 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 + aggregate is a trivial sum over that single group. MultiGroupAggregateTest below covers the + case where an aggregate genuinely spans more than one group. + """ _process_counter: int = 0 _client_counter: int = 0 - def __init__(self, max_conn) -> None: + def __init__(self, max_conn, metric_level=1) -> None: """Configure the server processes in preparation for the TestRun.""" + self._metric_level = metric_level self._configure_dns() self._configure_origin_server() - self._configure_trafficserver(max_conn) + self._configure_trafficserver(max_conn, metric_level) ConnectMethodTest._process_counter += 1 def _configure_dns(self) -> None: @@ -108,8 +145,8 @@ def _configure_origin_server(self) -> None: """Configure the httpbin origin server.""" self._server = Test.MakeHttpBinServer(f"server_{ConnectMethodTest._process_counter}") - def _configure_trafficserver(self, max_conn) -> None: - self._ts = Test.MakeATSProcess("ts2_" + str(max_conn)) + def _configure_trafficserver(self, max_conn, metric_level) -> None: + self._ts = Test.MakeATSProcess(f"ts2_{max_conn}_{metric_level}") self._ts.Disk.records_config.update( { @@ -119,7 +156,7 @@ def _configure_trafficserver(self, max_conn) -> None: 'proxy.config.diags.debug.tags': 'http|dns|hostdb|conn_track', 'proxy.config.http.server_ports': f"{self._ts.Variables.port} {self._ts.Variables.uds_path}", 'proxy.config.http.connect_ports': f"{self._server.Variables.Port}", - 'proxy.config.http.per_server.connection.metric_enabled': 1, + 'proxy.config.http.per_server.connection.metric_enabled': metric_level, 'proxy.config.http.per_server.connection.max': max_conn, }) @@ -136,17 +173,47 @@ def _configure_client_with_slow_response(self, tr) -> 'Test.Process': return p def _test_metrics(self, blocked) -> None: - """Use traffic_ctl to test metrics.""" + """Use traffic_ctl to test metrics, honoring the configured publication level.""" + host_name = 'www.this.origin.com' + group_name = f'{host_name}.127.0.0.1:{self._server.Variables.Port}' + tr = Test.AddTestRun("Check connection metrics") - tr.Processes.Default.Command = 'traffic_ctl metric match per_server; sleep 2' + 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 + + # The per hostname aggregate is published at every non-zero level. tr.Processes.Default.Streams.All = Testers.ContainsExpression( - f'per_server.total_connection.www.this.origin.com.127.0.0.1:{self._server.Variables.Port} 5', - 'incorrect statistic return, or possible error.') - tr.Processes.Default.Streams.All = Testers.ContainsExpression( - f'per_server.blocked_connection.www.this.origin.com.127.0.0.1:{self._server.Variables.Port} {blocked}', - 'incorrect statistic return, or possible error.') + f'per_server.total_connection.{host_name} 5', 'incorrect statistic return, or possible error.') + tr.Processes.Default.Streams.All += Testers.ContainsExpression( + f'per_server.blocked_connection.{host_name} {blocked}', 'incorrect statistic return, or possible error.') + + if self._metric_level >= 2: + # METRIC_LEVEL_GROUP additionally mirrors the per group metrics into the published store. + tr.Processes.Default.Streams.All += Testers.ContainsExpression( + f'per_server.total_connection.{group_name} 5', 'The per group metric should be published at METRIC_LEVEL_GROUP.') + else: + # METRIC_LEVEL_HOST 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 level 1.') + + # 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. + tr2 = Test.AddTestRun("Check hidden per group connection metrics") + tr2.Processes.Default.Command = 'traffic_ctl metric match per_server --include-hidden' + tr2.Processes.Default.ReturnCode = 0 + tr2.Processes.Default.Env = self._ts.Env + # No sleep needed: the hidden per group metrics are written directly on each connection, + # unlike the derived aggregates. + tr2.Processes.Default.Streams.All = Testers.ContainsExpression( + f'per_server.total_connection.{group_name} 5', + 'The per group metric should be visible with --include-hidden at any level.') + tr2.Processes.Default.Streams.All += Testers.ExcludesExpression( + 'INVALID_INCOMING_DATA', 'The --include-hidden query must not be rejected by the RPC decoder.') def run(self, blocked, gold_file) -> None: """Verify per_server.connection.max with CONNECT traffic.""" @@ -179,6 +246,149 @@ def run(self, blocked, gold_file) -> None: self._test_metrics(blocked) +class MultiGroupAggregateTest: + """Verify a per hostname aggregate that genuinely spans more than one group. + + The other tests here resolve a hostname to a single 127.0.0.1:port, so their "aggregate" is + trivially a set of one. Here two remap rules point at the same hostname ('multi.origin.com') + on two different origin ports, so under match 'both' the connection tracker creates two + 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 + 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 + a monotone peak, is that both gauges return to 0 once traffic drains and another tick passes. + """ + + _process_counter: int = 0 + _client_counter: int = 0 + + # Concurrent slow requests per group. Deliberately different so SUM (5) and MAX (3) differ. + _group_a_concurrency: int = 2 + _group_b_concurrency: int = 3 + + # How long each request holds its connection open. Must comfortably exceed + # _STAT_SYNC_WAIT_SECONDS so a sync tick is guaranteed to land while the connections are still + # open. NOTE: httpbin clamps /delay/ to 10 seconds, so the effective hold is min(this, 10); + # the waits below are derived from this value and tolerate that clamp. + _hold_seconds: int = 12 + + def __init__(self) -> None: + """Configure the test processes in preparation for the TestRun.""" + self._configure_dns() + self._configure_origin_servers() + self._configure_trafficserver() + MultiGroupAggregateTest._process_counter += 1 + + def _configure_dns(self) -> None: + """Configure a nameserver for the test.""" + self._dns = Test.MakeDNServer(f"magg_dns_{MultiGroupAggregateTest._process_counter}", default='127.0.0.1') + + def _configure_origin_servers(self) -> None: + """Configure the two httpbin origins which stand in for two groups of one hostname.""" + self._server_a = Test.MakeHttpBinServer(f"magg_server_a_{MultiGroupAggregateTest._process_counter}") + self._server_b = Test.MakeHttpBinServer(f"magg_server_b_{MultiGroupAggregateTest._process_counter}") + + def _configure_trafficserver(self) -> None: + """Configure Traffic Server with two remap rules to the same hostname on different ports.""" + self._ts = Test.MakeATSProcess(f"magg_ts_{MultiGroupAggregateTest._process_counter}") + self._ts.Disk.records_config.update( + { + 'proxy.config.dns.nameservers': f"127.0.0.1:{self._dns.Variables.Port}", + 'proxy.config.dns.resolv_conf': 'NULL', + '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, + 'proxy.config.http.per_server.connection.match': 'both', + }) + self._ts.Disk.remap_config.AddLines( + [ + f"map http://multi.origin.com/a/ http://multi.origin.com:{self._server_a.Variables.Port}/", + f"map http://multi.origin.com/b/ http://multi.origin.com:{self._server_b.Variables.Port}/", + ]) + + def _make_slow_client(self, tr, path) -> 'Test.Process': + """Configure a client which makes a slow request through one of the two remapped groups.""" + p = tr.Processes.Process(f'magg_client_{MultiGroupAggregateTest._client_counter}') + MultiGroupAggregateTest._client_counter += 1 + tr.MakeCurlCommand( + f"-v --fail -s -x 127.0.0.1:{self._ts.Variables.port} " + f"'http://multi.origin.com/{path}/delay/{MultiGroupAggregateTest._hold_seconds}'", + p=p, + ts=self._ts) + return p + + def _test_metrics_while_held(self) -> None: + """While the slow requests are still in flight, verify the live gauges reflect them.""" + total = MultiGroupAggregateTest._group_a_concurrency + MultiGroupAggregateTest._group_b_concurrency + group_max = max(MultiGroupAggregateTest._group_a_concurrency, MultiGroupAggregateTest._group_b_concurrency) + + tr = Test.AddTestRun("Check the host aggregate spans both groups while connections are held open") + 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( + f'per_server.total_connection.multi.origin.com {total}', + 'The host aggregate total_connection should be the SUM across both groups ' + f'({MultiGroupAggregateTest._group_a_concurrency} + {MultiGroupAggregateTest._group_b_concurrency}).') + tr.Processes.Default.Streams.All += Testers.ContainsExpression( + f'per_server.current_connection.multi.origin.com {total}', + '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 ' + 'count (MAX), not the sum across the two groups.') + + def _test_metrics_after_drain(self) -> None: + """After traffic drains and a further sync tick passes, both live gauges must read 0. + + This validates the behavior the design exists to provide: an instantaneous gauge, unlike a + monotone peak, comes back down. + """ + tr = Test.AddTestRun("Check the host aggregate drains back to 0 after traffic stops") + # The slow requests are already _STAT_SYNC_WAIT_SECONDS old by now; wait for the rest of + # their hold time and then for another sync tick to observe the drop to 0. + wait = max(0, MultiGroupAggregateTest._hold_seconds - _STAT_SYNC_WAIT_SECONDS) + _STAT_SYNC_WAIT_SECONDS + tr.Processes.Default.Command = f'sleep {wait}; traffic_ctl metric match per_server' + tr.Processes.Default.ReturnCode = 0 + tr.Processes.Default.Env = self._ts.Env + tr.Processes.Default.TimeOut = wait + 30 + tr.Processes.Default.Streams.All = Testers.ContainsExpression( + '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 ' + 'is a live gauge, not a monotone peak.') + + def run(self) -> None: + """Drive concurrent traffic through both groups, then check the aggregate metrics.""" + tr = Test.AddTestRun() + tr.Processes.Default.StartBefore(self._dns) + tr.Processes.Default.StartBefore(self._server_a) + tr.Processes.Default.StartBefore(self._server_b) + tr.Processes.Default.StartBefore(self._ts) + + clients = [self._make_slow_client(tr, 'a') for _ in range(MultiGroupAggregateTest._group_a_concurrency)] + clients += [self._make_slow_client(tr, 'b') for _ in range(MultiGroupAggregateTest._group_b_concurrency)] + for p in clients: + tr.Processes.Default.StartBefore(p) + + # Let the slow requests connect and overlap before checking anything; they stay open for + # _hold_seconds from about this point. + tr.Processes.Default.Command = 'sleep 1' + tr.Processes.Default.ReturnCode = 0 + tr.Processes.Default.TimeOut = 30 + + self._test_metrics_while_held() + self._test_metrics_after_drain() + + PerServerConnectionMaxTest().run() -ConnectMethodTest(3).run(blocked=2, gold_file="gold/two_503_congested.gold") -ConnectMethodTest(0).run(blocked=0, gold_file="gold/two_200_ok.gold") +ConnectMethodTest(3, metric_level=1).run(blocked=2, gold_file="gold/two_503_congested.gold") +ConnectMethodTest(0, metric_level=2).run(blocked=0, gold_file="gold/two_200_ok.gold") +MultiGroupAggregateTest().run()