ci: Add simulated DB delay to performance benchmarks #9924
Conversation
|
🚀 Thanks for opening this pull request! |
📝 WalkthroughWalkthroughThe performance CI workflow is updated to integrate Toxiproxy for simulating 10ms database latency during benchmarks. Setup and restart steps are added for baseline and PR runs respectively, MONGODB_URI environment variable is introduced pointing to the proxied MongoDB instance, and benchmark messages are updated to document the latency simulation. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
.github/workflows/ci-performance.yml (1)
343-343: Document latency simulation as a breaking change for benchmark interpretation.The PR comment now documents 10ms latency. However, this represents a significant change in how benchmarks are measured. Existing baseline numbers from before this change are not directly comparable.
Consider adding a note in the PR description or documentation about:
- Why 10ms latency was chosen
- How this affects interpretation of performance metrics
- Whether historical baseline numbers need to be re-established
Would you like me to suggest documentation updates for the benchmark README or CONTRIBUTING guide to explain the latency simulation and its implications?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/ci-performance.yml(3 hunks)
🔇 Additional comments (1)
.github/workflows/ci-performance.yml (1)
90-90: Verify MongoDB connection pool handles proxy latency correctly.The benchmark now runs against a proxied MongoDB with 10ms added latency. Ensure that the benchmark connection pool configuration (idle timeout, max connections, etc.) doesn't cause resource exhaustion or false timeouts due to the added latency.
Please verify:
- That the benchmark script's MongoDB connection pool is configured appropriately for network latency
- That no connection timeouts occur due to the added 10ms latency
- That the results are reproducible across runs
Also applies to: 170-170
| - name: Setup Toxiproxy for latency simulation (baseline) | ||
| run: | | ||
| # Download and install toxiproxy | ||
| wget -O toxiproxy.deb https://github.com/Shopify/toxiproxy/releases/download/v2.9.0/toxiproxy_2.9.0_amd64.deb | ||
| sudo dpkg -i toxiproxy.deb | ||
| # Start toxiproxy server in background | ||
| toxiproxy-server & | ||
| sleep 2 | ||
| # Create a proxy for MongoDB (27017 -> 27018) | ||
| toxiproxy-cli create mongodb -l localhost:27018 -u localhost:27017 | ||
| # Add 10ms latency to simulate network conditions | ||
| toxiproxy-cli toxic add mongodb -t latency -a latency=10 | ||
| echo "Toxiproxy setup complete - MongoDB accessible on port 27018 with 10ms latency" | ||
There was a problem hiding this comment.
Add error handling and health checks to Toxiproxy setup.
The Toxiproxy setup lacks error handling and health checks. If the download fails, the dpkg command fails, or the server doesn't start properly, the workflow continues silently and benchmarks run against an unavailable proxy, producing invalid results.
Additionally, the 2-second sleep after starting the server is arbitrary and may not be sufficient for the proxy to be ready in all CI conditions.
Apply this diff to add error handling and health checks:
- name: Setup Toxiproxy for latency simulation (baseline)
run: |
+ set -e
+ trap 'echo "Toxiproxy setup failed"; exit 1' ERR
+
# Download and install toxiproxy
wget -O toxiproxy.deb https://github.com/Shopify/toxiproxy/releases/download/v2.9.0/toxiproxy_2.9.0_amd64.deb
sudo dpkg -i toxiproxy.deb
# Start toxiproxy server in background
toxiproxy-server &
- sleep 2
+ sleep 3
+
+ # Wait for toxiproxy to be ready (health check)
+ for i in {1..10}; do
+ if nc -z localhost 8474 2>/dev/null; then
+ echo "Toxiproxy server is ready"
+ break
+ fi
+ echo "Waiting for Toxiproxy server... ($i/10)"
+ sleep 1
+ done
# Create a proxy for MongoDB (27017 -> 27018)
toxiproxy-cli create mongodb -l localhost:27018 -u localhost:27017
+ if [ $? -ne 0 ]; then
+ echo "Failed to create MongoDB proxy"
+ exit 1
+ fi
# Add 10ms latency to simulate network conditions
toxiproxy-cli toxic add mongodb -t latency -a latency=10
+ if [ $? -ne 0 ]; then
+ echo "Failed to add latency toxic"
+ exit 1
+ fi
+ # Verify the proxy is working
+ if nc -z localhost 27018 2>/dev/null; then
+ echo "Toxiproxy setup complete - MongoDB accessible on port 27018 with 10ms latency"
+ else
+ echo "Toxiproxy proxy is not accessible on port 27018"
+ exit 1
+ fi
- echo "Toxiproxy setup complete - MongoDB accessible on port 27018 with 10ms latency"Note: This assumes netcat (nc) is available; you may need to install it or use an alternative connectivity test.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - name: Setup Toxiproxy for latency simulation (baseline) | |
| run: | | |
| # Download and install toxiproxy | |
| wget -O toxiproxy.deb https://github.com/Shopify/toxiproxy/releases/download/v2.9.0/toxiproxy_2.9.0_amd64.deb | |
| sudo dpkg -i toxiproxy.deb | |
| # Start toxiproxy server in background | |
| toxiproxy-server & | |
| sleep 2 | |
| # Create a proxy for MongoDB (27017 -> 27018) | |
| toxiproxy-cli create mongodb -l localhost:27018 -u localhost:27017 | |
| # Add 10ms latency to simulate network conditions | |
| toxiproxy-cli toxic add mongodb -t latency -a latency=10 | |
| echo "Toxiproxy setup complete - MongoDB accessible on port 27018 with 10ms latency" | |
| - name: Setup Toxiproxy for latency simulation (baseline) | |
| run: | | |
| set -e | |
| trap 'echo "Toxiproxy setup failed"; exit 1' ERR | |
| # Download and install toxiproxy | |
| wget -O toxiproxy.deb https://github.com/Shopify/toxiproxy/releases/download/v2.9.0/toxiproxy_2.9.0_amd64.deb | |
| sudo dpkg -i toxiproxy.deb | |
| # Start toxiproxy server in background | |
| toxiproxy-server & | |
| sleep 3 | |
| # Wait for toxiproxy to be ready (health check) | |
| for i in {1..10}; do | |
| if nc -z localhost 8474 2>/dev/null; then | |
| echo "Toxiproxy server is ready" | |
| break | |
| fi | |
| echo "Waiting for Toxiproxy server... ($i/10)" | |
| sleep 1 | |
| done | |
| # Create a proxy for MongoDB (27017 -> 27018) | |
| toxiproxy-cli create mongodb -l localhost:27018 -u localhost:27017 | |
| if [ $? -ne 0 ]; then | |
| echo "Failed to create MongoDB proxy" | |
| exit 1 | |
| fi | |
| # Add 10ms latency to simulate network conditions | |
| toxiproxy-cli toxic add mongodb -t latency -a latency=10 | |
| if [ $? -ne 0 ]; then | |
| echo "Failed to add latency toxic" | |
| exit 1 | |
| fi | |
| # Verify the proxy is working | |
| if nc -z localhost 27018 2>/dev/null; then | |
| echo "Toxiproxy setup complete - MongoDB accessible on port 27018 with 10ms latency" | |
| else | |
| echo "Toxiproxy proxy is not accessible on port 27018" | |
| exit 1 | |
| fi |
| wget -O toxiproxy.deb https://github.com/Shopify/toxiproxy/releases/download/v2.9.0/toxiproxy_2.9.0_amd64.deb | ||
| sudo dpkg -i toxiproxy.deb |
There was a problem hiding this comment.
Add checksum validation for downloaded Toxiproxy binary.
Downloading executables without verifying integrity is a supply-chain risk. If the GitHub release is compromised or a MITM attack occurs, malicious code could run in the CI environment.
Consider adding checksum validation:
wget -O toxiproxy.deb https://github.com/Shopify/toxiproxy/releases/download/v${TOXIPROXY_VERSION}/toxiproxy_${TOXIPROXY_VERSION}_amd64.deb
+ wget -O toxiproxy.deb.sha256 https://github.com/Shopify/toxiproxy/releases/download/v${TOXIPROXY_VERSION}/toxiproxy_${TOXIPROXY_VERSION}_amd64.deb.sha256
+ sha256sum -c toxiproxy.deb.sha256 || (echo "Checksum validation failed"; exit 1)
sudo dpkg -i toxiproxy.debAlternatively, use a package manager or container image that handles verification automatically.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
.github/workflows/ci-performance.yml around lines 71-72: the workflow downloads
and installs the Toxiproxy .deb without verifying integrity; update the job to
validate the download by obtaining the expected checksum (preferably a vetted
SHA256 from a trusted source or a checksum file committed to the repo), compute
the SHA256 of the downloaded toxiproxy.deb in CI and compare it against the
expected value, and fail the job if they differ; alternatively replace the
direct download with a verified source such as the OS package manager or a
signed container image that provides built-in verification.
| - name: Setup Toxiproxy for latency simulation (baseline) | ||
| run: | | ||
| # Download and install toxiproxy | ||
| wget -O toxiproxy.deb https://github.com/Shopify/toxiproxy/releases/download/v2.9.0/toxiproxy_2.9.0_amd64.deb |
There was a problem hiding this comment.
Hard-coded Toxiproxy version requires a maintenance plan.
The Toxiproxy version is hard-coded to v2.9.0 with no documented strategy for updates or deprecation handling. As CI infrastructure evolves, this could become a source of maintenance debt or security issues.
Consider using a version variable pinned at the top of the workflow for easier updates, or document the maintenance cadence.
Apply this diff to add versioning at the workflow level:
env:
NODE_VERSION: 24.11.0
MONGODB_VERSION: 8.0.4
+ TOXIPROXY_VERSION: 2.9.0Then update the setup step to reference it:
- wget -O toxiproxy.deb https://github.com/Shopify/toxiproxy/releases/download/v2.9.0/toxiproxy_2.9.0_amd64.deb
- sudo dpkg -i toxiproxy.deb
+ wget -O toxiproxy.deb https://github.com/Shopify/toxiproxy/releases/download/v${TOXIPROXY_VERSION}/toxiproxy_${TOXIPROXY_VERSION}_amd64.deb
+ sudo dpkg -i toxiproxy.debCommittable suggestion skipped: line range outside the PR's diff.
| - name: Restart Toxiproxy for PR benchmarks | ||
| run: | | ||
| # Restart toxiproxy server (toxiproxy-cli is already installed from baseline) | ||
| pkill toxiproxy-server || true | ||
| sleep 1 | ||
| toxiproxy-server & | ||
| sleep 2 | ||
| # Recreate proxy with same configuration | ||
| toxiproxy-cli create mongodb -l localhost:27018 -u localhost:27017 | ||
| toxiproxy-cli toxic add mongodb -t latency -a latency=10 | ||
| echo "Toxiproxy restarted with 10ms latency" | ||
There was a problem hiding this comment.
Add health checks and error handling to Toxiproxy restart step.
The restart step uses pkill ... || true which silently suppresses errors, potentially leaving stale processes. There's no verification that the old process actually stopped or that the new one starts correctly. This can cause state corruption or port conflicts.
Apply this diff to add error handling and validation:
- name: Restart Toxiproxy for PR benchmarks
run: |
+ set -e
+ trap 'echo "Toxiproxy restart failed"; exit 1' ERR
+
# Restart toxiproxy server (toxiproxy-cli is already installed from baseline)
- pkill toxiproxy-server || true
- sleep 1
- toxiproxy-server &
- sleep 2
+ if pgrep -f toxiproxy-server > /dev/null; then
+ echo "Stopping existing Toxiproxy instance..."
+ pkill -f toxiproxy-server
+ sleep 2
+ # Verify the old process is gone
+ if pgrep -f toxiproxy-server > /dev/null; then
+ echo "Warning: Toxiproxy process still running, using pkill -9"
+ pkill -9 -f toxiproxy-server
+ sleep 1
+ fi
+ fi
+
+ echo "Starting Toxiproxy server..."
+ toxiproxy-server &
+ TOXIPROXY_PID=$!
+ sleep 3
+
+ # Wait for toxiproxy to be ready
+ for i in {1..10}; do
+ if nc -z localhost 8474 2>/dev/null; then
+ echo "Toxiproxy server is ready (PID: $TOXIPROXY_PID)"
+ break
+ fi
+ echo "Waiting for Toxiproxy server... ($i/10)"
+ sleep 1
+ done
# Recreate proxy with same configuration
toxiproxy-cli create mongodb -l localhost:27018 -u localhost:27017
toxiproxy-cli toxic add mongodb -t latency -a latency=10
+ if [ $? -ne 0 ]; then
+ echo "Failed to recreate MongoDB proxy"
+ exit 1
+ fi
- echo "Toxiproxy restarted with 10ms latency"
+ # Verify the proxy is working
+ if nc -z localhost 27018 2>/dev/null; then
+ echo "Toxiproxy restarted with 10ms latency - MongoDB accessible on port 27018"
+ else
+ echo "Toxiproxy proxy is not accessible on port 27018 after restart"
+ exit 1
+ fi🤖 Prompt for AI Agents
.github/workflows/ci-performance.yml around lines 152 to 165: the Toxiproxy
restart step lacks verification and silences failures; update the step to
explicitly stop any existing toxiproxy-server and fail if it cannot be
terminated within a short timeout, verify the proxy port is free before
starting, start toxiproxy-server and poll until it responds on its control port
(failing the job on timeout), recreate the proxy and toxic but check the return
codes or HTTP responses for success and retry a couple times before failing, and
emit clear error messages on each failure so the CI job fails fast on
unrecoverable errors.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## alpha #9924 +/- ##
==========================================
- Coverage 93.07% 93.05% -0.02%
==========================================
Files 187 187
Lines 15224 15224
Branches 177 177
==========================================
- Hits 14170 14167 -3
- Misses 1042 1045 +3
Partials 12 12 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Not necessary; variability is low enough at current test setup |
2 participants
Pull Request
Issue
Using mongodb-runner to simulate the DB, there is currently an insignificant network delay.
Approach
For more realistic results, add simulated DB delay to performance benchmarks.
Summary by CodeRabbit