Copilot commented on code in PR #13616:
URL: https://github.com/apache/trafficserver/pull/13616#discussion_r3970508979
##########
include/tsutil/Metrics.h:
##########
@@ -231,43 +299,65 @@ class Metrics
return std::make_tuple(name, type, metric->_value.load());
}
+ /** Equality.
+ *
+ * Three way rather than a plain position compare: any exhausted iterator
equals the end
+ * sentinel, and equals any other exhausted iterator, since two of them
may have skipped a
+ * different number of unlisted slots. Two live iterators still compare by
position.
+ */
bool
operator==(const iterator &o) const
{
- return _it == o._it && std::addressof(_metrics) ==
std::addressof(o._metrics);
- }
+ if (std::addressof(_metrics) != std::addressof(o._metrics)) {
+ return false;
+ }
- bool
- operator!=(const iterator &o) const
- {
- return _it != o._it || std::addressof(_metrics) !=
std::addressof(o._metrics);
+ bool const a = at_end(), b = o.at_end();
+
+ if (a || b) {
+ return a && b;
+ }
+ return _it == o._it;
Review Comment:
Iterator equality depends on per-iterator snapshot `_bound` via `at_end()`.
If two positional iterators are created at different times (different `_bound`)
and used as a half-open range (`for (it = start; it != stop; ++it)`), `it` can
become exhausted early (per its smaller `_bound`) and then never compare equal
to `stop`, causing a non-terminating loop where `++it` can’t make progress
(because it stays `at_end()` but `it != stop` remains true). A tangible fix is
to make comparisons robust across different snapshot bounds, e.g. compute
exhaustion for comparison using a shared bound (such as `min(this->_bound,
o._bound)` for non-sentinel iterators), or enforce/signal that positional
iterators from different snapshots are not comparable (e.g. store a
snapshot/epoch token and return false / assert when mismatched) so callers
can’t accidentally form an unterminating subrange.
##########
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 assertion makes the test depend on there being at least one *listed*
metric already present in the singleton store (from static init or prior
tests). If the test suite (or another test) ends up with all metrics unlisted
at this point, this becomes order-dependent/flaky even though the behavior
under test (‘unlisted tail terminates iteration’) is still correct. Consider
making the test self-contained by creating a known listed metric in this
SECTION and asserting that iteration completes and does not include the
unlisted tail names, without requiring `distance(...) > 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]
