You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@brpc.apache.org by "wasphin (via GitHub)" <gi...@apache.org> on 2026/10/05 04:55:34 UTC
[PR] Fix timing assumptions in multidimensional latency recorder test (brpc)
wasphin opened a new pull request, #3581:
URL: https://github.com/apache/brpc/pull/3581
### What problem does this PR solve?
Issue Number: N/A
Problem Summary:
`MultiDimensionTest.mlatencyrecorder` can fail when its one-second sleep
ends before the asynchronous sampler updates the latency windows. The
Bazel CI job below observed latency 0 instead of 4:
https://github.com/apache/brpc/actions/runs/37211025654/job/111462085596
The QPS assertion also assumes that seven recorded requests yield at least
seven requests per second, although QPS depends on the actual sampling
interval and randomized rounding.
### What is changed and the side effects?
Changed:
- Poll both latency windows until samples are available, with a documented
five-second deadline to tolerate scheduling delays on busy CI runners.
- Check positive QPS instead of assuming a minimum rate of seven.
- Keep the fatal pointer assertion and use nonfatal assertions for
independent metric checks.
- Restore modified flags automatically with `FlagSaver`, including when
a fatal assertion returns from the test.
Side effects:
- Performance effects: The test stops waiting once both windows have
samples; delayed or failed sampling can extend the wait to five seconds.
- Breaking backward compatibility: None; this change affects only a test.
---
### Check List:
- The changed test source compiled with Bazel on macOS. Linking the test
binary was blocked by unresolved `bthread_*` symbols, so runtime tests
could not execute. The existing CMake configuration also referenced a
missing Google Test source directory. GitHub Actions will report tests.
- This fixes an existing test and adds no production behavior.
- The change follows the repository's Code of Conduct.
--
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: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] Fix timing assumptions in multidimensional latency recorder test (brpc)
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #3581:
URL: https://github.com/apache/brpc/pull/3581#discussion_r4180851475
##########
test/bvar_multi_dimension_unittest.cpp:
##########
@@ -420,17 +410,24 @@ TEST_F(MultiDimensionTest, mlatencyrecorder) {
bvar::LatencyRecorder* my_latencyrecorder = my_mlatencyrecorder.get_stats(labels_value);
ASSERT_TRUE(my_latencyrecorder);
*my_latencyrecorder << 1 << 2 << 3 << 4 << 5 << 6 << 7;
- sleep(1);
- ASSERT_EQ(4, my_latencyrecorder->latency());
- ASSERT_EQ(7, my_latencyrecorder->max_latency());
- ASSERT_LE(7, my_latencyrecorder->qps());
- ASSERT_EQ(7, my_latencyrecorder->count());
-
- GFLAGS_NAMESPACE::SetCommandLineOption("bvar_dump_interval", old_bvar_dump_interval.c_str());
- GFLAGS_NAMESPACE::SetCommandLineOption("mbvar_dump", old_mbvar_dump.c_str());
- GFLAGS_NAMESPACE::SetCommandLineOption("bvar_latency_p1", old_bvar_latency_p1.c_str());
- GFLAGS_NAMESPACE::SetCommandLineOption("bvar_latency_p2", old_bvar_latency_p2.c_str());
- GFLAGS_NAMESPACE::SetCommandLineOption("bvar_latency_p3", old_bvar_latency_p3.c_str());
+ EXPECT_EQ(7, my_latencyrecorder->count());
+
+ // Sampling runs once per second. Allow five seconds for scheduling
+ // delays on busy CI runners while keeping the wait bounded.
+ const int64_t sampling_timeout_us = 5 * 1000000L;
+ const int64_t deadline =
+ butil::monotonic_time_us() + sampling_timeout_us;
+ while ((my_latencyrecorder->latency() == 0 ||
+ my_latencyrecorder->max_latency() == 0) &&
+ butil::monotonic_time_us() < deadline) {
+ usleep(10000);
+ }
+
+ EXPECT_EQ(4, my_latencyrecorder->latency());
+ EXPECT_EQ(7, my_latencyrecorder->max_latency());
+ // QPS uses the actual sampling interval and randomized rounding.
+ // Seven requests can yield fewer than seven requests per second.
+ EXPECT_GT(my_latencyrecorder->qps(), 0);
Review Comment:
These post-loop reads are not the values that satisfied the polling condition. With a one-second window and no further writes, the next sampler tick makes `max_latency()` and `qps()` zero; if this test is descheduled after leaving the loop, the same timing failure remains. Cache each nonzero metric while polling (including QPS) and assert the cached values.
--
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: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] Fix timing assumptions in multidimensional latency recorder test (brpc)
Posted by "wwbmmm (via GitHub)" <gi...@apache.org>.
wwbmmm merged PR #3581:
URL: https://github.com/apache/brpc/pull/3581
--
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: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org