fix(experimentation): retry experiment computes on transient ClickHouse errors - #8273
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
2 Skipped Deployments
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThe change adds a 120-second timeout for background ClickHouse queries and caches clients by timeout variant. Exposure and metric-statistics queries use this timeout. Experiment computation tasks use a three-minute timeout and raise Estimated code review effort: 3 (Moderate) | ~20 minutes Mergeability Score: ⚪ Minimal · up to The change adjusts retry handling and background query timeouts; the supplied evidence identifies no actionable merge-blocking correctness, availability, or deployment risk beyond normal checks. 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 |
github-actions
Bot
added
api
Docker builds report
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 512e0bf4-0731-4580-b052-c50b33090bc0
📒 Files selected for processing (6)
api/experimentation/services.pyapi/experimentation/tasks.pyapi/tests/unit/experimentation/test_services.pyapi/tests/unit/experimentation/test_tasks.pyapi/tests/unit/experimentation/test_views.pydocs/docs/deployment-self-hosting/observability/_events-catalogue.md
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #8273 +/- ##
=======================================
Coverage 98.74% 98.75%
=======================================
Files 1569 1569
Lines 62778 62827 +49
=======================================
+ Hits 61993 62042 +49
Misses 785 785 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
✅ private-cloud · depot-ubuntu-latest-arm-16 — run #19282 (attempt 1)Playwright Test Results (private-cloud - depot-ubuntu-latest-arm-16)
Details
🗂️ Previous results✅ private-cloud · depot-ubuntu-latest-16 — run #19282 (attempt 1)Playwright Test Results (private-cloud - depot-ubuntu-latest-16)
Details
✅ oss · depot-ubuntu-latest-16 — run #19282 (attempt 1)Playwright Test Results (oss - depot-ubuntu-latest-16)
Details
✅ oss · depot-ubuntu-latest-arm-16 — run #19282 (attempt 1)Playwright Test Results (oss - depot-ubuntu-latest-arm-16)
Details
✅ private-cloud · depot-ubuntu-latest-arm-16 — run #19259 (attempt 1)Playwright Test Results (private-cloud - depot-ubuntu-latest-arm-16)
Details
✅ private-cloud · depot-ubuntu-latest-16 — run #19259 (attempt 1)Playwright Test Results (private-cloud - depot-ubuntu-latest-16)
Details
Skipped testsfirefox › tests/onboarding-tests.pw.ts › Onboarding › New user connects via the single-page onboarding flow @oss ✅ oss · depot-ubuntu-latest-arm-16 — run #19259 (attempt 1)Playwright Test Results (oss - depot-ubuntu-latest-arm-16)
Details
✅ oss · depot-ubuntu-latest-16 — run #19259 (attempt 1)Playwright Test Results (oss - depot-ubuntu-latest-16)
Details
✅ private-cloud · depot-ubuntu-latest-16 — run #19251 (attempt 1)Playwright Test Results (private-cloud - depot-ubuntu-latest-16)
Details
✅ oss · depot-ubuntu-latest-16 — run #19251 (attempt 1)Playwright Test Results (oss - depot-ubuntu-latest-16)
Details
✅ oss · depot-ubuntu-latest-arm-16 — run #19251 (attempt 1)Playwright Test Results (oss - depot-ubuntu-latest-arm-16)
Details
✅ private-cloud · depot-ubuntu-latest-arm-16 — run #19251 (attempt 1)Playwright Test Results (private-cloud - depot-ubuntu-latest-arm-16)
Details
Skipped testsfirefox › tests/onboarding-tests.pw.ts › Onboarding › New user connects via the single-page onboarding flow @oss |
✅ oss · depot-ubuntu-latest-16 — run #19249 (attempt 1)Playwright Test Results (oss - depot-ubuntu-latest-16)
Details
|
Visual Regression19 screenshots compared. See report for details. |
github-actions
Bot
added
docs
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
api/tests/unit/experimentation/test_services.py (1)
112-141: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winVerify reuse of the background-timeout client.
The test calls
_get_clickhouse_clientwith the background timeout only once. It proves default-client reuse and timeout-variant separation, but it does not prove reuse for repeated background calls. Add a second background call and assert that it returns the same client whilemock_client_cls.call_countremains2.Proposed test adjustment
background_client = services._get_clickhouse_client( send_receive_timeout=services.CLICKHOUSE_BACKGROUND_QUERY_TIMEOUT_SECONDS, ) + same_background_client = services._get_clickhouse_client( + send_receive_timeout=services.CLICKHOUSE_BACKGROUND_QUERY_TIMEOUT_SECONDS, + ) # Then assert client is same_client assert background_client is not client + assert same_background_client is background_client assert mock_client_cls.call_count == 2
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 5ec8ac89-4610-43eb-8285-4d1461ace406
📒 Files selected for processing (3)
api/experimentation/services.pyapi/tests/unit/experimentation/test_services.pydocs/docs/deployment-self-hosting/observability/_events-catalogue.md
…lickhouse-timeouts # Conflicts: # docs/docs/deployment-self-hosting/observability/_events-catalogue.md
2 participants
Thanks for submitting a PR! Please check the boxes below:
docs/if required so people know about the feature.Changes
Contributes to #8274
Experiment computes fail with
TimeoutErrorwhen a ClickHouse read exceeds the 30ssend_receive_timeout, and the failure was swallowed, so refreshes silently produced nothing.TaskBackoffErroronOSErrorincompute_experiment_exposures/compute_experiment_resultsso the task processor retries.How did you test this code?
Unit tests covering the retry behaviour, per-timeout client caching, and background timeout usage; full experimentation suite, lint, and mypy pass.