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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
