Conversation
![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 introduces a new benchmark tool, pubsub-hedging, to evaluate Cloud Pub/Sub publish latency and request hedging against a simulated tail-latency mock server. The review feedback identifies three key issues: an ineffective timeout mechanism when draining the benchmark's JoinSet that can lead to indefinite blocking, a potential division-by-zero panic if the message_rate argument is set to zero, and a lack of a timeout during the warmup phase's task draining loop.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #6804 +/- ##
=======================================
Coverage 96.97% 96.97%
=======================================
Files 325 325
Lines 106426 106426
=======================================
+ Hits 103204 103209 +5
+ Misses 3222 3217 -5 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a new benchmark crate, pubsub-hedging, designed to evaluate Cloud Pub/Sub message publishing latency with and without request hedging against an in-process simulated tail latency server. The changes include adding the benchmark orchestrator, CLI argument parsing, a mock server with configurable latencies, and statistics calculation utilities, along with workspace configuration updates. The review feedback suggests explicitly aborting warmup tasks if they timeout to prevent them from running in the background and polluting the benchmark results.
5f20420 to
9c4d8fb
Compare
Add a standalone benchmark binary in `src/pubsub/benchmarks/hedging` to measure publisher throughput and latency percentiles under simulated network tail latencies (fast, degraded, and stall modes). The benchmark includes an in-process mock server backed by `pubsub-grpc-mock` which tracks total and hedged publish RPC counts via telemetry headers, running unbatched messages (1 message per batch) with a configurable warmup phase.
9c4d8fb to
cb1213f
Compare
dbolduc
left a comment
dbolduc
left a comment
There was a problem hiding this comment.
I wonder how well this models prod
| let mut interval = tokio::time::interval(interval_duration); | ||
| interval.set_missed_tick_behavior(tokio::time::MissedTickBehavior::Burst); | ||
|
|
||
| if !args.warmup.is_zero() { |
There was a problem hiding this comment.
optional nit: factor out warmup into like warmup(...).await? or something just so main is more streamlined
| self.config.degraded_latency | ||
| } else { | ||
| self.config.stall_latency | ||
| }; |
There was a problem hiding this comment.
comment: Oh so we are simulating server behavior, not hitting prod. hmm. I feel like we already know the answers to the test.
There is value in simulating things that do happen but are hard to reproduce.
| use std::time::Duration; | ||
| use tokio::task::JoinHandle; | ||
|
|
||
| use pubsub_grpc_mock::google::pubsub::v1::publisher_server::{Publisher, PublisherServer}; |
There was a problem hiding this comment.
comment: Ah we can use the generated gRPC server directly. IIRC our mocking framework only supports one service bc I am lazy.
2 participants
Add a standalone benchmark binary in
src/pubsub/benchmarks/hedgingto measure publisher throughput and tail latency under simulated network tail latencies (fast, degraded, and stall modes).The benchmark includes an in-process mock server backed by
pubsub-grpc-mockwhich tracks total and hedged publish RPC counts via telemetry headers.Hedging configuration is not yet added because we do not yet support it in the publisher.
For #6776