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