You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@trafficserver.apache.org by "cmcfarlen (via GitHub)" <gi...@apache.org> on 2026/08/06 00:07:00 UTC
[PR] Add hidden metrics and MAX/MIN/incremental derived metric aggregation [trafficserver]
cmcfarlen opened a new pull request, #13505:
URL: https://github.com/apache/trafficserver/pull/13505
Adds two facilities to `ts::Metrics`, plus a `traffic_ctl` option to inspect the first, and fixes three pre-existing defects found along the way. A follow-up PR uses these for per-upstream-server connection metrics; this PR is independently useful and stands on its own.
### Hidden metrics
A second `Storage` instance, reached through `Metrics::hidden_instance()`, for high-cardinality intermediate values that are worth recording but not worth publishing. `Gauge::createHiddenPtr` / `Counter::createHiddenPtr` return the same correctly typed pointer as `createPtr`, so a hidden metric is read and written with the ordinary typed mutators with no cast at the call site.
A separate store rather than a per-metric "hidden" flag is deliberate: it makes hidden metrics *structurally* unreachable from the published store, so no consumer can expose one by forgetting to check a flag.
Since that also makes them hard to debug, `traffic_ctl metric match --include-hidden` lists them. The rec type bit for this (`RECT_HIDDEN_METRIC = 0x40`) sits deliberately outside `RECT_ALL` (`0x3F`), so hidden metrics are returned only when explicitly asked for and never as a side effect of a broad query.
### Derived metric aggregation
- `MAX` and `MIN` in addition to `SUM`. 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`.
- `Derived::add_source()`, for aggregates whose sources are discovered while the process runs rather than known at startup. Repeatedly calling `derive()` for one derived name does not work for this: 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 into a single entry, and re-registering an existing source is a no-op.
### Pre-existing defects fixed
- **`Storage::create()` had no exhaustion check.** Filling the last blob let the following bookkeeping call `addBlob()` and write one past the end of `_blobs`. Verified by temporarily shrinking `MAX_BLOBS`: the current code segfaults, and the `debug_assert` in `addBlob()` does not catch it because it is off by one against the access it guards (`_blobs[++_cur_blob]`) — and being a `debug_assert`, it is compiled out of release builds entirely. Now a `release_assert` against `MAX_BLOBS - 1`, with `create()` refusing the final slot and returning the reserved `bad_id`.
- **Unresolvable derived sources were not skipped.** 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, so the aggregate silently included that slot's value. Observable under `MAX`/`MIN`, where the `bad_id` value can become the winning one; under `SUM` it was hidden by `bad_id` holding zero.
- **The metrics unit tests were order dependent.** They asserted absolute metric ids and iterator positions, which only hold when the case runs first against an otherwise empty store. Any other case that creates a published metric made them fail. This lands first so the fragility is never introduced.
### Testing
- Unit tests grow from 487 assertions / 32 cases to 6208 / 36, and pass under `--order rand` across many seeds. Each fix was checked against the unfixed code first to confirm the new assertions actually discriminate.
- `tests/gold_tests/jsonrpc/metric_match_include_hidden.test.py` covers the `--include-hidden` RPC round trip end to end. This is load-bearing: the rec type also has to be accepted by the JSONRPC request decoder, which validates each requested type against a whitelist and rejects the entire request otherwise. That was invisible to every build-level check and only showed up end to end.
- Verified against a running `traffic_server` with two temporary hidden metrics registered: absent from `metric match`, present with `--include-hidden`, and absent from a broad `proxy.process` query of ~900 published metrics.
- Docs build clean under `-W` with `nitpicky = True`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add hidden metrics and MAX/MIN/incremental derived metric aggregation [trafficserver]
Posted by "bryancall (via GitHub)" <gi...@apache.org>.
bryancall commented on code in PR #13505:
URL: https://github.com/apache/trafficserver/pull/13505#discussion_r3807782523
##########
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 [--include-hidden] METRIC [METRIC ...]");
Review Comment:
The flag position now agrees with the rst, which is what I asked for. The operand name does not: this says `METRIC` while `traffic_ctl.en.rst:952` says `REGEX`, and the subcommand's own description one line up is "Get metrics matching a regular expression".
`METRIC` is the misleading one here, since `metric get` genuinely takes literal names and this reads as if `match` does too. The sibling command at line 124 spells it `traffic_ctl config match [OPTIONS] REGEX [REGEX ...]`. Suggest `traffic_ctl metric match [--include-hidden] REGEX [REGEX ...]`.
##########
doc/appendices/command-line/traffic_ctl.en.rst:
##########
@@ -949,13 +949,21 @@ traffic_ctl metric
Display the current value of the specified statistics.
.. program:: traffic_ctl metric
-.. option:: match REGEX [REGEX...]
+.. option:: match [--include-hidden] REGEX [REGEX...]
:ref:`admin_lookup_records`
Display the current values of all statistics whose names match
the given regular expression.
+.. program:: traffic_ctl metric match
+.. option:: --include-hidden
Review Comment:
This fixes the cross-reference scope, which is real and worth having, but not the thing I was complaining about. `--include-hidden` is still a column-0 `.. option::` directive, so it still renders as a top-level sibling of `get`, `match`, `describe` and `monitor` rather than as a flag belonging to `match`.
The file already establishes the pattern 45 lines up: `ssl-multicert show` nests its `.. option:: --yaml, -y` inside the body of the subcommand option. Indenting this block three spaces so it sits inside `match` gets the rendering right, and makes both `.. program::` lines here unnecessary.
##########
src/tsutil/unit_tests/test_Metrics.cc:
##########
@@ -144,3 +177,466 @@ TEST_CASE("Metrics", "[libtsapi][Metrics]")
REQUIRE(m[derivedce].load() == 10);
}
}
+
+TEST_CASE("Metrics derived ops", "[libtsapi][Metrics]")
+{
+ auto &m = Metrics::instance();
+
+ SECTION("max and min")
+ {
+ auto a = Metrics::Gauge::createPtr("op-a");
+ auto b = Metrics::Gauge::createPtr("op-b");
+ auto c = Metrics::Gauge::createPtr("op-c");
+
+ Metrics::Derived::derive({
+ {"op-sum", Metrics::MetricType::GAUGE, {a, b, c}, Metrics::Derived::Op::SUM},
+ {"op-max", Metrics::MetricType::GAUGE, {a, b, c}, Metrics::Derived::Op::MAX},
+ {"op-min", Metrics::MetricType::GAUGE, {a, b, c}, Metrics::Derived::Op::MIN},
+ });
+
+ Metrics::Gauge::store(a, 3);
+ Metrics::Gauge::store(b, 9);
+ Metrics::Gauge::store(c, 5);
+
+ Metrics::Derived::update_derived();
+
+ REQUIRE(m[m.lookup("op-sum")].load() == 17);
+ REQUIRE(m[m.lookup("op-max")].load() == 9);
+ REQUIRE(m[m.lookup("op-min")].load() == 3);
+ }
+
+ SECTION("min is not clamped by a zero seed")
+ {
+ // A zero-seeded accumulator is correct for SUM but wrong for MIN: it would report 0 here
+ // instead of the smallest source value.
+ auto a = Metrics::Gauge::createPtr("posmin-a");
+ auto b = Metrics::Gauge::createPtr("posmin-b");
+
+ Metrics::Derived::derive({
+ {"posmin", Metrics::MetricType::GAUGE, {a, b}, Metrics::Derived::Op::MIN},
+ });
+
+ Metrics::Gauge::store(a, 7);
+ Metrics::Gauge::store(b, 12);
+ Metrics::Derived::update_derived();
+
+ REQUIRE(m[m.lookup("posmin")].load() == 7);
+ }
+
+ SECTION("negative values aggregate correctly")
+ {
+ auto a = Metrics::Gauge::createPtr("neg-a");
+ auto b = Metrics::Gauge::createPtr("neg-b");
+
+ Metrics::Derived::derive({
+ {"neg-max", Metrics::MetricType::GAUGE, {a, b}, Metrics::Derived::Op::MAX},
+ {"neg-min", Metrics::MetricType::GAUGE, {a, b}, Metrics::Derived::Op::MIN},
+ });
+
+ Metrics::Gauge::store(a, -5);
+ Metrics::Gauge::store(b, -2);
+ Metrics::Derived::update_derived();
+
+ REQUIRE(m[m.lookup("neg-max")].load() == -2);
+ REQUIRE(m[m.lookup("neg-min")].load() == -5);
+ }
+
+ SECTION("a single source works for every op")
+ {
+ auto a = Metrics::Gauge::createPtr("solo-a");
+
+ Metrics::Derived::derive({
+ {"solo-sum", Metrics::MetricType::GAUGE, {a}, Metrics::Derived::Op::SUM},
+ {"solo-max", Metrics::MetricType::GAUGE, {a}, Metrics::Derived::Op::MAX},
+ {"solo-min", Metrics::MetricType::GAUGE, {a}, Metrics::Derived::Op::MIN},
+ });
+
+ Metrics::Gauge::store(a, 42);
+ Metrics::Derived::update_derived();
+
+ REQUIRE(m[m.lookup("solo-sum")].load() == 42);
+ REQUIRE(m[m.lookup("solo-max")].load() == 42);
+ REQUIRE(m[m.lookup("solo-min")].load() == 42);
+ }
+
+ // An unknown name resolves to NOT_FOUND, which _splitID masks to blob 0 / offset 0 -- the
+ // reserved bad_id slot, which holds 0. Under SUM that is invisible, so MIN is tested with
+ // strictly positive sources and MAX with strictly negative ones: in both of those an
+ // aliased-in 0 would become the winning value and change the result. They are separate
+ // sections so that one failure does not mask the other.
+ SECTION("an unresolvable source is skipped under MIN")
+ {
+ auto a = Metrics::Gauge::createPtr("guard-pos-a");
+ auto b = Metrics::Gauge::createPtr("guard-pos-b");
+
+ Metrics::Derived::derive({
+ {"guard-min", Metrics::MetricType::GAUGE, {a, "guard-does-not-exist", b}, Metrics::Derived::Op::MIN},
+ });
+
+ Metrics::Gauge::store(a, 5);
+ Metrics::Gauge::store(b, 8);
+ Metrics::Derived::update_derived();
+
+ REQUIRE(m[m.lookup("guard-min")].load() == 5); // 0 if the unknown source were included
+ }
+
+ SECTION("an unresolvable source is skipped under MAX")
+ {
+ auto a = Metrics::Gauge::createPtr("guard-neg-a");
+ auto b = Metrics::Gauge::createPtr("guard-neg-b");
+
+ Metrics::Derived::derive({
+ {"guard-max", Metrics::MetricType::GAUGE, {a, "guard-does-not-exist", b}, Metrics::Derived::Op::MAX},
+ });
+
+ Metrics::Gauge::store(a, -9);
+ Metrics::Gauge::store(b, -4);
+ Metrics::Derived::update_derived();
+
+ REQUIRE(m[m.lookup("guard-max")].load() == -4); // 0 if the unknown source were included
+ }
+
+ SECTION("an invalid source id is skipped")
+ {
+ auto a = Metrics::Gauge::createPtr("guardid-a");
+
+ Metrics::Derived::derive({
+ {"guardid-max", Metrics::MetricType::GAUGE, {a, Metrics::NOT_FOUND}, Metrics::Derived::Op::MAX},
+ });
+
+ Metrics::Gauge::store(a, -7);
+ Metrics::Derived::update_derived();
+
+ REQUIRE(m[m.lookup("guardid-max")].load() == -7);
+ }
+
+ SECTION("op defaults to SUM for backward compatibility")
+ {
+ auto a = Metrics::Counter::createPtr("dflt-a");
+ auto b = Metrics::Counter::createPtr("dflt-b");
+ Metrics::Derived::derive({
+ {"dflt-sum", Metrics::MetricType::COUNTER, {a, b}}
+ });
+ Metrics::Counter::increment(a, 2);
+ Metrics::Counter::increment(b, 4);
+ Metrics::Derived::update_derived();
+ REQUIRE(m[m.lookup("dflt-sum")].load() == 6);
+ }
+}
+
+TEST_CASE("Metrics derived add_source", "[libtsapi][Metrics]")
+{
+ auto &m = Metrics::instance();
+
+ SECTION("sources registered one at a time accumulate into a single derived metric")
+ {
+ auto a = Metrics::Gauge::createPtr("inc-a");
+ auto b = Metrics::Gauge::createPtr("inc-b");
+ auto c = Metrics::Gauge::createPtr("inc-c");
+
+ // Registered separately, as sources are discovered at runtime. derive() cannot be used this
+ // way: it appends a new entry per call, so several entries would target the same id and each
+ // update would overwrite the others with its own subset.
+ Metrics::Derived::add_source("inc-max", Metrics::MetricType::GAUGE, a, Metrics::Derived::Op::MAX);
+ Metrics::Derived::add_source("inc-max", Metrics::MetricType::GAUGE, b, Metrics::Derived::Op::MAX);
+ Metrics::Derived::add_source("inc-max", Metrics::MetricType::GAUGE, c, Metrics::Derived::Op::MAX);
+
+ Metrics::Gauge::store(a, 4);
+ Metrics::Gauge::store(b, 11);
+ Metrics::Gauge::store(c, 7);
+
+ Metrics::Derived::update_derived();
+
+ REQUIRE(m[m.lookup("inc-max")].load() == 11);
+ }
+
+ SECTION("a SUM aggregate sees every source, not just the last registered")
+ {
+ // This is what distinguishes add_source from repeated derive() calls: a SUM over all three
+ // sources rather than over whichever subset was registered last.
+ auto a = Metrics::Gauge::createPtr("incsum-a");
+ auto b = Metrics::Gauge::createPtr("incsum-b");
+ auto c = Metrics::Gauge::createPtr("incsum-c");
+
+ Metrics::Derived::add_source("incsum", Metrics::MetricType::GAUGE, a);
+ Metrics::Derived::add_source("incsum", Metrics::MetricType::GAUGE, b);
+ Metrics::Derived::add_source("incsum", Metrics::MetricType::GAUGE, c);
+
+ Metrics::Gauge::store(a, 1);
+ Metrics::Gauge::store(b, 20);
+ Metrics::Gauge::store(c, 300);
+ Metrics::Derived::update_derived();
+
+ REQUIRE(m[m.lookup("incsum")].load() == 321);
+ }
+
+ SECTION("add_source is idempotent for a repeated source")
+ {
+ auto a = Metrics::Gauge::createPtr("idem-a");
+
+ Metrics::Derived::add_source("idem-sum", Metrics::MetricType::GAUGE, a);
+ Metrics::Derived::add_source("idem-sum", Metrics::MetricType::GAUGE, a);
+ Metrics::Gauge::store(a, 6);
+ Metrics::Derived::update_derived();
+
+ REQUIRE(m[m.lookup("idem-sum")].load() == 6); // not 12
+ }
+
+ SECTION("a null source is ignored")
+ {
+ auto a = Metrics::Gauge::createPtr("null-a");
+
+ Metrics::Derived::add_source("null-sum", Metrics::MetricType::GAUGE, nullptr);
+ Metrics::Derived::add_source("null-sum", Metrics::MetricType::GAUGE, a);
+ Metrics::Gauge::store(a, 9);
+ Metrics::Derived::update_derived();
+
+ REQUIRE(m[m.lookup("null-sum")].load() == 9);
+ }
+
+ SECTION("a hidden metric can feed a published derived metric")
+ {
+ // The whole point of the facility: high cardinality sources stay hidden while only the
+ // aggregate is published.
+ auto h1 = Metrics::Gauge::createHiddenPtr("hidden.src.", "one");
+ auto h2 = Metrics::Gauge::createHiddenPtr("hidden.src.", "two");
+
+ Metrics::Derived::add_source("hidden-derived-sum", Metrics::MetricType::GAUGE, h1);
+ Metrics::Derived::add_source("hidden-derived-sum", Metrics::MetricType::GAUGE, h2);
+ Metrics::Gauge::store(h1, 21);
+ Metrics::Gauge::store(h2, 2);
+ Metrics::Derived::update_derived();
+
+ // The derived metric itself lives in the PUBLISHED store...
+ REQUIRE(m.lookup("hidden-derived-sum") != Metrics::NOT_FOUND);
+ REQUIRE(m[m.lookup("hidden-derived-sum")].load() == 23);
+ // ...while its sources remain absent from it.
+ REQUIRE(m.lookup("hidden.src.one") == Metrics::NOT_FOUND);
+ REQUIRE(m.lookup("hidden.src.two") == Metrics::NOT_FOUND);
+ }
+
+ SECTION("one source can feed two derived metrics with different ops")
+ {
+ // Commit 9 relies on this: a per-group gauge feeds both a SUM and a MAX aggregate.
+ auto a = Metrics::Gauge::createHiddenPtr("dual.src.a");
+ auto b = Metrics::Gauge::createHiddenPtr("dual.src.b");
+
+ Metrics::Derived::add_source("dual-sum", Metrics::MetricType::GAUGE, a, Metrics::Derived::Op::SUM);
+ Metrics::Derived::add_source("dual-sum", Metrics::MetricType::GAUGE, b, Metrics::Derived::Op::SUM);
+ Metrics::Derived::add_source("dual-max", Metrics::MetricType::GAUGE, a, Metrics::Derived::Op::MAX);
+ Metrics::Derived::add_source("dual-max", Metrics::MetricType::GAUGE, b, Metrics::Derived::Op::MAX);
+
+ Metrics::Gauge::store(a, 2);
+ Metrics::Gauge::store(b, 3);
+ Metrics::Derived::update_derived();
+
+ REQUIRE(m[m.lookup("dual-sum")].load() == 5);
+ REQUIRE(m[m.lookup("dual-max")].load() == 3);
+ }
+
+ SECTION("add_source works on a derived metric created by derive")
+ {
+ auto a = Metrics::Gauge::createPtr("mix-a");
+ auto b = Metrics::Gauge::createPtr("mix-b");
+
+ Metrics::Derived::derive({
+ {"mix-sum", Metrics::MetricType::GAUGE, {a}, Metrics::Derived::Op::SUM},
+ });
+ Metrics::Derived::add_source("mix-sum", Metrics::MetricType::GAUGE, b);
+
+ Metrics::Gauge::store(a, 10);
+ Metrics::Gauge::store(b, 5);
+ Metrics::Derived::update_derived();
+
+ REQUIRE(m[m.lookup("mix-sum")].load() == 15);
+ }
+}
+
+TEST_CASE("Metrics hidden store", "[libtsapi][Metrics]")
+{
+ auto &m = Metrics::instance();
+ auto &h = Metrics::hidden_instance();
+
+ SECTION("stores are separate")
+ {
+ REQUIRE(std::addressof(m) != std::addressof(h));
+
+ auto hp = Metrics::Counter::createHiddenPtr("hidden.only");
+ REQUIRE(hp != nullptr);
+
+ // Not visible in the published store, by name or by iteration.
+ REQUIRE(m.lookup("hidden.only") == Metrics::NOT_FOUND);
+ for (auto &&[name, type, value] : m) {
+ REQUIRE(name != "hidden.only");
+ }
+
+ // Visible in the hidden store.
+ REQUIRE(h.lookup("hidden.only") != Metrics::NOT_FOUND);
+ bool found = false;
+ for (auto &&[name, type, value] : h) {
+ if (name == "hidden.only") {
+ found = true;
+ }
+ }
+ REQUIRE(found);
+ }
+
+ SECTION("same name in both stores is independent")
+ {
+ auto pub = Metrics::Counter::createPtr("dual.name");
+ auto hid = Metrics::Counter::createHiddenPtr("dual.name");
+ REQUIRE(pub != hid);
+
+ // Exercised through the typed facade, which is what proves no cast is needed at a call site
+ // holding a Counter::AtomicType *.
+ Metrics::Counter::increment(pub, 3);
+ Metrics::Counter::increment(hid, 7);
+ REQUIRE(Metrics::Counter::load(pub) == 3);
+ REQUIRE(Metrics::Counter::load(hid) == 7);
+ }
+
+ SECTION("createHiddenPtr is idempotent by name")
+ {
+ auto a = Metrics::Counter::createHiddenPtr("hidden.idem");
+ auto b = Metrics::Counter::createHiddenPtr("hidden.idem");
+ REQUIRE(a == b);
+ }
+
+ SECTION("prefixed create")
+ {
+ auto p = Metrics::Counter::createHiddenPtr("pfx.", "suffix");
+ REQUIRE(p != nullptr);
+ REQUIRE(h.lookup("pfx.suffix") != Metrics::NOT_FOUND);
+ }
+
+ SECTION("the metric type is recorded in the hidden store")
+ {
+ Metrics::IdType cid{}, gid{};
+
+ REQUIRE(Metrics::Counter::createHiddenPtr("hidden.typed.counter") != nullptr);
+ REQUIRE(Metrics::Gauge::createHiddenPtr("hidden.typed.gauge") != nullptr);
+
+ cid = h.lookup("hidden.typed.counter");
+ gid = h.lookup("hidden.typed.gauge");
+ REQUIRE(cid != Metrics::NOT_FOUND);
+ REQUIRE(gid != Metrics::NOT_FOUND);
+
+ REQUIRE(h.type(cid) == Metrics::MetricType::COUNTER);
+ REQUIRE(h.type(gid) == Metrics::MetricType::GAUGE);
+ }
+
+ SECTION("hidden gauge works with the typed Gauge API")
+ {
+ auto g = Metrics::Gauge::createHiddenPtr("hidden.gauge.", "one");
+ REQUIRE(g != nullptr);
+ Metrics::Gauge::store(g, 42);
+ REQUIRE(Metrics::Gauge::load(g) == 42);
+ Metrics::Gauge::increment(g);
+ REQUIRE(Metrics::Gauge::load(g) == 43);
+ Metrics::Gauge::decrement(g);
+ REQUIRE(Metrics::Gauge::load(g) == 42);
+ }
+
+ SECTION("hidden metrics are shared across threads")
+ {
+ // Metrics is thread_local but Storage is shared, so the same name must resolve to the same
+ // atomic on every thread. This is how per-group counters are created from many event threads.
+ constexpr int N_THREADS = 4;
+ std::vector<std::thread> threads;
+ std::array<Metrics::Counter::AtomicType *, N_THREADS> ptrs{};
+
+ for (int i = 0; i < N_THREADS; ++i) {
+ threads.emplace_back([i, &ptrs]() {
+ auto p = Metrics::Counter::createHiddenPtr("hidden.threaded");
+ ptrs[i] = p;
+ Metrics::Counter::increment(p, 10);
+ });
+ }
+ for (auto &t : threads) {
+ t.join();
+ }
+
+ for (int i = 1; i < N_THREADS; ++i) {
+ REQUIRE(ptrs[i] == ptrs[0]);
+ }
+ REQUIRE(Metrics::Counter::load(ptrs[0]) == N_THREADS * 10);
+ }
+}
+
+TEST_CASE("Metrics blob growth boundary", "[libtsapi][Metrics]")
+{
+ // Storage packs metrics into fixed-size blobs (MAX_SIZE entries each). Creating more than
+ // MAX_SIZE metrics forces at least one new blob to be allocated, which is exactly where an
+ // off-by-one in the blob/offset bookkeeping would corrupt or orphan entries. Use the hidden
+ // store so this doesn't dump thousands of names into the published store that other test cases
+ // iterate over.
+ auto &h = Metrics::hidden_instance();
+ constexpr int COUNT = Metrics::MAX_SIZE + 100;
+ std::vector<Metrics::Counter::AtomicType *> ptrs;
+ std::vector<std::string> names;
+
+ ptrs.reserve(COUNT);
+ names.reserve(COUNT);
+
+ for (int i = 0; i < COUNT; ++i) {
+ names.push_back("blob.growth." + std::to_string(i));
+ auto p = Metrics::Counter::createHiddenPtr(names[i]);
+ REQUIRE(p != nullptr);
+ ptrs.push_back(p);
+ Metrics::Counter::increment(p, i);
+ }
+
+ for (int i = 0; i < COUNT; ++i) {
+ auto id = h.lookup(names[i]);
+ REQUIRE(id != Metrics::NOT_FOUND);
+ REQUIRE(h.valid(id));
+
+ // Re-creating by name must be idempotent and resolve to the exact same atomic: a blob
+ // boundary bug that aliases two entries onto the same slot, or orphans one behind the
+ // boundary, would fail this.
+ auto p2 = Metrics::Counter::createHiddenPtr(names[i]);
+ REQUIRE(p2 == ptrs[i]);
+
+ // Distinct values catch aliasing: if two logically distinct entries were mapped to the same
+ // underlying atomic, this readback would not match the index written above.
+ REQUIRE(Metrics::Counter::load(ptrs[i]) == i);
+ }
+
+ // Every pointer must be distinct: no two names should have been aliased onto the same atomic.
+ std::vector<Metrics::Counter::AtomicType *> sorted_ptrs = ptrs;
+ std::sort(sorted_ptrs.begin(), sorted_ptrs.end());
+ REQUIRE(std::adjacent_find(sorted_ptrs.begin(), sorted_ptrs.end()) == sorted_ptrs.end());
+}
+
+TEST_CASE("Metrics span lands exactly on a blob boundary", "[libtsapi][Metrics]")
+{
+ // A span has to be contiguous, so createSpan(MAX_SIZE) always starts a fresh blob and then fills
Review Comment:
Good test to add, and picking `MAX_SIZE` because it is the one size that reaches the boundary deterministically is the right instinct. Two notes.
The stated mechanism is not quite right. `createSpan(MAX_SIZE)` does not always start a fresh blob: when `_cur_off == 0` on entry, `_cur_off + size > MAX_SIZE` is `1024 > 1024`, which is false, so no `addBlob()` and the span is placed in the existing empty blob. The conclusion holds either way, so this is only comment accuracy, but the comment is the sole explanation of why `MAX_SIZE` is special and a reader who trusts it could conclude a smaller size behaves the same. The property that actually holds is that a span of `MAX_SIZE` always lands at offset 0 of an empty blob, growing one first if the current blob is partly used, and then fills it completely.
The test also does not fail deterministically against the pre-fix code. Without the trailing `addBlob()`, `_cur_off` is left at `MAX_SIZE` and the next `create()` writes `names[1024]`, one past the end of a `std::array<NameAndId, 1024>`. That is undefined behavior a sanitizer catches, but in a plain build it usually lands in adjacent storage and the REQUIREs still pass, so this stops guarding the fix in any non-sanitizer lane. Asserting that the iterator can actually reach `end()` after the span would pin the other half directly, since an offset parked at `MAX_SIZE` makes `end()` unreachable for `iterator::next()`.
##########
src/records/RecCore.cc:
##########
@@ -612,6 +612,36 @@ RecLookupMatchingRecords(unsigned rec_type, const char *match, void (*callback)(
});
}
+ if (rec_type & RECT_HIDDEN_METRIC) {
+ // Opt-in only: hidden metrics are never reachable through RECT_ALL, see RecDefs.h.
+ auto &hidden = ts::Metrics::hidden_instance();
+ // Slot 0 of every Storage is the reserved bad_id placeholder, so it exists under the same name
+ // in both stores. Skip it here, otherwise a query matching it returns two identically named
+ // records that differ only in value.
+ auto it = hidden.begin();
Review Comment:
The skip is correct and solves the duplicate. Three follow-on notes, none blocking.
Skipping slot 0 removes the last way to see the hidden store's `bad_id` counter from the CLI. `RecLookupRecord` consults only `ts::Metrics::instance()`, never `hidden_instance()`, so `metric get` cannot reach it either. That counter accumulates every failed `createHiddenPtr`, and the new `Metrics.en.rst` tells operators that unbounded hidden metric names are the likely cause of exhaustion, so the diagnostic the docs point at is now unreadable. Emitting it under a disambiguated name, appending a `.hidden` suffix for the hidden store's slot 0, would resolve the duplicate confusion and keep the signal.
Moving from a range-for to an explicit loop also changed `end()` from evaluated once to evaluated per iteration, so this now picks up entries registered by another thread mid-walk where the published loop 30 lines up does not. I checked termination and it is fine, but the two loops now have different snapshot semantics for no stated reason. `auto const stop = hidden.end();` would keep them consistent.
There is also no test for the skip, and it is implemented as manual iterator arithmetic rather than a condition inside the loop, which is the shape a later refactor back to a range-for quietly undoes. A section in `test_RecHiddenMetricLookup.cc` calling `RecLookupMatchingRecords` with `RECT_PROCESS | RECT_HIDDEN_METRIC` and a `bad_id` pattern, asserting a count of 1, is about four lines and fails pre-fix with a count of 2.
##########
src/tsutil/Metrics.cc:
##########
@@ -168,6 +187,17 @@ Metrics::Storage::createSpan(size_t size, Metrics::MetricType type, Metrics::IdT
release_assert(size <= MAX_SIZE);
ts::lock_guard lock(_mutex);
+ // On the final blob there is nowhere left to grow, so refuse a span that would fill or overflow
+ // it rather than letting addBlob() assert. Same intent as the guard in create(), and the same
+ // cost: some slots of the last blob go unused.
+ if (_cur_blob >= MAX_BLOBS - 1 && _cur_off + size >= MAX_SIZE) {
Review Comment:
This guard does not cover the case its comment describes, and the trailing `addBlob()` you added below makes a state that was previously survivable into a release abort.
`createSpan` can now advance the blob index twice in one call: once at line 201 because a span has to be contiguous, and once at line 220 when it ends exactly on the boundary. The guard only inspects `_cur_blob` as it is on entry, so it misses the case where the first advance is what puts you on the last blob.
Walking `_cur_blob == MAX_BLOBS - 2` (8190), `_cur_off == 500`, `size == MAX_SIZE`:
- line 193: `8190 >= 8191` is false, so no refusal
- line 201: `500 + 1024 > 1024`, so `addBlob()` runs, its `release_assert(8190 < 8191)` passes, leaving `_cur_blob == 8191` and `_cur_off == 0`
- line 214: `_cur_off` becomes exactly 1024
- line 220: `1024 >= 1024`, so `addBlob()` runs again and `release_assert(8191 < 8191)` fires
`release_assert` is defined outside the debug gate, so that aborts traffic_server in a release build. Sweeping `_cur_blob` in [8188, 8191] against every `_cur_off` and `size` gives exactly 1023 aborting triples, all at `_cur_blob == 8190`, `_cur_off` in [1, 1023], `size == MAX_SIZE`. The same sweep against the pre-commit body aborts zero times at 8190, so this state is newly fatal rather than a pre-existing hole.
Reachability is why this is not a blocker: it needs about 8.38M live published metrics, roughly half a gigabyte of blob storage plus an 8.4M-entry lookup map, and `createSpan` has no non-test callers. It is still worth closing, because the guard reads as complete and is not.
The obvious rewrite in terms of a target blob and offset is wrong, so I want to save you the detour. With `_cur_blob == 8191`, `_cur_off == 500`, `size == 600` it computes `target_off == 0`, fails the `>= MAX_SIZE` test, and falls through to an `addBlob()` that the current guard correctly refuses. Counting the advances instead subsumes the existing behavior:
```c++
// A span can advance the blob index twice: once because it must be contiguous, and once more
// when it ends exactly on the boundary. Refuse unless both advances are available.
const bool needs_fresh_blob = (_cur_off + size > MAX_SIZE);
const size_t end_off = (needs_fresh_blob ? 0 : _cur_off) + size;
const unsigned needed = (needs_fresh_blob ? 1u : 0u) + (end_off >= MAX_SIZE ? 1u : 0u);
if (_cur_blob + needed > MAX_BLOBS - 1) {
if (id) {
*id = 0; // Slot 0 is the reserved bad_id.
}
return {};
}
```
That turns the 1023 aborting triples into graceful bad_id refusals, never leaves `_cur_off == MAX_SIZE`, and matches the current guard on every case it already handles.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add hidden metrics and MAX/MIN/incremental derived metric aggregation [trafficserver]
Posted by "cmcfarlen (via GitHub)" <gi...@apache.org>.
cmcfarlen commented on PR #13505:
URL: https://github.com/apache/trafficserver/pull/13505#issuecomment-5345898909
Cherry-picked to 10.2.x
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add hidden metrics and MAX/MIN/incremental derived metric aggregation [trafficserver]
Posted by "cmcfarlen (via GitHub)" <gi...@apache.org>.
cmcfarlen merged PR #13505:
URL: https://github.com/apache/trafficserver/pull/13505
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add hidden metrics and MAX/MIN/incremental derived metric aggregation [trafficserver]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #13505:
URL: https://github.com/apache/trafficserver/pull/13505#discussion_r3724977511
##########
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);
Review Comment:
`RecRecord tmp;` is left uninitialized for hidden metrics, but it is passed into the YAML/JSONRPC conversion path. Because `tmp.rec_type` is set to `RECT_PROCESS`, the encoder treats it as a STAT record and will read `tmp.stat_meta` (and other fields) even though they were never initialized, leading to nondeterministic/garbage metadata in responses. Also, setting `rec_type` to `RECT_PROCESS` prevents hidden metrics from being returned if a client requests only `RECT_HIDDEN_METRIC` (64) in `rec_types`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add hidden metrics and MAX/MIN/incremental derived metric aggregation [trafficserver]
Posted by "bryancall (via GitHub)" <gi...@apache.org>.
bryancall commented on code in PR #13505:
URL: https://github.com/apache/trafficserver/pull/13505#discussion_r3805661018
##########
src/records/RecCore.cc:
##########
@@ -612,6 +612,27 @@ 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;
Review Comment:
`RecRecord tmp;` leaves `version`, `registered`, `rsb_id`, `order` and `data_default` indeterminate, and `convert<RecRecord>::encode` reads all of them unconditionally (`convert.h` lines 121-126, plus `data_default` in the RECD_INT and RECD_COUNTER cases). The path is reachable: `RecordsUtils.cc` hands each callback record straight to the encoder, so `traffic_ctl --format json metric match --include-hidden` can emit different version, rsb and order values on successive runs. Reading `registered`, an indeterminate `bool`, is undefined behavior that a MemorySanitizer build would flag.
Your comment correctly explains why no `stat_meta` block is emitted, which covers the fields Copilot was most concerned about, but it does not cover these five. `RecRecord tmp{};` zero-initializes the whole struct and costs nothing.
Not blocking, because the published-metric loop about 25 lines up has had the identical pattern for years, so this is not a regression. Since you are already in the file, worth fixing in both places.
##########
src/tsutil/Metrics.cc:
##########
@@ -64,6 +76,13 @@ Metrics::Storage::create(std::string_view name, const MetricType type)
return it->second;
}
+ // The slot is written below and the bookkeeping only then advances, calling addBlob() once
+ // _cur_off reaches MAX_SIZE. Refusing the final slot of the final blob keeps addBlob() from
+ // ever being reached in an exhausted store, at a cost of one slot out of MAX_BLOBS * MAX_SIZE.
+ if (_cur_blob >= MAX_BLOBS - 1 && _cur_off >= MAX_SIZE - 1) {
Review Comment:
The new guard covers `create()` only. `createSpan()` does `if (_cur_off + size > MAX_SIZE) addBlob();` and then `_cur_off += size`, so a span landing exactly on the boundary leaves `_cur_off == MAX_SIZE` without allocating a new blob. The next `create()` then indexes `names[1024]` on a `std::array<NameAndId, 1024>`. `end()` also becomes `_makeId(blob, MAX_SIZE)`, which `iterator::next()` can never reach because it wraps on `++offset == MAX_SIZE`.
Latent today, since the `Gauge::createSpan` and `Counter::createSpan` wrappers have no callers in the tree outside the unit tests. I raise it because it is the same class of bookkeeping off-by-one this PR is hardening against, and the new blob-growth-boundary test only drives `create()`. Either a matching guard or a comment noting `createSpan` is currently unused would close it.
##########
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
Review Comment:
`--include-hidden` is a sibling `.. option::` under `.. program:: traffic_ctl metric`, so it renders as another subcommand alongside `match`, `describe` and `get` rather than as a flag on `match`. A reader cannot tell it only applies to `match`.
The synopsis on the line above also puts the flag before the regex, while `add_example_usage` in `traffic_ctl.cc` writes `traffic_ctl metric match METRIC [--include-hidden]`. Worth making the two spellings agree.
##########
src/records/RecCore.cc:
##########
@@ -612,6 +612,27 @@ 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()) {
Review Comment:
The hidden-store iteration starts at `begin()`, which is the reserved `bad_id` slot every `Storage` constructor creates. So `traffic_ctl metric match --include-hidden bad_id` returns two records both named `proxy.process.api.metrics.bad_id`, one from each store, with different values. Cosmetic, but confusing in exactly the debugging situation this flag exists for. Skipping id 0 in this loop avoids it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add hidden metrics and MAX/MIN/incremental derived metric aggregation [trafficserver]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #13505:
URL: https://github.com/apache/trafficserver/pull/13505#discussion_r3771312217
##########
src/records/RecCore.cc:
##########
@@ -612,6 +612,27 @@ 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;
+
+ // Tag both bits so that a caller asking only for RECT_HIDDEN_METRIC passes the rec_type
+ // check the lookup callback applies to every record it is handed. Note this combination
+ // satisfies neither REC_TYPE_IS_STAT nor REC_TYPE_IS_CONFIG (both compare for equality),
+ // so the YAML encoder emits no stat_meta block for hidden metrics. That is intentional:
+ // the meta fields of this synthetic record were never populated.
+ tmp.rec_type = static_cast<RecT>(RECT_PROCESS | RECT_HIDDEN_METRIC);
+
+ tmp.name = name.data();
+ tmp.data_type = type == ts::Metrics::MetricType::COUNTER ? RECD_COUNTER : RECD_INT;
+ tmp.data.rec_int = val;
+ callback(&tmp, data);
Review Comment:
RecLookupMatchingRecords builds a synthetic RecRecord for hidden metrics, but leaves several fields (version/registered/order/rsb_id and data_default) uninitialized. The JSONRPC YAML encoder (convert<RecRecord>) reads those fields unconditionally, so this can produce nondeterministic output or undefined behavior. Value-initialize the struct and set required fields to safe defaults before invoking the callback.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org