grpc-benchmark: Add benchmark client and local runner#2737
Conversation
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements the benchmark client and worker client execution logic for the gRPC benchmark suite, including support for collecting latency statistics using histograms and running smoke tests. The feedback highlights several important improvements: preventing empty authority overrides from failing gRPC requests, handling empty histograms to avoid reporting incorrect minimum latencies, adding a small delay in the unary call retry loop to prevent 100% CPU usage on failures, and replacing an unsafe .unwrap() with proper error handling when accessing the benchmark client.
arjan-bal
changed the title
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a benchmark tool (bench) and implements the client-side benchmarking logic (BenchmarkClient) along with its integration into the worker service. It also updates the benchmark server to support dynamic port binding. The review feedback highlights two critical issues in client.rs: first, stale histogram reports from previous timed-out requests can corrupt benchmark statistics, which can be resolved by draining the receiver channel; second, if a streaming task fails, it exits prematurely, causing the stats aggregation to hang indefinitely. Re-establishing the stream on failure is recommended to prevent this hang.
e39fe6e to
8dfa7d9
Compare
| #[allow(unused)] | ||
| pub mod generated { | ||
| pub mod benchmark_service_grpc { | ||
| grpc::include_proto!("grpc/testing", "benchmark_service"); |
There was a problem hiding this comment.
@dfawley, since this macro includes both generates.rs and the _grpc.pb.rs, it doesn't handle multiple service files well. I wonder if we need our own generatetd_grpc.rs or require users to separately import protos and grpc.
There was a problem hiding this comment.
Yes, I agree something like would make sense. Not including the proto messages via our macro seems the most promising to me, but we'd probably have to do a little experimenting.
| } | ||
| let channel = builder.build(); | ||
|
|
||
| let histogram = Histogram::new_with_max(histogram_params.max_possible as u64, 3) |
There was a problem hiding this comment.
Move histogram initialization outside the for loop, since it doesn't change per-channel? Placed here it makes it seems there is a reason it is being create per-channel, since all the other invariants were created before the for loop.
There was a problem hiding this comment.
This ends up in the TestOptions/RpcOptions which is per-channel, though, so doesn't it have to be? Unless it's Copy.
There was a problem hiding this comment.
It is already being copied for each task in start_rpcs()
There was a problem hiding this comment.
Moved histogram initialization outside the loop and cloned it in the loop.
| } | ||
| let channel = builder.build(); | ||
|
|
||
| let histogram = Histogram::new_with_max(histogram_params.max_possible as u64, 3) |
There was a problem hiding this comment.
This ends up in the TestOptions/RpcOptions which is per-channel, though, so doesn't it have to be? Unless it's Copy.
3 participants
Contributes to: #2666
This PR introduces the benchmarking client. It also adds a local runner binary that exercises the worker, server, and client within a single process, making it easy to run benchmarks locally.
Similar to gRPC-Java, the client uses the
hdrhistogramcrate for its histogram implementation. Because the Rust crate does not support a thread-safe, lock-free implementation natively, the client leverages atokio::sync::watchchannel and atokio::sync::mpscchannel to coordinate between the worker task and the client RPC tasks when collecting stats. This design avoids the need to acquire a mutex lock when reporting the latency of each RPC.