From ccb5a6f35683f8b38818b1dabe135f18fa660a99 Mon Sep 17 00:00:00 2001 From: Chris McFarlen Date: Mon, 3 Aug 2026 12:31:07 -0500 Subject: [PATCH 01/10] 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. --- src/tsutil/unit_tests/test_Metrics.cc | 37 +++++++++++++++++++++++---- 1 file changed, 32 insertions(+), 5 deletions(-) diff --git a/src/tsutil/unit_tests/test_Metrics.cc b/src/tsutil/unit_tests/test_Metrics.cc index cc30cb79768..007d283418b 100644 --- a/src/tsutil/unit_tests/test_Metrics.cc +++ b/src/tsutil/unit_tests/test_Metrics.cc @@ -23,6 +23,8 @@ #include +#include + #include "tsutil/Metrics.h" using ts::Metrics; @@ -38,18 +40,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 +94,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"); From e30075cee4563a31373ed9cf473b8a379c4a53cd Mon Sep 17 00:00:00 2001 From: Chris McFarlen Date: Mon, 3 Aug 2026 12:34:32 -0500 Subject: [PATCH 02/10] 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. --- include/tsutil/Metrics.h | 58 +++++++++++++ src/tsutil/Metrics.cc | 10 +++ src/tsutil/unit_tests/test_Metrics.cc | 115 ++++++++++++++++++++++++++ 3 files changed, 183 insertions(+) diff --git a/include/tsutil/Metrics.h b/include/tsutil/Metrics.h index 4f47fd5485f..89f69a1df89 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) { diff --git a/src/tsutil/Metrics.cc b/src/tsutil/Metrics.cc index 92bcb3bf6b0..667597ef9df 100644 --- a/src/tsutil/Metrics.cc +++ b/src/tsutil/Metrics.cc @@ -42,6 +42,16 @@ 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! { diff --git a/src/tsutil/unit_tests/test_Metrics.cc b/src/tsutil/unit_tests/test_Metrics.cc index 007d283418b..cf626220705 100644 --- a/src/tsutil/unit_tests/test_Metrics.cc +++ b/src/tsutil/unit_tests/test_Metrics.cc @@ -23,7 +23,11 @@ #include +#include #include +#include +#include +#include #include "tsutil/Metrics.h" using ts::Metrics; @@ -171,3 +175,114 @@ TEST_CASE("Metrics", "[libtsapi][Metrics]") REQUIRE(m[derivedce].load() == 10); } } + +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); + } +} From dd1354819c83575ecff04d3845d6fb7801f2430c Mon Sep 17 00:00:00 2001 From: Chris McFarlen Date: Mon, 3 Aug 2026 12:47:19 -0500 Subject: [PATCH 03/10] 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. --- src/tsutil/Metrics.cc | 10 +++++- src/tsutil/unit_tests/test_Metrics.cc | 47 +++++++++++++++++++++++++++ 2 files changed, 56 insertions(+), 1 deletion(-) diff --git a/src/tsutil/Metrics.cc b/src/tsutil/Metrics.cc index 667597ef9df..9a7a4f2d492 100644 --- a/src/tsutil/Metrics.cc +++ b/src/tsutil/Metrics.cc @@ -58,7 +58,8 @@ 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; @@ -74,6 +75,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); diff --git a/src/tsutil/unit_tests/test_Metrics.cc b/src/tsutil/unit_tests/test_Metrics.cc index cf626220705..4b1b2ac3b59 100644 --- a/src/tsutil/unit_tests/test_Metrics.cc +++ b/src/tsutil/unit_tests/test_Metrics.cc @@ -23,9 +23,11 @@ #include +#include #include #include #include +#include #include #include @@ -286,3 +288,48 @@ TEST_CASE("Metrics hidden store", "[libtsapi][Metrics]") 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()); +} From df472635027c2c70c3a19bf5a8ad9e54604cbffc Mon Sep 17 00:00:00 2001 From: Chris McFarlen Date: Mon, 3 Aug 2026 12:57:34 -0500 Subject: [PATCH 04/10] 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. --- .../command-line/traffic_ctl.en.rst | 9 +++- include/records/RecDefs.h | 5 +- include/shared/rpc/RPCRequests.h | 2 + src/mgmt/rpc/handlers/records/Records.cc | 1 + src/records/RecCore.cc | 16 ++++++ src/traffic_ctl/CtrlCommands.cc | 10 ++-- src/traffic_ctl/CtrlCommands.h | 5 +- src/traffic_ctl/traffic_ctl.cc | 4 +- .../metric_match_include_hidden.test.py | 49 +++++++++++++++++++ 9 files changed, 93 insertions(+), 8 deletions(-) create mode 100644 tests/gold_tests/jsonrpc/metric_match_include_hidden.test.py 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/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/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/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 From bb94ce194d26ae31a4bbb50b84fd50b93f5dd612 Mon Sep 17 00:00:00 2001 From: Chris McFarlen Date: Mon, 3 Aug 2026 13:00:08 -0500 Subject: [PATCH 05/10] 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. --- include/tsutil/Metrics.h | 4 ++ src/tsutil/Metrics.cc | 29 ++++++-- src/tsutil/unit_tests/test_Metrics.cc | 95 +++++++++++++++++++++++++++ 3 files changed, 124 insertions(+), 4 deletions(-) diff --git a/include/tsutil/Metrics.h b/include/tsutil/Metrics.h index 89f69a1df89..f5ffba9b630 100644 --- a/include/tsutil/Metrics.h +++ b/include/tsutil/Metrics.h @@ -645,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}; }; /** diff --git a/src/tsutil/Metrics.cc b/src/tsutil/Metrics.cc index 9a7a4f2d492..14133f97f34 100644 --- a/src/tsutil/Metrics.cc +++ b/src/tsutil/Metrics.cc @@ -22,6 +22,7 @@ */ #include "tsutil/Assert.h" +#include #include #include #include @@ -246,6 +247,7 @@ namespace details struct DerivedMetric { Metrics::IdType metric; std::vector derived_from; + Metrics::Derived::Op op{Metrics::Derived::Op::SUM}; }; struct DerivativeMetrics { @@ -259,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); } } @@ -293,6 +313,7 @@ Metrics::Derived::derive(const std::initializer_list(d)) { diff --git a/src/tsutil/unit_tests/test_Metrics.cc b/src/tsutil/unit_tests/test_Metrics.cc index 4b1b2ac3b59..54f669f531d 100644 --- a/src/tsutil/unit_tests/test_Metrics.cc +++ b/src/tsutil/unit_tests/test_Metrics.cc @@ -178,6 +178,101 @@ TEST_CASE("Metrics", "[libtsapi][Metrics]") } } +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); + } + + 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 hidden store", "[libtsapi][Metrics]") { auto &m = Metrics::instance(); From 15e1d8e34d528aa57b9497c5aa37a85d540157fd Mon Sep 17 00:00:00 2001 From: Chris McFarlen Date: Mon, 3 Aug 2026 14:45:26 -0500 Subject: [PATCH 06/10] 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. --- src/tsutil/Metrics.cc | 18 +++++++--- src/tsutil/unit_tests/test_Metrics.cc | 51 +++++++++++++++++++++++++++ 2 files changed, 65 insertions(+), 4 deletions(-) diff --git a/src/tsutil/Metrics.cc b/src/tsutil/Metrics.cc index 14133f97f34..62afa92181d 100644 --- a/src/tsutil/Metrics.cc +++ b/src/tsutil/Metrics.cc @@ -316,12 +316,22 @@ 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); diff --git a/src/tsutil/unit_tests/test_Metrics.cc b/src/tsutil/unit_tests/test_Metrics.cc index 54f669f531d..95892641655 100644 --- a/src/tsutil/unit_tests/test_Metrics.cc +++ b/src/tsutil/unit_tests/test_Metrics.cc @@ -259,6 +259,57 @@ TEST_CASE("Metrics derived ops", "[libtsapi][Metrics]") 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"); From e3fd471e7c5f29c407f1acea238c922026fe6ac6 Mon Sep 17 00:00:00 2001 From: Chris McFarlen Date: Mon, 3 Aug 2026 14:47:24 -0500 Subject: [PATCH 07/10] 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. --- include/tsutil/Metrics.h | 15 +++ src/tsutil/Metrics.cc | 29 ++++++ src/tsutil/unit_tests/test_Metrics.cc | 128 ++++++++++++++++++++++++++ 3 files changed, 172 insertions(+) diff --git a/include/tsutil/Metrics.h b/include/tsutil/Metrics.h index f5ffba9b630..a1d4d2fe2b6 100644 --- a/include/tsutil/Metrics.h +++ b/include/tsutil/Metrics.h @@ -664,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/tsutil/Metrics.cc b/src/tsutil/Metrics.cc index 62afa92181d..6f078726b50 100644 --- a/src/tsutil/Metrics.cc +++ b/src/tsutil/Metrics.cc @@ -295,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() { @@ -344,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 95892641655..7dc8b749641 100644 --- a/src/tsutil/unit_tests/test_Metrics.cc +++ b/src/tsutil/unit_tests/test_Metrics.cc @@ -324,6 +324,134 @@ TEST_CASE("Metrics derived ops", "[libtsapi][Metrics]") } } +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(); From 80217521e8cf203c0285e021307a9b4af9db37db Mon Sep 17 00:00:00 2001 From: Chris McFarlen Date: Mon, 3 Aug 2026 16:11:26 -0500 Subject: [PATCH 08/10] 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. --- .../internal-libraries/Metrics.en.rst | 198 ++++++++++++++++++ .../internal-libraries/index.en.rst | 1 + 2 files changed, 199 insertions(+) create mode 100644 doc/developer-guide/internal-libraries/Metrics.en.rst 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 From ec2f6bf265314f7584110b535a52a5f7f4b87eda Mon Sep 17 00:00:00 2001 From: Chris McFarlen Date: Mon, 3 Aug 2026 14:52:21 -0500 Subject: [PATCH 09/10] net: fix and always maintain the per group peak connection count Two problems with the per group peak, which is reported as the "max" field of the connection tracker group dump. update_max_count() made a single compare_exchange_weak attempt with no retry, so a racing update, or a spurious failure of the weak form, silently discarded the sample. Retry until the value is stored or is no longer the largest. It was also only called when a maximum was configured. With metrics enabled and no configured maximum, the count was reserved and then discarded, so the peak stayed at zero. Pass the reserved count through in that case too. --- include/iocore/net/ConnectionTracker.h | 10 +++++++--- src/proxy/http/HttpSM.cc | 4 +++- 2 files changed, 10 insertions(+), 4 deletions(-) diff --git a/include/iocore/net/ConnectionTracker.h b/include/iocore/net/ConnectionTracker.h index ae3691fdbe2..238746be522 100644 --- a/include/iocore/net/ConnectionTracker.h +++ b/include/iocore/net/ConnectionTracker.h @@ -489,9 +489,13 @@ ConnectionTracker::TxnState::clear() inline void ConnectionTracker::TxnState::update_max_count(int count) { - auto cmax = _g->_count_max.load(); - if (count > cmax) { - _g->_count_max.compare_exchange_weak(cmax, count); + auto cmax = _g->_count_max.load(std::memory_order_relaxed); + + while (count > cmax) { + if (_g->_count_max.compare_exchange_weak(cmax, count, std::memory_order_relaxed, std::memory_order_relaxed)) { + break; + } + // cmax was reloaded by the failed exchange; retry if we are still larger. } } diff --git a/src/proxy/http/HttpSM.cc b/src/proxy/http/HttpSM.cc index 64173d0f3c9..d89ef892b4d 100644 --- a/src/proxy/http/HttpSM.cc +++ b/src/proxy/http/HttpSM.cc @@ -5866,7 +5866,9 @@ HttpSM::do_http_server_open(bool raw, bool only_direct) } else if (t_state.txn_conf->connection_tracker_config.server_min > 0 || t_state.http_config_param->global_connection_tracker_config.metric_enabled) { auto &ct_state = t_state.outbound_conn_track_state; - ct_state.reserve(); + // Feed the count through as well, otherwise the group's peak stays at zero whenever metrics + // are enabled without a configured maximum. + ct_state.update_max_count(ct_state.reserve()); } // We did not manage to get an existing session and need to open a new connection From b096ed757808c46ee31afa3f602ab1ec2f49bf75 Mon Sep 17 00:00:00 2001 From: Chris McFarlen Date: Wed, 5 Aug 2026 18:14:42 -0500 Subject: [PATCH 10/10] net: per server connection metrics via hidden and derived metrics Aggregate the per group connection counts into per hostname metrics, using hidden metrics as the inputs and a derived metric for the aggregation, so the work happens on the stat sync task rather than on every connection. metric_enabled becomes a level rather than a flag: 0 disables, 1 publishes only the per hostname aggregates, 2 also mirrors the per group metrics into the published store. The per group metrics themselves are always created in the hidden store, so changing the level at runtime only changes what is registered for publication and never has to move a metric between stores. Aggregates are registered only for the 'both' match type, the only one with more than one group per hostname; for 'host' the group name is already the bare hostname and the two would collide on a single name. current_connection_max is the largest current count among a hostname's groups, sampled, rather than a monotone peak, so it falls again as traffic drains and a maximum over time can be computed downstream. --- doc/admin-guide/files/records.yaml.en.rst | 29 ++ .../statistics/core/http-connection.en.rst | 58 ++++ include/iocore/net/ConnectionTracker.h | 54 +++- src/iocore/net/ConnectionTracker.cc | 43 ++- src/records/RecordsConfig.cc | 2 +- .../per_server_connection_max.test.py | 256 ++++++++++++++++-- 6 files changed, 408 insertions(+), 34 deletions(-) diff --git a/doc/admin-guide/files/records.yaml.en.rst b/doc/admin-guide/files/records.yaml.en.rst index a34387bb4d3..4ba1e11e31f 100644 --- a/doc/admin-guide/files/records.yaml.en.rst +++ b/doc/admin-guide/files/records.yaml.en.rst @@ -2007,6 +2007,35 @@ Origin Server Connect Attempts the connection. Useful when the origin supports keep-alive, removing the time needed to set up a new connection from the next request at the expense of added (inactive) connections. +.. ts:cv:: CONFIG proxy.config.http.per_server.connection.metric_enabled INT 0 + :reloadable: + + Publish per upstream server connection metrics. These metrics are dynamically named, one set per + upstream server group or hostname, so the number of them scales with the number of distinct + upstream servers seen. See :ref:`per-server-connection-metrics`. + + ===== ====================================================================================== + Value Effect + ===== ====================================================================================== + ``0`` No per server connection metrics. + ``1`` Publish only the per hostname aggregate metrics. The per group metrics from which the + aggregates are computed exist internally but are not published. + ``2`` Publish the per hostname aggregates and the per group metrics. + ===== ====================================================================================== + + Level ``2`` can produce a very large number of metrics when the + :ts:cv:`match type ` includes the address or + port, since there is then one set per address and port rather than one per hostname. + +.. ts:cv:: CONFIG proxy.config.http.per_server.connection.metric_prefix STRING NULL + :reloadable: + + An optional prefix inserted into the per server connection metric names, between the fixed + ``proxy.process.http.per_server..`` portion of the name and the upstream server group + or hostname. Useful to distinguish metrics from separate + :ts:cv:`match ` configurations sharing the same + upstream. See :ref:`per-server-connection-metrics`. + .. ts:cv:: CONFIG proxy.config.http.connect_attempts_rr_retries INT 3 :reloadable: :overridable: diff --git a/doc/admin-guide/monitoring/statistics/core/http-connection.en.rst b/doc/admin-guide/monitoring/statistics/core/http-connection.en.rst index 7b3b23940f0..78a75e11f8f 100644 --- a/doc/admin-guide/monitoring/statistics/core/http-connection.en.rst +++ b/doc/admin-guide/monitoring/statistics/core/http-connection.en.rst @@ -201,6 +201,64 @@ HTTP Connection Current number of TCP connections for tunnels where the far end is the server, except for those counted by ``proxy.process.tunnel.current_server_connections_tls`` +.. _per-server-connection-metrics: + +Per Server Connection Metrics +----------------------------- + +Unlike the metrics above these do not have fixed names. They are created dynamically, one set per +upstream server group as defined by :ts:cv:`proxy.config.http.per_server.connection.match`, and, +when the match type is ``both``, one aggregate set per hostname. Whether any of them are published, +and at what granularity, is controlled by +:ts:cv:`proxy.config.http.per_server.connection.metric_enabled`. An optional +:ts:cv:`proxy.config.http.per_server.connection.metric_prefix` can be inserted into the names. + +Per group names are ``proxy.process.http.per_server..``, where ```` depends on +the match type: an IP address, an ``address:port`` pair, a hostname, or, for ``both``, +``.``. Per hostname names are +``proxy.process.http.per_server..``. Aggregates exist only for match type +``both``, because that is the only match type with more than one group per hostname; for match type +``host`` the group name is already the bare hostname, so an aggregate would carry the same name as +the single group it summarises. + +For a group, ```` is one of: + +current_connection + Gauge. The number of connections currently open to the group. + +total_connection + Counter. The total number of connections ever opened to the group. Never decreases. + +blocked_connection + Counter. The total number of connection attempts to the group blocked by + :ts:cv:`proxy.config.http.per_server.connection.max`. Never decreases. + +For a hostname aggregate, ```` is one of those three, each summed across the groups of that +hostname, plus: + +current_connection_max + Gauge. The largest ``current_connection`` value among the groups of that hostname at the moment + of sampling, so the maximum rather than the sum of the groups' current counts. This is useful + because :ts:cv:`proxy.config.http.per_server.connection.max` is enforced per group rather than + per hostname, so the busiest group is what determines whether connections are about to be + blocked. Like ``current_connection`` it rises and falls with traffic and is not a high-water + mark. There is no per group ``current_connection_max``; it exists only as a hostname aggregate. + +Every published per server metric is recomputed periodically, currently every 5 seconds, rather than +on every connection event, so a reader sees a value up to that interval old. This is true of the +hostname aggregates and, at level ``2``, of the published per group metrics as well: those are +mirrored from the internal ones by the same periodic mechanism, not written as connections open and +close. It applies to ``current_connection_max`` too, which reports the maximum across groups as of +the last sample rather than a running peak. To obtain the peak over a longer window, compute a +maximum over time from this gauge in the monitoring system. + +At :ts:cv:`metric_enabled ` level ``1`` the +per group metrics still exist internally, since the aggregates are computed from them, but are not +published. They can be listed with ``traffic_ctl metric match per_server --include-hidden``, which +reads them directly and so is not subject to the sampling delay above. That visibility is intended +for debugging and is not a stable interface: the existence, granularity and naming of the per group +metrics may change independently of the published aggregates. + HTTP/2 ------ diff --git a/include/iocore/net/ConnectionTracker.h b/include/iocore/net/ConnectionTracker.h index 238746be522..5041c278274 100644 --- a/include/iocore/net/ConnectionTracker.h +++ b/include/iocore/net/ConnectionTracker.h @@ -82,16 +82,36 @@ class ConnectionTracker MatchType server_match{MATCH_IP}; ///< Server match type. }; + /** Levels for the @c metric_enabled configuration. + * + * The per group metrics are always created, in the hidden metric store, whenever metrics are + * enabled at all. What varies by level is what gets published: + * - @c METRIC_LEVEL_NONE: no per group metrics are created and nothing is published. + * - @c METRIC_LEVEL_HOST: the per group metrics stay hidden; the per hostname aggregates + * (computed across the groups of a hostname by a @c Derived metric) are published. + * - @c METRIC_LEVEL_GROUP: as above, plus the per group metrics are also mirrored into the + * published store. + * + * Keeping the per group metrics in the hidden store at every level means changing the level at + * runtime is only a change of what is registered for publication, with no metric to migrate + * between the two stores. + */ + enum MetricLevel { + METRIC_LEVEL_NONE = 0, ///< No per server metrics. + METRIC_LEVEL_HOST = 1, ///< Only the per hostname aggregate metrics are published. + METRIC_LEVEL_GROUP = 2, ///< The per hostname aggregates and the per group metrics are published. + }; + /** Static configuration values. */ struct GlobalConfig { GlobalConfig() = default; GlobalConfig(GlobalConfig const &); GlobalConfig &operator=(GlobalConfig const &); - std::chrono::seconds client_alert_delay{60}; ///< Alert delay in seconds. - std::chrono::seconds server_alert_delay{60}; ///< Alert delay in seconds. - bool metric_enabled{false}; ///< Enabling per server metrics. - std::string metric_prefix; ///< Per server metric prefix. + std::chrono::seconds client_alert_delay{60}; ///< Alert delay in seconds. + std::chrono::seconds server_alert_delay{60}; ///< Alert delay in seconds. + MetricLevel metric_enabled{METRIC_LEVEL_NONE}; ///< Which per server metrics to publish. + std::string metric_prefix; ///< Per server metric prefix. swoc::IPRangeSet client_exempt_list; ///< The set of IP addresses to not block due client connection counting. mutable ts::bravo::shared_mutex client_exempt_list_mutex; ///< Protects client_exempt_list from concurrent access. }; @@ -145,7 +165,8 @@ class ConnectionTracker std::atomic _in_queue{0}; ///< # of connections queued, waiting for a connection. std::atomic _last_alert{0}; ///< Absolute time of the last alert. - // Recording data as metrics + // Recording data as metrics. These are always in the hidden metric store when created; see + // @c MetricLevel for how they are published. ts::Metrics::Gauge::AtomicType *_count_metric = nullptr; ts::Metrics::Counter::AtomicType *_count_total_metric = nullptr; ts::Metrics::Counter::AtomicType *_blocked_metric = nullptr; @@ -171,6 +192,20 @@ class ConnectionTracker std::time_t get_last_alert_epoch_time() const; static std::string metric_name(const Key &key, std::string_view fqdn, std::string metric_prefix); + /** Name of the metric which aggregates a value across all groups of a hostname. + * + * Only @c MATCH_BOTH groups have more than one group per hostname. For @c MATCH_HOST there is + * exactly one group per hostname, so an aggregate would be over a set of one, and + * @c Group::metric_name already returns the FQDN alone for that match type - identical to what + * this would return, so publishing both would collide on one name. + * + * @param key The group key. + * @param fqdn The full FQDN. + * @param metric_prefix The configured metric prefix. + * @return The metric name, or an empty string if @a key is not @c MATCH_BOTH. + */ + static std::string host_metric_name(const Key &key, std::string_view fqdn, std::string metric_prefix); + /// Release the reference count to this group and remove it from the /// group table if it is no longer referenced. void release(); @@ -433,6 +468,15 @@ ConnectionTracker::Group::metric_name(const Key &key, std::string_view fqdn, std return metric_prefix.empty() ? std::move(metric_name) : metric_prefix + "." + metric_name; } +inline std::string +ConnectionTracker::Group::host_metric_name(const Key &key, std::string_view fqdn, std::string metric_prefix) +{ + if (MATCH_BOTH != key._match_type) { + return {}; // Only MATCH_BOTH has more than one group per hostname to aggregate across. + } + return metric_prefix.empty() ? std::string(fqdn) : metric_prefix + "." + std::string(fqdn); +} + inline bool ConnectionTracker::TxnState::is_active() const { diff --git a/src/iocore/net/ConnectionTracker.cc b/src/iocore/net/ConnectionTracker.cc index 45ce7e60f07..e8d4fc1eee5 100644 --- a/src/iocore/net/ConnectionTracker.cc +++ b/src/iocore/net/ConnectionTracker.cc @@ -26,6 +26,8 @@ #include "records/RecCore.h" #include "swoc/IPAddr.h" +#include + using namespace std::literals; ConnectionTracker::TableSingleton ConnectionTracker::_inbound_table; @@ -156,7 +158,9 @@ Config_Update_Conntrack_Metric_Enabled(const char * /* name ATS_UNUSED */, RecDa auto config = static_cast(cookie); if (RECD_INT == dtype) { - config->metric_enabled = data.rec_int; + auto level = std::clamp(static_cast(data.rec_int), static_cast(ConnectionTracker::METRIC_LEVEL_NONE), + static_cast(ConnectionTracker::METRIC_LEVEL_GROUP)); + config->metric_enabled = static_cast(level); return true; } return false; @@ -440,11 +444,40 @@ ConnectionTracker::Group::Group(DirectionType direction, Key const &key, std::st { Metrics::Gauge::increment(net_rsb.connection_tracker_table_size); // only add metrics for server connections - if (_global_config->metric_enabled && direction == DirectionType::OUTBOUND) { + if (_global_config->metric_enabled != METRIC_LEVEL_NONE && direction == DirectionType::OUTBOUND) { std::string _metric_name = metric_name(key, fqdn, _global_config->metric_prefix); - _count_metric = Metrics::Gauge::createPtr("proxy.process.http.per_server.current_connection.", _metric_name); - _count_total_metric = Metrics::Counter::createPtr("proxy.process.http.per_server.total_connection.", _metric_name); - _blocked_metric = Metrics::Counter::createPtr("proxy.process.http.per_server.blocked_connection.", _metric_name); + // Per group metrics always live in the hidden store. metric_enabled controls what is published + // from them (see MetricLevel), not whether they exist. + _count_metric = Metrics::Gauge::createHiddenPtr("proxy.process.http.per_server.current_connection.", _metric_name); + _count_total_metric = Metrics::Counter::createHiddenPtr("proxy.process.http.per_server.total_connection.", _metric_name); + _blocked_metric = Metrics::Counter::createHiddenPtr("proxy.process.http.per_server.blocked_connection.", _metric_name); + + // Only MATCH_BOTH groups have siblings sharing a hostname to aggregate across. + std::string _host_metric_name = host_metric_name(key, fqdn, _global_config->metric_prefix); + if (!_host_metric_name.empty()) { + Metrics::Derived::add_source("proxy.process.http.per_server.current_connection." + _host_metric_name, + Metrics::MetricType::GAUGE, _count_metric, Metrics::Derived::Op::SUM); + Metrics::Derived::add_source("proxy.process.http.per_server.total_connection." + _host_metric_name, + Metrics::MetricType::COUNTER, _count_total_metric, Metrics::Derived::Op::SUM); + Metrics::Derived::add_source("proxy.process.http.per_server.blocked_connection." + _host_metric_name, + Metrics::MetricType::COUNTER, _blocked_metric, Metrics::Derived::Op::SUM); + // The largest current count among this hostname's groups, sampled. Deliberately taken over + // the instantaneous gauge rather than each group's all time peak, so the value falls again + // and a maximum over time can be computed by whatever scrapes it. + Metrics::Derived::add_source("proxy.process.http.per_server.current_connection_max." + _host_metric_name, + Metrics::MetricType::GAUGE, _count_metric, Metrics::Derived::Op::MAX); + } + + if (_global_config->metric_enabled >= METRIC_LEVEL_GROUP) { + // Mirror the per group metrics into the published store under their own name. A single + // source SUM is an identity: the published value always equals the hidden source. + Metrics::Derived::add_source("proxy.process.http.per_server.current_connection." + _metric_name, Metrics::MetricType::GAUGE, + _count_metric, Metrics::Derived::Op::SUM); + Metrics::Derived::add_source("proxy.process.http.per_server.total_connection." + _metric_name, Metrics::MetricType::COUNTER, + _count_total_metric, Metrics::Derived::Op::SUM); + Metrics::Derived::add_source("proxy.process.http.per_server.blocked_connection." + _metric_name, Metrics::MetricType::COUNTER, + _blocked_metric, Metrics::Derived::Op::SUM); + } if (dbg_ctl.on()) { swoc::LocalBufferWriter<256> w; diff --git a/src/records/RecordsConfig.cc b/src/records/RecordsConfig.cc index 3ad853798e4..c52253a0a34 100644 --- a/src/records/RecordsConfig.cc +++ b/src/records/RecordsConfig.cc @@ -395,7 +395,7 @@ static constexpr RecordElement RecordsConfig[] = , {RECT_CONFIG, "proxy.config.http.per_server.connection.min", RECD_INT, "0", RECU_DYNAMIC, RR_NULL, RECC_STR, "^[0-9]+$", RECA_NULL} , - {RECT_CONFIG, "proxy.config.http.per_server.connection.metric_enabled", RECD_INT, "0", RECU_DYNAMIC, RR_NULL, RECC_STR, "[0-1]", RECA_NULL} + {RECT_CONFIG, "proxy.config.http.per_server.connection.metric_enabled", RECD_INT, "0", RECU_DYNAMIC, RR_NULL, RECC_STR, "^[0-2]$", RECA_NULL} , {RECT_CONFIG, "proxy.config.http.per_server.connection.metric_prefix", RECD_STRING, "", RECU_DYNAMIC, RR_NULL, RECC_NULL, nullptr, RECA_NULL} , diff --git a/tests/gold_tests/origin_connection/per_server_connection_max.test.py b/tests/gold_tests/origin_connection/per_server_connection_max.test.py index e7bad788ab3..9dc9b12f204 100644 --- a/tests/gold_tests/origin_connection/per_server_connection_max.test.py +++ b/tests/gold_tests/origin_connection/per_server_connection_max.test.py @@ -1,5 +1,6 @@ ''' -Verify the behavior of proxy.config.http.per_server.connection.max. +Verify the behavior of proxy.config.http.per_server.connection.max and the per server +connection metrics (proxy.config.http.per_server.connection.metric_enabled). ''' # Licensed to the Apache Software Foundation (ASF) under one # or more contributor license agreements. See the NOTICE file @@ -22,6 +23,17 @@ Test.SkipIf(Condition.CurlUsingUnixDomainSocket()) +# The per hostname aggregates are derived metrics, recomputed once per +# REC_RAW_STAT_SYNC_INTERVAL_MS (5000ms, src/records/P_RecDefs.h) on an ET_TASK thread. Nothing in +# records.yaml drives that interval, so a test has to sleep comfortably longer than one sync period +# before reading an aggregate rather than trying to configure a faster one. Reading too early +# silently compares against zeros. +_STAT_SYNC_WAIT_SECONDS: int = 6 + +# NOTE: assigning to a Streams attribute REPLACES any tester already set for that stream +# (TesterSet.Assign), so every assertion after the first on the same stream must use '+=' or it +# silently discards the earlier ones. + class PerServerConnectionMaxTest: """Define an object to test our max origin connection behavior.""" @@ -54,7 +66,10 @@ def _configure_trafficserver(self) -> None: 'proxy.config.diags.debug.enabled': 1, 'proxy.config.diags.debug.tags': 'http|conn_track', 'proxy.config.http.per_server.connection.max': self._origin_max_connections, - 'proxy.config.http.per_server.connection.metric_enabled': 1, + # Level 2 (METRIC_LEVEL_GROUP): the match here is 'port', which never has more than + # one group per hostname, so there is no aggregate to read and the per group + # metrics themselves have to be published to be checked below. + 'proxy.config.http.per_server.connection.metric_enabled': 2, 'proxy.config.http.per_server.connection.metric_prefix': 'foo', 'proxy.config.http.per_server.connection.match': 'port', }) @@ -64,16 +79,26 @@ def _configure_trafficserver(self) -> None: def _test_metrics(self) -> None: """Use traffic_ctl to test metrics.""" + group_name = f'foo.127.0.0.1:{self._server.Variables.http_port}' + tr = Test.AddTestRun("Check connection metrics") - tr.Processes.Default.Command = 'traffic_ctl metric match per_server' + # At level 2 the per group metrics are published by mirroring the hidden ones through a + # derived metric, so a sync tick has to pass before they carry a value. + tr.Processes.Default.Command = f'sleep {_STAT_SYNC_WAIT_SECONDS}; traffic_ctl metric match per_server' tr.Processes.Default.ReturnCode = 0 tr.Processes.Default.Env = self._ts.Env + tr.Processes.Default.TimeOut = _STAT_SYNC_WAIT_SECONDS + 30 tr.Processes.Default.Streams.All = Testers.ContainsExpression( - f'per_server.total_connection.foo.127.0.0.1:{self._server.Variables.http_port} 4', - 'incorrect statistic return, or possible error.') - tr.Processes.Default.Streams.All = Testers.ContainsExpression( - f'per_server.blocked_connection.foo.127.0.0.1:{self._server.Variables.http_port} 1', - 'incorrect statistic return, or possible error.') + f'per_server.total_connection.{group_name} 4', 'incorrect statistic return, or possible error.') + tr.Processes.Default.Streams.All += Testers.ExcludesExpression( + 'INVALID_INCOMING_DATA', 'The metric query must not be rejected.') + tr.Processes.Default.Streams.All += Testers.ContainsExpression( + f'per_server.blocked_connection.{group_name} 1', 'incorrect statistic return, or possible error.') + + # A 'port' match has one group per address:port and no hostname, so no aggregate should be + # registered for it at all. + tr.Processes.Default.Streams.All += Testers.ExcludesExpression( + 'per_server.current_connection_max.', 'A non-"both" match type must not register a hostname aggregate.') def run(self) -> None: """Configure the TestRun.""" @@ -88,16 +113,28 @@ def run(self) -> None: class ConnectMethodTest: - """Test our max origin connection behavior with CONNECT traffic.""" + """Test our max origin connection behavior with CONNECT traffic. + + Also covers the two publication levels of proxy.config.http.per_server.connection.metric_enabled: + - 1 (METRIC_LEVEL_HOST): only the per hostname aggregate is published; the per group metrics + stay hidden and are visible only with --include-hidden. + - 2 (METRIC_LEVEL_GROUP): the per hostname aggregate is published, and the per group metrics + are also mirrored into the published store. + + The match here defaults to 'both' and there is exactly one group for this hostname, so the + aggregate is a trivial sum over that single group. MultiGroupAggregateTest below covers the + case where an aggregate genuinely spans more than one group. + """ _process_counter: int = 0 _client_counter: int = 0 - def __init__(self, max_conn) -> None: + def __init__(self, max_conn, metric_level=1) -> None: """Configure the server processes in preparation for the TestRun.""" + self._metric_level = metric_level self._configure_dns() self._configure_origin_server() - self._configure_trafficserver(max_conn) + self._configure_trafficserver(max_conn, metric_level) ConnectMethodTest._process_counter += 1 def _configure_dns(self) -> None: @@ -108,8 +145,8 @@ def _configure_origin_server(self) -> None: """Configure the httpbin origin server.""" self._server = Test.MakeHttpBinServer(f"server_{ConnectMethodTest._process_counter}") - def _configure_trafficserver(self, max_conn) -> None: - self._ts = Test.MakeATSProcess("ts2_" + str(max_conn)) + def _configure_trafficserver(self, max_conn, metric_level) -> None: + self._ts = Test.MakeATSProcess(f"ts2_{max_conn}_{metric_level}") self._ts.Disk.records_config.update( { @@ -119,7 +156,7 @@ def _configure_trafficserver(self, max_conn) -> None: 'proxy.config.diags.debug.tags': 'http|dns|hostdb|conn_track', 'proxy.config.http.server_ports': f"{self._ts.Variables.port} {self._ts.Variables.uds_path}", 'proxy.config.http.connect_ports': f"{self._server.Variables.Port}", - 'proxy.config.http.per_server.connection.metric_enabled': 1, + 'proxy.config.http.per_server.connection.metric_enabled': metric_level, 'proxy.config.http.per_server.connection.max': max_conn, }) @@ -136,17 +173,47 @@ def _configure_client_with_slow_response(self, tr) -> 'Test.Process': return p def _test_metrics(self, blocked) -> None: - """Use traffic_ctl to test metrics.""" + """Use traffic_ctl to test metrics, honoring the configured publication level.""" + host_name = 'www.this.origin.com' + group_name = f'{host_name}.127.0.0.1:{self._server.Variables.Port}' + tr = Test.AddTestRun("Check connection metrics") - tr.Processes.Default.Command = 'traffic_ctl metric match per_server; sleep 2' + tr.Processes.Default.Command = f'sleep {_STAT_SYNC_WAIT_SECONDS}; traffic_ctl metric match per_server' tr.Processes.Default.ReturnCode = 0 tr.Processes.Default.Env = self._ts.Env + tr.Processes.Default.TimeOut = _STAT_SYNC_WAIT_SECONDS + 30 + + # The per hostname aggregate is published at every non-zero level. tr.Processes.Default.Streams.All = Testers.ContainsExpression( - f'per_server.total_connection.www.this.origin.com.127.0.0.1:{self._server.Variables.Port} 5', - 'incorrect statistic return, or possible error.') - tr.Processes.Default.Streams.All = Testers.ContainsExpression( - f'per_server.blocked_connection.www.this.origin.com.127.0.0.1:{self._server.Variables.Port} {blocked}', - 'incorrect statistic return, or possible error.') + f'per_server.total_connection.{host_name} 5', 'incorrect statistic return, or possible error.') + tr.Processes.Default.Streams.All += Testers.ContainsExpression( + f'per_server.blocked_connection.{host_name} {blocked}', 'incorrect statistic return, or possible error.') + + if self._metric_level >= 2: + # METRIC_LEVEL_GROUP additionally mirrors the per group metrics into the published store. + tr.Processes.Default.Streams.All += Testers.ContainsExpression( + f'per_server.total_connection.{group_name} 5', 'The per group metric should be published at METRIC_LEVEL_GROUP.') + else: + # METRIC_LEVEL_HOST keeps the per group metrics hidden, so none of the three per group + # names may appear in a normal query. current_connection_max is not among them: it only + # ever exists as a hostname aggregate, never per group. + for counter in ('current_connection', 'total_connection', 'blocked_connection'): + tr.Processes.Default.Streams.All += Testers.ExcludesExpression( + f'per_server.{counter}.{group_name} ', f'per_server.{counter}.{group_name} must stay hidden at level 1.') + + # The per group metrics must be visible with --include-hidden at either level. This is also + # the end to end test for that traffic_ctl option. + tr2 = Test.AddTestRun("Check hidden per group connection metrics") + tr2.Processes.Default.Command = 'traffic_ctl metric match per_server --include-hidden' + tr2.Processes.Default.ReturnCode = 0 + tr2.Processes.Default.Env = self._ts.Env + # No sleep needed: the hidden per group metrics are written directly on each connection, + # unlike the derived aggregates. + tr2.Processes.Default.Streams.All = Testers.ContainsExpression( + f'per_server.total_connection.{group_name} 5', + 'The per group metric should be visible with --include-hidden at any level.') + tr2.Processes.Default.Streams.All += Testers.ExcludesExpression( + 'INVALID_INCOMING_DATA', 'The --include-hidden query must not be rejected by the RPC decoder.') def run(self, blocked, gold_file) -> None: """Verify per_server.connection.max with CONNECT traffic.""" @@ -179,6 +246,149 @@ def run(self, blocked, gold_file) -> None: self._test_metrics(blocked) +class MultiGroupAggregateTest: + """Verify a per hostname aggregate that genuinely spans more than one group. + + The other tests here resolve a hostname to a single 127.0.0.1:port, so their "aggregate" is + trivially a set of one. Here two remap rules point at the same hostname ('multi.origin.com') + on two different origin ports, so under match 'both' the connection tracker creates two + distinct groups sharing one host aggregate. The two groups are given different concurrency so + the SUM and the MAX are distinguishable from each other. + + current_connection and current_connection_max are instantaneous gauges recomputed from the live + per group values every ~5s, so they rise and fall with traffic rather than remembering a peak. + Observing a non-zero value therefore requires holding connections open across a sync tick. The + most robust assertion, and the one that actually distinguishes this instantaneous behavior from + a monotone peak, is that both gauges return to 0 once traffic drains and another tick passes. + """ + + _process_counter: int = 0 + _client_counter: int = 0 + + # Concurrent slow requests per group. Deliberately different so SUM (5) and MAX (3) differ. + _group_a_concurrency: int = 2 + _group_b_concurrency: int = 3 + + # How long each request holds its connection open. Must comfortably exceed + # _STAT_SYNC_WAIT_SECONDS so a sync tick is guaranteed to land while the connections are still + # open. NOTE: httpbin clamps /delay/ to 10 seconds, so the effective hold is min(this, 10); + # the waits below are derived from this value and tolerate that clamp. + _hold_seconds: int = 12 + + def __init__(self) -> None: + """Configure the test processes in preparation for the TestRun.""" + self._configure_dns() + self._configure_origin_servers() + self._configure_trafficserver() + MultiGroupAggregateTest._process_counter += 1 + + def _configure_dns(self) -> None: + """Configure a nameserver for the test.""" + self._dns = Test.MakeDNServer(f"magg_dns_{MultiGroupAggregateTest._process_counter}", default='127.0.0.1') + + def _configure_origin_servers(self) -> None: + """Configure the two httpbin origins which stand in for two groups of one hostname.""" + self._server_a = Test.MakeHttpBinServer(f"magg_server_a_{MultiGroupAggregateTest._process_counter}") + self._server_b = Test.MakeHttpBinServer(f"magg_server_b_{MultiGroupAggregateTest._process_counter}") + + def _configure_trafficserver(self) -> None: + """Configure Traffic Server with two remap rules to the same hostname on different ports.""" + self._ts = Test.MakeATSProcess(f"magg_ts_{MultiGroupAggregateTest._process_counter}") + self._ts.Disk.records_config.update( + { + 'proxy.config.dns.nameservers': f"127.0.0.1:{self._dns.Variables.Port}", + 'proxy.config.dns.resolv_conf': 'NULL', + 'proxy.config.diags.debug.enabled': 1, + 'proxy.config.diags.debug.tags': 'http|dns|hostdb|conn_track', + 'proxy.config.http.per_server.connection.metric_enabled': 1, + 'proxy.config.http.per_server.connection.match': 'both', + }) + self._ts.Disk.remap_config.AddLines( + [ + f"map http://multi.origin.com/a/ http://multi.origin.com:{self._server_a.Variables.Port}/", + f"map http://multi.origin.com/b/ http://multi.origin.com:{self._server_b.Variables.Port}/", + ]) + + def _make_slow_client(self, tr, path) -> 'Test.Process': + """Configure a client which makes a slow request through one of the two remapped groups.""" + p = tr.Processes.Process(f'magg_client_{MultiGroupAggregateTest._client_counter}') + MultiGroupAggregateTest._client_counter += 1 + tr.MakeCurlCommand( + f"-v --fail -s -x 127.0.0.1:{self._ts.Variables.port} " + f"'http://multi.origin.com/{path}/delay/{MultiGroupAggregateTest._hold_seconds}'", + p=p, + ts=self._ts) + return p + + def _test_metrics_while_held(self) -> None: + """While the slow requests are still in flight, verify the live gauges reflect them.""" + total = MultiGroupAggregateTest._group_a_concurrency + MultiGroupAggregateTest._group_b_concurrency + group_max = max(MultiGroupAggregateTest._group_a_concurrency, MultiGroupAggregateTest._group_b_concurrency) + + tr = Test.AddTestRun("Check the host aggregate spans both groups while connections are held open") + tr.Processes.Default.Command = f'sleep {_STAT_SYNC_WAIT_SECONDS}; traffic_ctl metric match per_server' + tr.Processes.Default.ReturnCode = 0 + tr.Processes.Default.Env = self._ts.Env + tr.Processes.Default.TimeOut = _STAT_SYNC_WAIT_SECONDS + 30 + tr.Processes.Default.Streams.All = Testers.ContainsExpression( + f'per_server.total_connection.multi.origin.com {total}', + 'The host aggregate total_connection should be the SUM across both groups ' + f'({MultiGroupAggregateTest._group_a_concurrency} + {MultiGroupAggregateTest._group_b_concurrency}).') + tr.Processes.Default.Streams.All += Testers.ContainsExpression( + f'per_server.current_connection.multi.origin.com {total}', + 'While held open, the host aggregate current_connection should be the SUM of the ' + 'currently open connections across both groups.') + tr.Processes.Default.Streams.All += Testers.ContainsExpression( + f'per_server.current_connection_max.multi.origin.com {group_max}', + 'While held open, current_connection_max should be the largest single group current ' + 'count (MAX), not the sum across the two groups.') + + def _test_metrics_after_drain(self) -> None: + """After traffic drains and a further sync tick passes, both live gauges must read 0. + + This validates the behavior the design exists to provide: an instantaneous gauge, unlike a + monotone peak, comes back down. + """ + tr = Test.AddTestRun("Check the host aggregate drains back to 0 after traffic stops") + # The slow requests are already _STAT_SYNC_WAIT_SECONDS old by now; wait for the rest of + # their hold time and then for another sync tick to observe the drop to 0. + wait = max(0, MultiGroupAggregateTest._hold_seconds - _STAT_SYNC_WAIT_SECONDS) + _STAT_SYNC_WAIT_SECONDS + tr.Processes.Default.Command = f'sleep {wait}; traffic_ctl metric match per_server' + tr.Processes.Default.ReturnCode = 0 + tr.Processes.Default.Env = self._ts.Env + tr.Processes.Default.TimeOut = wait + 30 + tr.Processes.Default.Streams.All = Testers.ContainsExpression( + 'per_server.current_connection.multi.origin.com 0', + 'Once all connections close, the host aggregate current_connection must drain to 0.') + tr.Processes.Default.Streams.All += Testers.ContainsExpression( + 'per_server.current_connection_max.multi.origin.com 0', + 'Once all connections close, current_connection_max must also come back down to 0: it ' + 'is a live gauge, not a monotone peak.') + + def run(self) -> None: + """Drive concurrent traffic through both groups, then check the aggregate metrics.""" + tr = Test.AddTestRun() + tr.Processes.Default.StartBefore(self._dns) + tr.Processes.Default.StartBefore(self._server_a) + tr.Processes.Default.StartBefore(self._server_b) + tr.Processes.Default.StartBefore(self._ts) + + clients = [self._make_slow_client(tr, 'a') for _ in range(MultiGroupAggregateTest._group_a_concurrency)] + clients += [self._make_slow_client(tr, 'b') for _ in range(MultiGroupAggregateTest._group_b_concurrency)] + for p in clients: + tr.Processes.Default.StartBefore(p) + + # Let the slow requests connect and overlap before checking anything; they stay open for + # _hold_seconds from about this point. + tr.Processes.Default.Command = 'sleep 1' + tr.Processes.Default.ReturnCode = 0 + tr.Processes.Default.TimeOut = 30 + + self._test_metrics_while_held() + self._test_metrics_after_drain() + + PerServerConnectionMaxTest().run() -ConnectMethodTest(3).run(blocked=2, gold_file="gold/two_503_congested.gold") -ConnectMethodTest(0).run(blocked=0, gold_file="gold/two_200_ok.gold") +ConnectMethodTest(3, metric_level=1).run(blocked=2, gold_file="gold/two_503_congested.gold") +ConnectMethodTest(0, metric_level=2).run(blocked=0, gold_file="gold/two_200_ok.gold") +MultiGroupAggregateTest().run()