This is an automated email from the ASF dual-hosted git repository. cmcfarlen pushed a commit to branch 10.2.x in repository https://gitbox.apache.org/repos/asf/trafficserver.git
commit 4c0cfafc7ce957eaaf9e4d2fe3a660ebf951b31f Author: Chris McFarlen <[email protected]> AuthorDate: Tue Aug 18 16:33:28 2026 -0500 Add hidden metrics and MAX/MIN/incremental derived metric aggregation (#13505) * tsutil: make the Metrics unit tests order independent The tests asserted absolute metric ids and iterator positions, which only hold when the case runs first against an otherwise empty store. Any other test case that creates a published metric makes them fail. Assert on relative state instead so the cases can run in any order. * tsutil: add a separate storage for hidden metrics Hidden metrics are stored but never published. Using a separate Storage instance rather than a per-metric flag makes them structurally unreachable from the published store, so no metric consumer can expose them by omission. Gauge and Counter each gain createHiddenPtr overloads which return the same correctly typed pointer as createPtr, so a hidden metric is read and written with the normal typed mutators and no cast is needed at the call site. * tsutil: fail gracefully when metric storage is exhausted Storage::create() had no exhaustion check, so filling the last blob let the following bookkeeping call addBlob() and write one past the end of _blobs. Refuse the final slot instead and return the reserved bad_id, which keeps addBlob() from ever being reached in a full store and costs one slot out of 8M. The guard in addBlob() was also off by one against the access it protects, since the write is to _blobs[++_cur_blob], and being a debug_assert it was compiled out of release builds entirely. Make it a release_assert against MAX_BLOBS - 1. * traffic_ctl: add --include-hidden to metric match Hidden metrics are invisible to normal queries by design, which makes them hard to debug. Add an opt-in rec type bit, deliberately outside RECT_ALL so hidden metrics are never returned unless explicitly requested. The rec type also has to be accepted by the JSONRPC request decoder, which validates each requested type against a whitelist and rejects the whole request otherwise. No wire or schema change is needed, as rec_types is already an untyped list of ints. * tsutil: support MAX and MIN aggregation for derived metrics Derived metrics could only sum their sources. Add an op to the spec so a derived metric can also take the max or min across its sources, which is what an aggregate over instantaneous gauges needs. The accumulator is seeded from the first source rather than from zero, since a zero seed is only correct for SUM and would clamp MIN to <= 0. op defaults to SUM, so existing specs are unaffected. * tsutil: skip derived metric sources that do not resolve A source given by name or id that does not resolve was still passed to lookup(), which masks the unresolved id down to the reserved bad_id slot. The aggregate then silently included that slot's value instead of skipping the source, with no error reported. Resolve each source first and skip it if it does not resolve. This is observable under MAX and MIN, where the bad_id value can become the winning one; under SUM it happened to be hidden by bad_id holding zero. * tsutil: allow adding derived metric sources at runtime derive() only accepts a fixed initializer_list, which does not work for aggregates whose sources are discovered as the process runs. Calling it repeatedly for one derived name does not help either: it appends a separate entry per call, all targeting the same metric, so each update overwrites the others with its own subset and the last writer silently wins. add_source() accumulates sources into a single entry instead. Registering a source that is already present is a no-op, so a caller that may re-register the same source need not track that itself. * doc: document hidden and derived metrics Add a developer guide page for the metrics registry covering the hidden store, how it differs from the published one and why it is a separate store rather than a flag, and the derived metric aggregation ops including when derived values are recomputed and what that means for a sampled maximum. * Tag hidden metrics with RECT_HIDDEN_METRIC as well as RECT_PROCESS The record lookup callback rejects any record whose rec_type shares no bit with the requested mask, so tagging hidden metrics RECT_PROCESS alone made a request for only RECT_HIDDEN_METRIC fail with REQUESTED_TYPE_MISMATCH. Now that the request decoder accepts that type on its own, such a request is expressible, so set both bits. Add a unit test covering the hidden-only and include-hidden requests and confirming RECT_ALL still excludes them. * Value-initialize the synthetic records in the record lookups Both lookup functions build a RecRecord on the stack for metrics, which live outside the g_records array, and hand it to the caller's callback. The JSONRPC encoder reads version, registered, rsb_id, order and data_default unconditionally, so leaving them indeterminate lets a --format json metric query emit different values on successive runs, and reading an indeterminate bool is undefined behavior. Five sites, all with the same one word fix. Only the hidden metric loop is new in this branch; the rest have had the pattern for years. * Grow a new blob when a span ends on the blob boundary createSpan checked whether a span fit before reserving it but never re-checked afterwards, so a span ending exactly on MAX_SIZE left the offset at MAX_SIZE with no new blob allocated. The next create() then wrote one past the end of that blob's name array, and end() became an id that iterator::next() can never reach, since it wraps at ++offset == MAX_SIZE. create() has always grown as soon as it consumed the last slot; do the same here. Also refuse a span that would fill or overflow the final blob, so the new growth cannot ask addBlob() to go past the last one and trip its assert. createSpan(MAX_SIZE) always starts a fresh blob and fills it exactly, whatever the current offset, so the added test reaches the boundary deterministically. It fails without the fix. * Polish the --include-hidden surface Three small corrections to the flag added earlier in this branch: Skip slot 0 when walking the hidden store. Every Storage reserves it for the bad_id placeholder, so it exists under the same name in both stores and a query matching it returned two records differing only in value, in exactly the debugging situation the flag is for. Scope the option to 'match' with a nested program directive. As a bare option under 'traffic_ctl metric' it rendered as a peer of get, match and describe, so it read as another subcommand rather than a flag on match. This follows the 'config get --records' pattern earlier in the file. Put the flag before the positional in the CLI example usage so it agrees with that synopsis, which is also the convention the rest of the file uses. (cherry picked from commit 266ee969bc30b8e02a74752a366e6533feb38195) --- doc/appendices/command-line/traffic_ctl.en.rst | 10 +- .../internal-libraries/Metrics.en.rst | 198 ++++++++ .../internal-libraries/index.en.rst | 1 + include/records/RecDefs.h | 5 +- include/shared/rpc/RPCRequests.h | 2 + include/tsutil/Metrics.h | 77 ++++ src/mgmt/rpc/handlers/records/Records.cc | 1 + src/records/CMakeLists.txt | 1 + src/records/RecCore.cc | 38 +- .../unit_tests/test_RecHiddenMetricLookup.cc | 117 +++++ src/traffic_ctl/CtrlCommands.cc | 10 +- src/traffic_ctl/CtrlCommands.h | 5 +- src/traffic_ctl/traffic_ctl.cc | 4 +- src/tsutil/Metrics.cc | 115 ++++- src/tsutil/unit_tests/test_Metrics.cc | 506 ++++++++++++++++++++- .../jsonrpc/metric_match_include_hidden.test.py | 49 ++ 16 files changed, 1113 insertions(+), 26 deletions(-) diff --git a/doc/appendices/command-line/traffic_ctl.en.rst b/doc/appendices/command-line/traffic_ctl.en.rst index 1779f9cc32..762dfdfaad 100644 --- a/doc/appendices/command-line/traffic_ctl.en.rst +++ b/doc/appendices/command-line/traffic_ctl.en.rst @@ -827,13 +827,21 @@ 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. +.. program:: traffic_ctl metric match +.. 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 0000000000..4822170546 --- /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 0dc9820afd..6c1574e4ef 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/records/RecDefs.h b/include/records/RecDefs.h index 8befda0d89..2935f528f7 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 0a047d529d..b7c5879358 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<int> CONFIG_REC_TYPES = {1, 16}; static const std::vector<int> 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<int> 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 630f079b4d..40cdef9522 100644 --- a/include/tsutil/Metrics.h +++ b/include/tsutil/Metrics.h @@ -114,6 +114,22 @@ public: // 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<int64_t> as the underlying class for a single metric, and be happy. IdType @@ -415,6 +431,27 @@ public: return reinterpret_cast<AtomicType *>(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<AtomicType *>(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<AtomicType *>(instance.lookup(instance._create(tmpname, MetricType::GAUGE))); + } + static Metrics::Gauge::SpanType createSpan(size_t size, IdType *id = nullptr) { @@ -512,6 +549,27 @@ public: return reinterpret_cast<AtomicType *>(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<AtomicType *>(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<AtomicType *>(instance.lookup(instance._create(tmpname, MetricType::COUNTER))); + } + static Metrics::Counter::SpanType createSpan(size_t size, IdType *id = nullptr) { @@ -585,11 +643,15 @@ public: 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<Metrics::AtomicType *, Metrics::IdType, std::string_view>; std::string_view derived_name; Metrics::MetricType derived_type; std::initializer_list<MetricSpec> derived_from; + Op op{Op::SUM}; }; /** @@ -600,6 +662,21 @@ public: */ static void derive(const std::initializer_list<DerivedMetricSpec> &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/mgmt/rpc/handlers/records/Records.cc b/src/mgmt/rpc/handlers/records/Records.cc index e2bb3f0cb2..2cbb1ae4d4 100644 --- a/src/mgmt/rpc/handlers/records/Records.cc +++ b/src/mgmt/rpc/handlers/records/Records.cc @@ -104,6 +104,7 @@ template <> struct convert<RequestRecordElement> { 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/records/CMakeLists.txt b/src/records/CMakeLists.txt index 29afffb34c..f5e643ae69 100644 --- a/src/records/CMakeLists.txt +++ b/src/records/CMakeLists.txt @@ -53,6 +53,7 @@ if(BUILD_TESTING) unit_tests/test_RecDumpRecords.cc unit_tests/test_ConfigReloadTask.cc unit_tests/test_ConfigRegistry.cc + unit_tests/test_RecHiddenMetricLookup.cc ) target_link_libraries(test_records PRIVATE records configmanager inkevent Catch2::Catch2 ts::tscore libswoc::libswoc) add_catch2_test(NAME test_records COMMAND test_records) diff --git a/src/records/RecCore.cc b/src/records/RecCore.cc index c5daa449bf..9639430d24 100644 --- a/src/records/RecCore.cc +++ b/src/records/RecCore.cc @@ -521,7 +521,7 @@ RecLookupRecord(const char *name, void (*callback)(const RecRecord *, void *), v auto it = metrics.find(name); if (it != metrics.end()) { - RecRecord r; + RecRecord r{}; auto &&[name, type, val] = *it; r.rec_type = RECT_PLUGIN; @@ -554,7 +554,7 @@ RecLookupRecord(const char *name, void (*callback)(const RecRecord *, void *), v auto &strings = ts::Metrics::StaticString::instance(); if (auto m = strings.lookup(std::string{name}); m) { - RecRecord r; + RecRecord r{}; r.rec_type = RECT_PLUGIN; r.data_type = RECD_STRING; r.name = name; @@ -585,7 +585,7 @@ RecLookupMatchingRecords(unsigned rec_type, const char *match, void (*callback)( // librecords callback with a "pseudo" record. for (auto &&[name, type, val] : ts::Metrics::instance()) { if (regex.exec(name.data())) { - RecRecord tmp; + RecRecord tmp{}; tmp.rec_type = RECT_PROCESS; @@ -598,7 +598,7 @@ RecLookupMatchingRecords(unsigned rec_type, const char *match, void (*callback)( // Finally check string metrics ts::Metrics::StaticString::instance().for_each([&](const std::string &name, const std::string &value) { if (regex.exec(name)) { - RecRecord tmp; + RecRecord tmp{}; tmp.rec_type = RECT_PROCESS; @@ -612,6 +612,36 @@ 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. + auto &hidden = ts::Metrics::hidden_instance(); + // Slot 0 of every Storage is the reserved bad_id placeholder, so it exists under the same name + // in both stores. Skip it here, otherwise a query matching it returns two identically named + // records that differ only in value. + auto it = hidden.begin(); + + ++it; + for (; it != hidden.end(); ++it) { + auto &&[name, type, val] = *it; + + if (regex.exec(name.data())) { + RecRecord tmp{}; + + // Tag both bits so that a caller asking only for RECT_HIDDEN_METRIC passes the rec_type + // check the lookup callback applies to every record it is handed. Note this combination + // satisfies neither REC_TYPE_IS_STAT nor REC_TYPE_IS_CONFIG (both compare for equality), + // so the YAML encoder emits no stat_meta block for hidden metrics. That is intentional: + // the meta fields of this synthetic record were never populated. + tmp.rec_type = static_cast<RecT>(RECT_PROCESS | RECT_HIDDEN_METRIC); + + 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/unit_tests/test_RecHiddenMetricLookup.cc b/src/records/unit_tests/test_RecHiddenMetricLookup.cc new file mode 100644 index 0000000000..d1e2d4c4fc --- /dev/null +++ b/src/records/unit_tests/test_RecHiddenMetricLookup.cc @@ -0,0 +1,117 @@ +/** @file + + Catch-based tests for hidden metric lookup through librecords + + @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 <catch2/catch_test_macros.hpp> +#include <string> +#include <vector> + +#include "../P_RecCore.h" +#include "tsutil/Metrics.h" + +namespace +{ +struct LookupEntry { + RecT rec_type; + std::string name; + RecInt int_value{0}; +}; + +void +collect(const RecRecord *record, void *data) +{ + auto *entries = static_cast<std::vector<LookupEntry> *>(data); + + entries->push_back({record->rec_type, record->name ? record->name : "", record->data.rec_int}); +} + +// Mirrors the check the JSONRPC record lookup applies to every record it is handed, see +// rpc::handlers::utils::get_yaml_record_regex(). A record whose rec_type shares no bit with +// the requested mask is rejected with REQUESTED_TYPE_MISMATCH. +bool +passes_requested_type_check(unsigned requested, RecT rec_type) +{ + return (requested & rec_type) != 0; +} + +} // namespace + +TEST_CASE("RecLookupMatchingRecords - hidden metrics", "[librecords][RecLookup][hidden]") +{ + const std::string name = "proxy.test.lookup.hidden_gauge"; + auto *m = ts::Metrics::Gauge::createHiddenPtr(name); + + REQUIRE(m != nullptr); + m->store(42); + + SECTION("a hidden-only request returns the metric and survives the requested-type check") + { + std::vector<LookupEntry> entries; + + REQUIRE(RecLookupMatchingRecords(RECT_HIDDEN_METRIC, name.c_str(), collect, &entries) == REC_ERR_OKAY); + + bool found = false; + + for (const auto &e : entries) { + if (e.name == name) { + found = true; + CHECK(e.int_value == 42); + // Both bits must be set: RECT_HIDDEN_METRIC so a caller that asked only for hidden + // metrics is not rejected, and RECT_PROCESS so the record still encodes as a metric. + CHECK((e.rec_type & RECT_HIDDEN_METRIC) != 0); + CHECK((e.rec_type & RECT_PROCESS) != 0); + CHECK(passes_requested_type_check(RECT_HIDDEN_METRIC, e.rec_type)); + break; + } + } + + REQUIRE(found); + } + + SECTION("an include-hidden request also survives the requested-type check") + { + std::vector<LookupEntry> entries; + const unsigned requested = RECT_PROCESS | RECT_NODE | RECT_PLUGIN | RECT_HIDDEN_METRIC; + + REQUIRE(RecLookupMatchingRecords(requested, name.c_str(), collect, &entries) == REC_ERR_OKAY); + + bool found = false; + + for (const auto &e : entries) { + if (e.name == name) { + found = true; + CHECK(passes_requested_type_check(requested, e.rec_type)); + break; + } + } + + REQUIRE(found); + } + + SECTION("hidden metrics stay out of a normal RECT_ALL request") + { + std::vector<LookupEntry> entries; + + REQUIRE(RecLookupMatchingRecords(RECT_ALL, name.c_str(), collect, &entries) == REC_ERR_OKAY); + + for (const auto &e : entries) { + CHECK(e.name != name); + } + } +} diff --git a/src/traffic_ctl/CtrlCommands.cc b/src/traffic_ctl/CtrlCommands.cc index 64e1880c71..b90d14c43c 100644 --- a/src/traffic_ctl/CtrlCommands.cc +++ b/src/traffic_ctl/CtrlCommands.cc @@ -196,12 +196,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); } @@ -735,7 +735,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 faddede10f..1709fecfa3 100644 --- a/src/traffic_ctl/CtrlCommands.h +++ b/src/traffic_ctl/CtrlCommands.h @@ -120,7 +120,9 @@ protected: /// @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 @@ private: 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 9aa39cc3d4..ba704f1745 100644 --- a/src/traffic_ctl/traffic_ctl.cc +++ b/src/traffic_ctl/traffic_ctl.cc @@ -218,7 +218,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 [--include-hidden] METRIC [METRIC ...]"); metric_command .add_command( "monitor", diff --git a/src/tsutil/Metrics.cc b/src/tsutil/Metrics.cc index ae96c1dbba..c339eea2df 100644 --- a/src/tsutil/Metrics.cc +++ b/src/tsutil/Metrics.cc @@ -22,6 +22,7 @@ */ #include "tsutil/Assert.h" +#include <algorithm> #include <memory> #include <mutex> #include <optional> @@ -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<Storage> _hidden_store = std::make_shared<Storage>(); + 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<Metrics::NamesAndAtomics>(); 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); @@ -166,6 +185,17 @@ Metrics::Storage::createSpan(size_t size, Metrics::MetricType type, Metrics::IdT release_assert(size <= MAX_SIZE); std::lock_guard lock(_mutex); + // On the final blob there is nowhere left to grow, so refuse a span that would fill or overflow + // it rather than letting addBlob() assert. Same intent as the guard in create(), and the same + // cost: some slots of the last blob go unused. + if (_cur_blob >= MAX_BLOBS - 1 && _cur_off + size >= MAX_SIZE) { + if (id) { + *id = 0; // Slot 0 is the reserved bad_id. + } + return {}; + } + + // A span has to be contiguous, so one that does not fit in the current blob starts a new one. if (_cur_off + size > MAX_SIZE) { addBlob(); } @@ -181,6 +211,14 @@ Metrics::Storage::createSpan(size_t size, Metrics::MetricType type, Metrics::IdT _cur_off += size; + // create() grows as soon as it consumes the last slot; do the same here. Otherwise a span ending + // exactly on the boundary leaves _cur_off at MAX_SIZE, and the next create() writes one past the + // end of the blob's name array. It also makes end() unreachable for iterator::next(), which + // wraps on ++offset == MAX_SIZE. + if (_cur_off >= MAX_SIZE) { + addBlob(); + } + return span; } @@ -226,6 +264,7 @@ namespace details struct DerivedMetric { Metrics::IdType metric; std::vector<Metrics::AtomicType *> derived_from; + Metrics::Derived::Op op{Metrics::Derived::Op::SUM}; }; struct DerivativeMetrics { @@ -239,12 +278,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); } } @@ -255,6 +312,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() { @@ -273,14 +350,25 @@ Metrics::Derived::derive(const std::initializer_list<Metrics::Derived::DerivedMe for (auto &m : metrics) { details::DerivedMetric dm{}; dm.metric = instance._create(m.derived_name, m.derived_type); + dm.op = m.op; for (auto &d : m.derived_from) { + Metrics::AtomicType *ptr = nullptr; + if (std::holds_alternative<Metrics::AtomicType *>(d)) { - dm.derived_from.push_back(std::get<Metrics::AtomicType *>(d)); + ptr = std::get<Metrics::AtomicType *>(d); } else if (std::holds_alternative<Metrics::IdType>(d)) { - dm.derived_from.push_back(instance.lookup(std::get<Metrics::IdType>(d))); - } else if (std::holds_alternative<std::string_view>(d)) { - dm.derived_from.push_back(instance.lookup(instance.lookup(std::get<std::string_view>(d)))); + auto id = std::get<Metrics::IdType>(d); + ptr = instance.valid(id) ? instance.lookup(id) : nullptr; + } else { + auto id = instance.lookup(std::get<std::string_view>(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); @@ -293,6 +381,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 cc30cb7976..960324997b 100644 --- a/src/tsutil/unit_tests/test_Metrics.cc +++ b/src/tsutil/unit_tests/test_Metrics.cc @@ -23,6 +23,14 @@ #include <catch2/catch_test_macros.hpp> +#include <algorithm> +#include <array> +#include <iterator> +#include <memory> +#include <string> +#include <thread> +#include <vector> + #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<ts::Metrics::IdType>(i))); + } m.rename(span_id + 0, "span.0"); m.rename(span_id + 1, "span.1"); @@ -144,3 +177,466 @@ 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<std::thread> threads; + std::array<Metrics::Counter::AtomicType *, N_THREADS> 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<Metrics::Counter::AtomicType *> ptrs; + std::vector<std::string> 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<Metrics::Counter::AtomicType *> sorted_ptrs = ptrs; + std::sort(sorted_ptrs.begin(), sorted_ptrs.end()); + REQUIRE(std::adjacent_find(sorted_ptrs.begin(), sorted_ptrs.end()) == sorted_ptrs.end()); +} + +TEST_CASE("Metrics span lands exactly on a blob boundary", "[libtsapi][Metrics]") +{ + // A span has to be contiguous, so createSpan(MAX_SIZE) always starts a fresh blob and then fills + // it completely, whatever the current offset was. That makes this the one span size that reaches + // the boundary case deterministically: the offset ends up at MAX_SIZE, and unlike create(), + // createSpan used not to grow a new blob afterwards. The next create() then indexed one past the + // end of the blob's name array, and end() became an id that iterator::next() can never reach + // because it wraps at ++offset == MAX_SIZE. + // + // createSpan only ever targets the published store, so this necessarily allocates there. + Metrics::IdType span_id = Metrics::NOT_FOUND; + auto span = Metrics::Counter::createSpan(Metrics::MAX_SIZE, &span_id); + + REQUIRE(span.size() == Metrics::MAX_SIZE); + REQUIRE(span_id != Metrics::NOT_FOUND); + REQUIRE(span_id != 0); // 0 is the reserved bad_id, returned only when the store cannot grow. + + // The store must still be usable, and the new metric must be a real, resolvable entry rather + // than something written past the end of a blob. + auto p = Metrics::Counter::createPtr("span.boundary.after"); + REQUIRE(p != nullptr); + + auto &m = Metrics::instance(); + auto id = m.lookup("span.boundary.after"); + REQUIRE(id != Metrics::NOT_FOUND); + REQUIRE(m.valid(id)); + + // And it must behave like any other metric. + Metrics::Counter::increment(p, 7); + REQUIRE(Metrics::Counter::load(p) == 7); + REQUIRE(Metrics::Counter::createPtr("span.boundary.after") == p); +} 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 0000000000..9dd35c1198 --- /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
