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/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/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/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/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