Copilot commented on code in PR #3643:
URL: https://github.com/apache/brpc/pull/3643#discussion_r4350869562
##########
test/brpc_prometheus_metrics_unittest.cpp:
##########
@@ -49,6 +53,53 @@ class DummyEchoServiceImpl : public test::EchoService {
}
};
+TEST(PrometheusMetrics, method_metrics_remain_exposed_during_startup) {
+ for (int i = 0; i < 20; ++i) {
+ DummyEchoServiceImpl echo_svc;
+ brpc::Server server;
+ ASSERT_EQ(0, server.AddService(&echo_svc,
brpc::SERVER_DOESNT_OWN_SERVICE));
+ brpc::ServerOptions options;
+ options.server_info_name = "prometheus_startup_alias";
+ const std::string count_name =
+ "rpc_server_prometheus_startup_alias_test_echo_service_echo_count";
+ std::atomic<bool> stop(false);
+ std::atomic<bool> observed(false);
+ std::atomic<bool> disappeared(false);
+ bthread::CountdownEvent started;
+ {
+ // Start observing before Start() can publish the first metric.
+ // echo.proto enables idl_support, so both full and short method
+ // names refer to the same MethodStatus.
+ std::thread observer([&] {
+ started.signal();
+ while (!stop.load(std::memory_order_relaxed)) {
+ if (!bvar::Variable::describe_exposed(count_name).empty())
{
+ observed.store(true, std::memory_order_relaxed);
+ } else if (observed.load(std::memory_order_relaxed)) {
+ disappeared.store(true, std::memory_order_relaxed);
+ }
+ }
+ });
+ BUTIL_SCOPE_EXIT {
+ stop.store(true, std::memory_order_relaxed);
+ observer.join();
+ };
+ started.wait();
+ ASSERT_EQ(0, server.Start("127.0.0.1:0", &options));
+ const int64_t deadline_us = butil::cpuwide_time_us() + 5000000;
+ while (!observed.load(std::memory_order_relaxed) &&
+ butil::cpuwide_time_us() < deadline_us) {
+ bthread_usleep(1000);
+ }
+ ASSERT_TRUE(observed.load(std::memory_order_relaxed));
+ ASSERT_EQ(0, server.Stop(0));
+ ASSERT_EQ(0, server.Join());
Review Comment:
The observer keeps running through server.Stop()/Join(). During shutdown,
it's normal for bvars/metrics to be unexposed, which can set `disappeared=true`
and fail the test even though no startup regression occurred. Stop the observer
immediately after the startup observation is complete (e.g., set `stop=true`
and join right after `ASSERT_TRUE(observed...)`), or gate the `disappeared`
check to only the startup window (before initiating Stop).
##########
test/brpc_prometheus_metrics_unittest.cpp:
##########
@@ -49,6 +53,53 @@ class DummyEchoServiceImpl : public test::EchoService {
}
};
+TEST(PrometheusMetrics, method_metrics_remain_exposed_during_startup) {
+ for (int i = 0; i < 20; ++i) {
+ DummyEchoServiceImpl echo_svc;
+ brpc::Server server;
+ ASSERT_EQ(0, server.AddService(&echo_svc,
brpc::SERVER_DOESNT_OWN_SERVICE));
+ brpc::ServerOptions options;
+ options.server_info_name = "prometheus_startup_alias";
+ const std::string count_name =
+ "rpc_server_prometheus_startup_alias_test_echo_service_echo_count";
+ std::atomic<bool> stop(false);
+ std::atomic<bool> observed(false);
+ std::atomic<bool> disappeared(false);
+ bthread::CountdownEvent started;
+ {
+ // Start observing before Start() can publish the first metric.
+ // echo.proto enables idl_support, so both full and short method
+ // names refer to the same MethodStatus.
+ std::thread observer([&] {
+ started.signal();
+ while (!stop.load(std::memory_order_relaxed)) {
+ if (!bvar::Variable::describe_exposed(count_name).empty())
{
+ observed.store(true, std::memory_order_relaxed);
+ } else if (observed.load(std::memory_order_relaxed)) {
+ disappeared.store(true, std::memory_order_relaxed);
+ }
Review Comment:
This loop is a tight busy-spin with no sleep/yield, which can burn CPU
during the test run (especially with 20 iterations). Add a small sleep/backoff
(e.g., bthread_usleep or std::this_thread::yield with a delay) to reduce load
while still catching brief disappearance windows.
##########
test/brpc_prometheus_metrics_unittest.cpp:
##########
@@ -49,6 +53,53 @@ class DummyEchoServiceImpl : public test::EchoService {
}
};
+TEST(PrometheusMetrics, method_metrics_remain_exposed_during_startup) {
+ for (int i = 0; i < 20; ++i) {
+ DummyEchoServiceImpl echo_svc;
+ brpc::Server server;
+ ASSERT_EQ(0, server.AddService(&echo_svc,
brpc::SERVER_DOESNT_OWN_SERVICE));
+ brpc::ServerOptions options;
+ options.server_info_name = "prometheus_startup_alias";
+ const std::string count_name =
+ "rpc_server_prometheus_startup_alias_test_echo_service_echo_count";
+ std::atomic<bool> stop(false);
+ std::atomic<bool> observed(false);
+ std::atomic<bool> disappeared(false);
+ bthread::CountdownEvent started;
+ {
+ // Start observing before Start() can publish the first metric.
+ // echo.proto enables idl_support, so both full and short method
+ // names refer to the same MethodStatus.
+ std::thread observer([&] {
+ started.signal();
Review Comment:
Initialize `bthread::CountdownEvent` with an explicit initial count (e.g.,
1) rather than relying on its default constructor. This makes the
synchronization intent unambiguous and avoids behavior changes if the default
initial count differs from what the test expects.
##########
test/brpc_prometheus_metrics_unittest.cpp:
##########
@@ -49,6 +53,53 @@ class DummyEchoServiceImpl : public test::EchoService {
}
};
+TEST(PrometheusMetrics, method_metrics_remain_exposed_during_startup) {
+ for (int i = 0; i < 20; ++i) {
+ DummyEchoServiceImpl echo_svc;
+ brpc::Server server;
+ ASSERT_EQ(0, server.AddService(&echo_svc,
brpc::SERVER_DOESNT_OWN_SERVICE));
+ brpc::ServerOptions options;
+ options.server_info_name = "prometheus_startup_alias";
+ const std::string count_name =
+ "rpc_server_prometheus_startup_alias_test_echo_service_echo_count";
+ std::atomic<bool> stop(false);
+ std::atomic<bool> observed(false);
+ std::atomic<bool> disappeared(false);
+ bthread::CountdownEvent started;
+ {
+ // Start observing before Start() can publish the first metric.
+ // echo.proto enables idl_support, so both full and short method
+ // names refer to the same MethodStatus.
+ std::thread observer([&] {
+ started.signal();
+ while (!stop.load(std::memory_order_relaxed)) {
+ if (!bvar::Variable::describe_exposed(count_name).empty())
{
+ observed.store(true, std::memory_order_relaxed);
+ } else if (observed.load(std::memory_order_relaxed)) {
+ disappeared.store(true, std::memory_order_relaxed);
+ }
+ }
+ });
+ BUTIL_SCOPE_EXIT {
+ stop.store(true, std::memory_order_relaxed);
+ observer.join();
+ };
+ started.wait();
Review Comment:
Initialize `bthread::CountdownEvent` with an explicit initial count (e.g.,
1) rather than relying on its default constructor. This makes the
synchronization intent unambiguous and avoids behavior changes if the default
initial count differs from what the test expects.
##########
test/brpc_prometheus_metrics_unittest.cpp:
##########
@@ -49,6 +53,53 @@ class DummyEchoServiceImpl : public test::EchoService {
}
};
+TEST(PrometheusMetrics, method_metrics_remain_exposed_during_startup) {
+ for (int i = 0; i < 20; ++i) {
+ DummyEchoServiceImpl echo_svc;
+ brpc::Server server;
+ ASSERT_EQ(0, server.AddService(&echo_svc,
brpc::SERVER_DOESNT_OWN_SERVICE));
+ brpc::ServerOptions options;
+ options.server_info_name = "prometheus_startup_alias";
+ const std::string count_name =
+ "rpc_server_prometheus_startup_alias_test_echo_service_echo_count";
+ std::atomic<bool> stop(false);
+ std::atomic<bool> observed(false);
+ std::atomic<bool> disappeared(false);
+ bthread::CountdownEvent started;
Review Comment:
Initialize `bthread::CountdownEvent` with an explicit initial count (e.g.,
1) rather than relying on its default constructor. This makes the
synchronization intent unambiguous and avoids behavior changes if the default
initial count differs from what the test expects.
--
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]
