Copilot commented on code in PR #13616:
URL: https://github.com/apache/trafficserver/pull/13616#discussion_r3970343119
##########
src/tsutil/Metrics.cc:
##########
@@ -190,9 +194,61 @@ Metrics::Storage::type(IdType id) const
return _extractType(id);
}
+bool
+Metrics::Storage::set_listed(Metrics::IdType id, bool listed)
+{
+ if (!_is_allocated(id)) {
+ return false;
+ }
+
+ auto [blob_ix, offset] = _splitID(id);
+ Metrics::NamesAndAtomics *blob = _blobs[blob_ix].get();
+
+ // Only this bit, so a flag added later is not clobbered by unlisting or
relisting.
+ if (listed) {
+ std::get<2>(*blob)[offset].fetch_and(static_cast<uint8_t>(~UNLISTED),
MEMORY_ORDER);
+ } else {
+ std::get<2>(*blob)[offset].fetch_or(UNLISTED, MEMORY_ORDER);
+ }
+
+ return true;
+}
+
+bool
+Metrics::Storage::listed(Metrics::IdType id) const
+{
+ if (!_is_allocated(id)) {
+ return false;
+ }
+
+ auto [blob_ix, offset] = _splitID(id);
+ Metrics::NamesAndAtomics *blob = _blobs[blob_ix].get();
Review Comment:
`listed()` is `const` but stores the blob pointer in a non-const pointer
type (`Metrics::NamesAndAtomics *`). It would be more const-correct to use a
`const` pointer here (e.g., `Metrics::NamesAndAtomics const *blob`), which
prevents accidental mutation inside a `const` method and makes intent clearer.
##########
src/tsutil/unit_tests/test_Metrics.cc:
##########
@@ -707,3 +707,251 @@ TEST_CASE("Metrics id lookup is safe against concurrent
creation", "[libtsapi][M
// would mean the sweep above never left the first one.
REQUIRE(hi - lo > Metrics::MAX_SIZE);
}
+
+TEST_CASE("Metrics unlisting", "[libtsapi][Metrics]")
+{
+ auto &m = Metrics::instance();
+
+ SECTION("an unlisted metric is skipped by iteration")
+ {
+ Metrics::Counter::create("unlisted.iter.before");
+ auto target = Metrics::Counter::create("unlisted.iter.target");
+ Metrics::Counter::create("unlisted.iter.after");
+
+ REQUIRE(m.unlist(target));
+
+ bool saw_before = false, saw_target = false, saw_after = false;
+
+ for (auto &&[name, type, value] : m) {
+ saw_before |= (name == "unlisted.iter.before");
+ saw_target |= (name == "unlisted.iter.target");
+ saw_after |= (name == "unlisted.iter.after");
+ }
Review Comment:
Several new loops use structured bindings but only consume `name`, leaving
`type`/`value` unused. With common warning settings (often `-Werror` in CI),
this can trigger unused-variable warnings in unit tests. Consider iterating as
`for (auto &&entry : m)` and using `std::get<0>(entry)` (or otherwise avoid
binding unused elements).
##########
src/tsutil/unit_tests/test_Metrics.cc:
##########
@@ -707,3 +707,251 @@ TEST_CASE("Metrics id lookup is safe against concurrent
creation", "[libtsapi][M
// would mean the sweep above never left the first one.
REQUIRE(hi - lo > Metrics::MAX_SIZE);
}
+
+TEST_CASE("Metrics unlisting", "[libtsapi][Metrics]")
+{
+ auto &m = Metrics::instance();
+
+ SECTION("an unlisted metric is skipped by iteration")
+ {
+ Metrics::Counter::create("unlisted.iter.before");
+ auto target = Metrics::Counter::create("unlisted.iter.target");
+ Metrics::Counter::create("unlisted.iter.after");
+
+ REQUIRE(m.unlist(target));
+
+ bool saw_before = false, saw_target = false, saw_after = false;
+
+ for (auto &&[name, type, value] : m) {
+ saw_before |= (name == "unlisted.iter.before");
+ saw_target |= (name == "unlisted.iter.target");
+ saw_after |= (name == "unlisted.iter.after");
+ }
+
+ REQUIRE(saw_before);
+ REQUIRE_FALSE(saw_target);
+ REQUIRE(saw_after);
+ }
+
+ SECTION("creating an unlisted name again relists it")
+ {
+ auto p = Metrics::Counter::createPtr("unlisted.resurrect");
+ auto id = m.lookup("unlisted.resurrect");
+
+ Metrics::Counter::increment(p, 5);
+ REQUIRE(m.unlist(id));
+ REQUIRE_FALSE(m.listed(id));
+
+ // Same name, same id, same atomic, and the mark is gone.
+ auto p2 = Metrics::Counter::createPtr("unlisted.resurrect");
+ REQUIRE(p2 == p);
+ REQUIRE(m.lookup("unlisted.resurrect") == id);
+ REQUIRE(m.listed(id));
+
+ // Visible again, with its value intact.
+ bool found = false;
+ for (auto &&[name, type, value] : m) {
+ if (name == "unlisted.resurrect") {
+ found = true;
+ REQUIRE(value == 5);
+ }
+ }
+ REQUIRE(found);
+ }
+
+ SECTION("unlist and relist by name")
+ {
+ auto id = Metrics::Counter::create("unlisted.byname");
+
+ REQUIRE(m.unlist("unlisted.byname"));
+ REQUIRE_FALSE(m.listed(id));
+
+ REQUIRE(m.relist("unlisted.byname"));
+ REQUIRE(m.listed(id));
+
+ bool found = false;
+ for (auto &&[name, type, value] : m) {
+ found |= (name == "unlisted.byname");
+ }
+ REQUIRE(found);
+
+ // A name that was never created cannot be marked.
+ REQUIRE_FALSE(m.unlist("unlisted.byname.never.created"));
+ }
+
+ SECTION("an unlisted metric is still resolvable and still counts")
+ {
+ auto p = Metrics::Counter::createPtr("unlisted.resolvable");
+ auto id = m.lookup("unlisted.resolvable");
+
+ REQUIRE(m.unlist(id));
+
+ // Hidden from enumeration is not gone: by name, by id, and through the
atomic it is unchanged.
+ REQUIRE(m.lookup("unlisted.resolvable") == id);
+ REQUIRE(m.lookup(id) == p);
+ REQUIRE(m.valid(id));
+ REQUIRE(m.name(id) == "unlisted.resolvable");
+ REQUIRE(m.type(id) == Metrics::MetricType::COUNTER);
+
+ Metrics::Counter::increment(p, 3);
+ REQUIRE(Metrics::Counter::load(p) == 3);
+ }
+
+ SECTION("begin() skips an unlisted first slot")
+ {
+ // Slot 0 is the reserved bad_id and is what begin() would otherwise
return.
+ auto bad_id = m.lookup("proxy.process.api.metrics.bad_id");
+ REQUIRE(bad_id == 0);
+
+ REQUIRE(m.unlist(bad_id));
+ REQUIRE(std::get<0>(*m.begin()) != "proxy.process.api.metrics.bad_id");
+
+ REQUIRE(m.relist(bad_id));
+ REQUIRE(std::get<0>(*m.begin()) == "proxy.process.api.metrics.bad_id");
+ }
+
+ SECTION("an unlisted run at the end of the store terminates iteration")
+ {
+ // Skipping the last slots in the store is the case where the skip loop
has nothing unmarked
+ // left to land on.
+ constexpr int COUNT = 8;
+ std::vector<std::string> names;
+
+ names.reserve(COUNT);
+ for (int i = 0; i < COUNT; ++i) {
+ names.push_back("unlisted.tail." + std::to_string(i));
+ REQUIRE(m.unlist(Metrics::Counter::create(names[i])));
+ }
+
+ auto count = std::distance(m.begin(), m.end());
+ REQUIRE(count > 0);
+
+ for (auto &&[name, type, value] : m) {
+ for (auto const &n : names) {
+ REQUIRE(name != n);
+ }
+ }
Review Comment:
This section asserts `std::distance(m.begin(), m.end()) > 0` to ensure the
loop runs, but that couples the test to the presence of at least one listed
metric from global/suite state. To make the test self-contained and robust,
create a known listed metric in this section (an 'anchor' that is not unlisted)
and assert it is observed during iteration, instead of relying on `count > 0`.
--
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]
