From 6f59e6949a5ea9ee6b88c8eec2a664235262fb3e Mon Sep 17 00:00:00 2001 From: songgaoye Date: Sun, 18 Oct 2026 23:04:58 +0800 Subject: [PATCH 1/4] Fix flaky latency recorder QPS test --- test/bvar_recorder_unittest.cpp | 70 +++++++++++++++++++++------------ 1 file changed, 44 insertions(+), 26 deletions(-) diff --git a/test/bvar_recorder_unittest.cpp b/test/bvar_recorder_unittest.cpp index d9d3fd7fb3..50ece12b86 100644 --- a/test/bvar_recorder_unittest.cpp +++ b/test/bvar_recorder_unittest.cpp @@ -19,6 +19,7 @@ #include // pthread_* +#include #include #include #include @@ -222,6 +223,11 @@ class LatencyRecorderWithWindowSample : public bvar::LatencyRecorder { bool get_latency_sample(bvar::detail::Sample* sample) const { return _latency_window.get_span(sample); } + + bool get_latency_sample(time_t window_size, + bvar::detail::Sample* sample) const { + return _latency_window.get_span(window_size, sample); + } }; TEST(RecorderTest, latency_recorder_window_statistics) { @@ -276,11 +282,10 @@ TEST(RecorderTest, latency_recorder_window_statistics) { } TEST(RecorderTest, latency_recorder_qps_accuracy) { - bvar::LatencyRecorder lr1(2); // set windows size to 2s - bvar::LatencyRecorder lr2(2); - bvar::LatencyRecorder lr3(2); - bvar::LatencyRecorder lr4(2); - usleep(3000000); // wait sampler to sample 3 times + LatencyRecorderWithWindowSample lr1(2); + LatencyRecorderWithWindowSample lr2(2); + LatencyRecorderWithWindowSample lr3(2); + LatencyRecorderWithWindowSample lr4(2); auto write = [](bvar::LatencyRecorder& lr, int times) { for (int i = 0; i < times; ++i) { @@ -291,29 +296,42 @@ TEST(RecorderTest, latency_recorder_qps_accuracy) { write(lr2, 11); write(lr3, 3); write(lr4, 1); - usleep(1000000); // wait sampler to sample 1 time - - auto read = [](bvar::LatencyRecorder& lr, double exp_qps, int window_size = 0) { - int64_t qps_sum = 0; - int64_t exp_qps_int = (int64_t)exp_qps; - for (int i = 0; i < 1000; ++i) { - int64_t qps = window_size ? lr.qps(window_size): lr.qps(); - EXPECT_GE(qps, exp_qps_int - 1); - EXPECT_LE(qps, exp_qps_int + 1); - qps_sum += qps; + + auto check_qps = [](LatencyRecorderWithWindowSample& lr, int count, + time_t window_size) { + const int64_t deadline = butil::monotonic_time_us() + 5 * 1000000L; + while (butil::monotonic_time_us() < deadline) { + bvar::detail::Sample before; + if (lr.get_latency_sample(window_size, &before) && + before.data.num == count && before.time_us > 0) { + // qps() reads the window again. Only compare it with `before' + // when no sampler tick occurs between the two reads. + const int64_t qps = window_size == lr.window_size() + ? lr.qps() : lr.qps(window_size); + bvar::detail::Sample after; + if (!lr.get_latency_sample(window_size, &after) || + after.time_us != before.time_us) { + continue; + } + const double expected_qps = + before.data.num * 1000000.0 / before.time_us; + EXPECT_GE(qps, static_cast(std::floor(expected_qps))); + EXPECT_LE(qps, static_cast(std::ceil(expected_qps))); + return; + } + usleep(10000); } - double err = fabs(qps_sum / 1000.0 - exp_qps); - return err; + ADD_FAILURE() << "Recorded latencies were not sampled"; }; - ASSERT_GT(0.1, read(lr1, 10/2.0)); - ASSERT_GT(0.1, read(lr2, 11/2.0)); - ASSERT_GT(0.1, read(lr3, 3/2.0)); - ASSERT_GT(0.1, read(lr4, 1/2.0)); - - ASSERT_GT(0.1, read(lr1, 10/3.0, 3)); - ASSERT_GT(0.1, read(lr2, 11/3.0, 3)); - ASSERT_GT(0.1, read(lr3, 3/3.0, 3)); - ASSERT_GT(0.1, read(lr4, 1/3.0, 3)); + + check_qps(lr1, 10, 2); + check_qps(lr2, 11, 2); + check_qps(lr3, 3, 2); + check_qps(lr4, 1, 2); + check_qps(lr1, 10, 3); + check_qps(lr2, 11, 3); + check_qps(lr3, 3, 3); + check_qps(lr4, 1, 3); } } // namespace From b83fe38a6063947c8aec37c264e7ef55a11273f9 Mon Sep 17 00:00:00 2001 From: songgaoye Date: Sun, 25 Oct 2026 13:28:22 +0800 Subject: [PATCH 2/4] Fix latency recorder QPS window test coverage --- test/bvar_recorder_unittest.cpp | 54 ++++++++++++++++++++++----------- 1 file changed, 36 insertions(+), 18 deletions(-) diff --git a/test/bvar_recorder_unittest.cpp b/test/bvar_recorder_unittest.cpp index 50ece12b86..1266650ef1 100644 --- a/test/bvar_recorder_unittest.cpp +++ b/test/bvar_recorder_unittest.cpp @@ -23,6 +23,7 @@ #include #include #include +#include #include "butil/time.h" #include "butil/macros.h" #include "bvar/recorder.h" @@ -228,6 +229,14 @@ class LatencyRecorderWithWindowSample : public bvar::LatencyRecorder { bvar::detail::Sample* sample) const { return _latency_window.get_span(window_size, sample); } + + bool has_full_latency_window() const { + std::vector samples; + _latency_window.get_samples(&samples); + // get_samples() excludes the oldest snapshot used by get_span(). + return samples.size() + 1 == + static_cast(_latency_window.window_size()); + } }; TEST(RecorderTest, latency_recorder_window_statistics) { @@ -282,31 +291,40 @@ TEST(RecorderTest, latency_recorder_window_statistics) { } TEST(RecorderTest, latency_recorder_qps_accuracy) { - LatencyRecorderWithWindowSample lr1(2); - LatencyRecorderWithWindowSample lr2(2); - LatencyRecorderWithWindowSample lr3(2); - LatencyRecorderWithWindowSample lr4(2); + LatencyRecorderWithWindowSample lr1_2s(2); + LatencyRecorderWithWindowSample lr2_2s(2); + LatencyRecorderWithWindowSample lr3_2s(2); + LatencyRecorderWithWindowSample lr4_2s(2); + LatencyRecorderWithWindowSample lr1_3s(3); + LatencyRecorderWithWindowSample lr2_3s(3); + LatencyRecorderWithWindowSample lr3_3s(3); + LatencyRecorderWithWindowSample lr4_3s(3); auto write = [](bvar::LatencyRecorder& lr, int times) { for (int i = 0; i < times; ++i) { lr << 1; } }; - write(lr1, 10); - write(lr2, 11); - write(lr3, 3); - write(lr4, 1); + write(lr1_2s, 10); + write(lr2_2s, 11); + write(lr3_2s, 3); + write(lr4_2s, 1); + write(lr1_3s, 10); + write(lr2_3s, 11); + write(lr3_3s, 3); + write(lr4_3s, 1); auto check_qps = [](LatencyRecorderWithWindowSample& lr, int count, - time_t window_size) { + time_t window_size, bool use_default_window) { const int64_t deadline = butil::monotonic_time_us() + 5 * 1000000L; while (butil::monotonic_time_us() < deadline) { bvar::detail::Sample before; if (lr.get_latency_sample(window_size, &before) && + lr.has_full_latency_window() && before.data.num == count && before.time_us > 0) { // qps() reads the window again. Only compare it with `before' // when no sampler tick occurs between the two reads. - const int64_t qps = window_size == lr.window_size() + const int64_t qps = use_default_window ? lr.qps() : lr.qps(window_size); bvar::detail::Sample after; if (!lr.get_latency_sample(window_size, &after) || @@ -324,14 +342,14 @@ TEST(RecorderTest, latency_recorder_qps_accuracy) { ADD_FAILURE() << "Recorded latencies were not sampled"; }; - check_qps(lr1, 10, 2); - check_qps(lr2, 11, 2); - check_qps(lr3, 3, 2); - check_qps(lr4, 1, 2); - check_qps(lr1, 10, 3); - check_qps(lr2, 11, 3); - check_qps(lr3, 3, 3); - check_qps(lr4, 1, 3); + check_qps(lr1_2s, 10, 2, true); + check_qps(lr2_2s, 11, 2, true); + check_qps(lr3_2s, 3, 2, true); + check_qps(lr4_2s, 1, 2, true); + check_qps(lr1_3s, 10, 3, false); + check_qps(lr2_3s, 11, 3, false); + check_qps(lr3_3s, 3, 3, false); + check_qps(lr4_3s, 1, 3, false); } } // namespace From 20a6949bdb98d1d66f26809595e7a28d1a10a1f4 Mon Sep 17 00:00:00 2001 From: songgaoye Date: Sun, 25 Oct 2026 19:13:49 +0800 Subject: [PATCH 3/4] Stabilize latency recorder QPS test --- test/bvar_recorder_unittest.cpp | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/test/bvar_recorder_unittest.cpp b/test/bvar_recorder_unittest.cpp index 1266650ef1..422ed422e3 100644 --- a/test/bvar_recorder_unittest.cpp +++ b/test/bvar_recorder_unittest.cpp @@ -305,6 +305,28 @@ TEST(RecorderTest, latency_recorder_qps_accuracy) { lr << 1; } }; + + // Fill the windows before writing the test batch. This keeps the batch in + // a complete window for multiple sampler ticks instead of just one. + const int64_t full_window_deadline = + butil::monotonic_time_us() + 5 * 1000000L; + bool full_windows = false; + while (butil::monotonic_time_us() < full_window_deadline) { + full_windows = lr1_2s.has_full_latency_window() && + lr2_2s.has_full_latency_window() && + lr3_2s.has_full_latency_window() && + lr4_2s.has_full_latency_window() && + lr1_3s.has_full_latency_window() && + lr2_3s.has_full_latency_window() && + lr3_3s.has_full_latency_window() && + lr4_3s.has_full_latency_window(); + if (full_windows) { + break; + } + usleep(10000); + } + ASSERT_TRUE(full_windows) << "Latency windows were not fully sampled"; + write(lr1_2s, 10); write(lr2_2s, 11); write(lr3_2s, 3); From 1b2c22f41d1bbee806ae93981f3490b43e414b7b Mon Sep 17 00:00:00 2001 From: songgaoye Date: Mon, 26 Oct 2026 19:19:26 +0800 Subject: [PATCH 4/4] Stabilize QPS tests with fixed sampler histories --- src/bvar/detail/sampler.h | 1 + src/bvar/latency_recorder.cpp | 12 ++- src/bvar/latency_recorder.h | 4 + src/bvar/window.h | 1 + test/bvar_recorder_unittest.cpp | 178 +++++++++++++++++--------------- 5 files changed, 106 insertions(+), 90 deletions(-) diff --git a/src/bvar/detail/sampler.h b/src/bvar/detail/sampler.h index d120458d18..2a9815c044 100644 --- a/src/bvar/detail/sampler.h +++ b/src/bvar/detail/sampler.h @@ -182,6 +182,7 @@ class CombinerSampleSource { // sampling immune to R being destructed first. template class ReducerSampler : public Sampler { + friend class LatencyRecorderTestPeer; typedef typename butil::conditional< HasShareCombiner::value, CombinerSampleSource, diff --git a/src/bvar/latency_recorder.cpp b/src/bvar/latency_recorder.cpp index 8c9379d8c2..6db4764b15 100644 --- a/src/bvar/latency_recorder.cpp +++ b/src/bvar/latency_recorder.cpp @@ -113,10 +113,10 @@ int CDF::describe_series( return 0; } -// Return random int value with expectation = `dval' -static int64_t double_to_random_int(double dval) { +// Return random int value with expectation = `dval'. +int64_t double_to_random_int(double dval, double random) { int64_t ival = static_cast(dval); - if (dval > ival + butil::fast_rand_double()) { + if (dval > ival + random) { ival += 1; } return ival; @@ -129,7 +129,8 @@ static int64_t get_window_recorder_qps(void* arg) { if (s.time_us <= 0) { return 0; } - return double_to_random_int(s.data.num * 1000000.0 / s.time_us); + return double_to_random_int( + s.data.num * 1000000.0 / s.time_us, butil::fast_rand_double()); } static int64_t get_recorder_count(void* arg) { @@ -209,7 +210,8 @@ int64_t LatencyRecorder::qps(time_t window_size) const { if (s.time_us <= 0) { return 0; } - return detail::double_to_random_int(s.data.num * 1000000.0 / s.time_us); + return detail::double_to_random_int( + s.data.num * 1000000.0 / s.time_us, butil::fast_rand_double()); } int LatencyRecorder::expose(const butil::StringPiece& prefix1, diff --git a/src/bvar/latency_recorder.h b/src/bvar/latency_recorder.h index 1cc09f51e3..8c820600b9 100644 --- a/src/bvar/latency_recorder.h +++ b/src/bvar/latency_recorder.h @@ -33,6 +33,10 @@ typedef Window RecorderWindow; typedef Window, SERIES_IN_SECOND> MaxWindow; typedef Window PercentileWindow; +// Round |dval| up with probability equal to its fractional part. |random| +// must be in [0, 1); keeping it explicit makes the rule deterministic to test. +int64_t double_to_random_int(double dval, double random); + // NOTE: Always use int64_t in the interfaces no matter what the impl. is. class CDF : public Variable { diff --git a/src/bvar/window.h b/src/bvar/window.h index d701dcc08d..b1adb1ad75 100644 --- a/src/bvar/window.h +++ b/src/bvar/window.h @@ -43,6 +43,7 @@ namespace detail { // Just for constructor reusing of Window<> template class WindowBase : public Variable { + friend class LatencyRecorderTestPeer; public: typedef typename R::value_type value_type; typedef typename R::sampler_type sampler_type; diff --git a/test/bvar_recorder_unittest.cpp b/test/bvar_recorder_unittest.cpp index 422ed422e3..bf6544632d 100644 --- a/test/bvar_recorder_unittest.cpp +++ b/test/bvar_recorder_unittest.cpp @@ -30,6 +30,52 @@ #include "bvar/latency_recorder.h" #include +namespace bvar { +namespace detail { + +// Give the real QPS APIs a fixed history without racing the sampling thread. +// The recorder's original sampler remains registered and is restored before +// destruction. Only this replacement sampler's queue is modified by the test. +class LatencyRecorderTestPeer { +public: + LatencyRecorderTestPeer(RecorderWindow& window, IntRecorder& recorder, + int count) + : _window(window), _original(window._sampler), + _sampler(new IntRecorder::sampler_type(&recorder)) { + CHECK_EQ(0, _sampler->set_window_size(4)); + _sampler->take_sample(); // Allocate room for five snapshots. + _sampler->_q.clear(); + for (int i = 0; i <= 4; ++i) { + CHECK(_sampler->_q.push(Sample( + i == 4 ? Stat(count, count) : Stat(), i * 1000000L))); + } + _sampler->add_borrower(); + _window._sampler = _sampler.get(); + // Keep the original borrow alive until it is restored. + } + + ~LatencyRecorderTestPeer() { + _window._sampler = _original; + _sampler->remove_borrower(); + } + + void advance() { + Sample next = *_sampler->_q.bottom(); + next.time_us += 1000000L; + _sampler->_q.elim_push(next); + } + +private: + LatencyRecorderTestPeer(const LatencyRecorderTestPeer&) = delete; + LatencyRecorderTestPeer& operator=(const LatencyRecorderTestPeer&) = delete; + RecorderWindow& _window; + IntRecorder::sampler_type* _original; + std::unique_ptr _sampler; +}; + +} // namespace detail +} // namespace bvar + namespace { #if !WITH_BABYLON_COUNTER TEST(RecorderTest, test_complement) { @@ -230,12 +276,9 @@ class LatencyRecorderWithWindowSample : public bvar::LatencyRecorder { return _latency_window.get_span(window_size, sample); } - bool has_full_latency_window() const { - std::vector samples; - _latency_window.get_samples(&samples); - // get_samples() excludes the oldest snapshot used by get_span(). - return samples.size() + 1 == - static_cast(_latency_window.window_size()); + std::unique_ptr fixed_history(int count) { + return std::unique_ptr( + new bvar::detail::LatencyRecorderTestPeer(_latency_window, _latency, count)); } }; @@ -291,87 +334,52 @@ TEST(RecorderTest, latency_recorder_window_statistics) { } TEST(RecorderTest, latency_recorder_qps_accuracy) { - LatencyRecorderWithWindowSample lr1_2s(2); - LatencyRecorderWithWindowSample lr2_2s(2); - LatencyRecorderWithWindowSample lr3_2s(2); - LatencyRecorderWithWindowSample lr4_2s(2); - LatencyRecorderWithWindowSample lr1_3s(3); - LatencyRecorderWithWindowSample lr2_3s(3); - LatencyRecorderWithWindowSample lr3_3s(3); - LatencyRecorderWithWindowSample lr4_3s(3); - - auto write = [](bvar::LatencyRecorder& lr, int times) { - for (int i = 0; i < times; ++i) { - lr << 1; - } - }; - - // Fill the windows before writing the test batch. This keeps the batch in - // a complete window for multiple sampler ticks instead of just one. - const int64_t full_window_deadline = - butil::monotonic_time_us() + 5 * 1000000L; - bool full_windows = false; - while (butil::monotonic_time_us() < full_window_deadline) { - full_windows = lr1_2s.has_full_latency_window() && - lr2_2s.has_full_latency_window() && - lr3_2s.has_full_latency_window() && - lr4_2s.has_full_latency_window() && - lr1_3s.has_full_latency_window() && - lr2_3s.has_full_latency_window() && - lr3_3s.has_full_latency_window() && - lr4_3s.has_full_latency_window(); - if (full_windows) { - break; - } - usleep(10000); - } - ASSERT_TRUE(full_windows) << "Latency windows were not fully sampled"; - - write(lr1_2s, 10); - write(lr2_2s, 11); - write(lr3_2s, 3); - write(lr4_2s, 1); - write(lr1_3s, 10); - write(lr2_3s, 11); - write(lr3_3s, 3); - write(lr4_3s, 1); - - auto check_qps = [](LatencyRecorderWithWindowSample& lr, int count, - time_t window_size, bool use_default_window) { - const int64_t deadline = butil::monotonic_time_us() + 5 * 1000000L; - while (butil::monotonic_time_us() < deadline) { - bvar::detail::Sample before; - if (lr.get_latency_sample(window_size, &before) && - lr.has_full_latency_window() && - before.data.num == count && before.time_us > 0) { - // qps() reads the window again. Only compare it with `before' - // when no sampler tick occurs between the two reads. - const int64_t qps = use_default_window - ? lr.qps() : lr.qps(window_size); - bvar::detail::Sample after; - if (!lr.get_latency_sample(window_size, &after) || - after.time_us != before.time_us) { - continue; - } - const double expected_qps = - before.data.num * 1000000.0 / before.time_us; - EXPECT_GE(qps, static_cast(std::floor(expected_qps))); - EXPECT_LE(qps, static_cast(std::ceil(expected_qps))); - return; + for (int count : {10, 11, 3, 1}) { + for (time_t window : {2, 3}) { + SCOPED_TRACE(::testing::Message() << "count=" << count + << " window=" << window); + // Default qps() uses two seconds; explicit qps(3) must override + // a four-second default. + LatencyRecorderWithWindowSample lr(window == 2 ? 2 : 4); + auto history = lr.fixed_history(count); + bvar::detail::Sample sample; + ASSERT_TRUE(lr.get_latency_sample(window, &sample)); + ASSERT_EQ(count, sample.data.num); + ASSERT_EQ(window * 1000000L, sample.time_us); + + const double expected = count / static_cast(window); + int64_t total = 0; + const int trials = 10000; + for (int i = 0; i < trials; ++i) { + const int64_t value = window == 2 ? lr.qps() : lr.qps(window); + ASSERT_GE(value, static_cast(std::floor(expected))); + ASSERT_LE(value, static_cast(std::ceil(expected))); + total += value; + } + // Exercise rounding through the public APIs, including low QPS. + // For independent uniform draws, Hoeffding bounds the chance of + // this tolerance failing at less than 2*exp(-50) per case. + EXPECT_NEAR(expected, total / static_cast(trials), 0.05); + + // Move the batch out of the requested window. Duration stays the + // same while count becomes zero, reproducing the rollover case. + for (time_t i = 0; i < window; ++i) { + history->advance(); } - usleep(10000); + ASSERT_TRUE(lr.get_latency_sample(window, &sample)); + EXPECT_EQ(0, sample.data.num); + EXPECT_EQ(window * 1000000L, sample.time_us); + EXPECT_EQ(0, window == 2 ? lr.qps() : lr.qps(window)); } - ADD_FAILURE() << "Recorded latencies were not sampled"; - }; - - check_qps(lr1_2s, 10, 2, true); - check_qps(lr2_2s, 11, 2, true); - check_qps(lr3_2s, 3, 2, true); - check_qps(lr4_2s, 1, 2, true); - check_qps(lr1_3s, 10, 3, false); - check_qps(lr2_3s, 11, 3, false); - check_qps(lr3_3s, 3, 3, false); - check_qps(lr4_3s, 1, 3, false); + } +} + +TEST(RecorderTest, latency_recorder_qps_rounding) { + // A random value equal to the fractional part rounds down. + EXPECT_EQ(10, bvar::detail::double_to_random_int(10.25, 0.25)); + EXPECT_EQ(11, bvar::detail::double_to_random_int(10.25, 0.24)); + EXPECT_EQ(10, bvar::detail::double_to_random_int(10.0, 0.0)); + EXPECT_EQ(11, bvar::detail::double_to_random_int(10.75, 0.1)); } } // namespace