chenBright commented on code in PR #3651:
URL: https://github.com/apache/brpc/pull/3651#discussion_r4373658992
##########
test/bvar_recorder_unittest.cpp:
##########
@@ -276,44 +291,87 @@ 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_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);
- 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;
+
+ // 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;
}
- double err = fabs(qps_sum / 1000.0 - exp_qps);
- return err;
+ 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<bvar::Stat> 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<bvar::Stat> after;
+ if (!lr.get_latency_sample(window_size, &after) ||
+ after.time_us != before.time_us) {
+ continue;
Review Comment:
time_us represents the window duration, not a sampling timestamp. A
deterministic probe reproduces a rollover where the count changes from 10 to 0
while both durations remain 2 seconds. The guard accepts the changed window,
causing the subsequent QPS assertion to fail.
##########
test/bvar_recorder_unittest.cpp:
##########
@@ -276,44 +291,87 @@ 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_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);
- 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;
+
+ // 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;
}
- double err = fabs(qps_sum / 1000.0 - exp_qps);
- return err;
+ 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<bvar::Stat> 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<bvar::Stat> 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<int64_t>(std::floor(expected_qps)));
+ EXPECT_LE(qps, static_cast<int64_t>(std::ceil(expected_qps)));
Review Comment:
Checking one result against floor(expected_qps) and ceil(expected_qps)
allows systematic underestimation. Replacing randomized rounding with
unconditional truncation passes the new test but fails the original test.
##########
test/bvar_recorder_unittest.cpp:
##########
@@ -276,44 +291,87 @@ 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_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);
- 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;
+
+ // 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;
}
- double err = fabs(qps_sum / 1000.0 - exp_qps);
- return err;
+ 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<bvar::Stat> 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<bvar::Stat> 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<int64_t>(std::floor(expected_qps)));
+ EXPECT_LE(qps, static_cast<int64_t>(std::ceil(expected_qps)));
+ return;
+ }
+ usleep(10000);
+ }
+ 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_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);
Review Comment:
These calls request 3 seconds from recorders constructed with a 3-second
default. Changing qps(window_size) to ignore its argument and return qps()
still passes the new test but fails the original test.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
