feat(experimentation): route external warehouse events to a static Kafka topic - #8530
Conversation
…fka topic External warehouse connections no longer provision a per-organisation S3 bucket and Firehose stream. The ingestion server produces to Kafka, so the environment's Redis destination is set to the shared external_wh_events topic instead. The S3-based delivery to customer ClickHouse instances and the per-organisation infrastructure models are removed along with it.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
2 Skipped Deployments
|
github-actions
Bot
added
api
|
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: Advanced Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 8 included reviews per hour; 3 remain after this review. 📝 WalkthroughWalkthroughThe change removes per-organisation AWS ingestion infrastructure and S3-based warehouse event delivery. External warehouse events now use the Priority: ➖ Normal Estimated code review effort: 4 (Complex) | ~60 minutes Merge Risk: ⚪ Minimal · up to New and deleted warehouse connections trigger ingestion synchronization, so routing and cleanup remain reconciled. 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 |
Docker builds report
|
github-actions
Bot
added
docs
✅ private-cloud · depot-ubuntu-latest-16 — run #20476 (attempt 1)Playwright Test Results (private-cloud - depot-ubuntu-latest-16)
Details
🗂️ Previous results✅ private-cloud · depot-ubuntu-latest-arm-16 — run #20476 (attempt 1)Playwright Test Results (private-cloud - depot-ubuntu-latest-arm-16)
Details
✅ oss · depot-ubuntu-latest-arm-16 — run #20476 (attempt 1)Playwright Test Results (oss - depot-ubuntu-latest-arm-16)
Details
✅ oss · depot-ubuntu-latest-16 — run #20476 (attempt 1)Playwright Test Results (oss - depot-ubuntu-latest-16)
Details
✅ private-cloud · depot-ubuntu-latest-arm-16 — run #20452 (attempt 1)Playwright Test Results (private-cloud - depot-ubuntu-latest-arm-16)
Details
✅ private-cloud · depot-ubuntu-latest-arm-16 — run #20451 (attempt 1)Playwright Test Results (private-cloud - depot-ubuntu-latest-arm-16)
Details
✅ private-cloud · depot-ubuntu-latest-16 — run #20452 (attempt 1)Playwright Test Results (private-cloud - depot-ubuntu-latest-16)
Details
✅ oss · depot-ubuntu-latest-arm-16 — run #20451 (attempt 1)Playwright Test Results (oss - depot-ubuntu-latest-arm-16)
Details
✅ oss · depot-ubuntu-latest-arm-16 — run #20452 (attempt 1)Playwright Test Results (oss - depot-ubuntu-latest-arm-16)
Details
✅ oss · depot-ubuntu-latest-16 — run #20452 (attempt 1)Playwright Test Results (oss - depot-ubuntu-latest-16)
Details
✅ private-cloud · depot-ubuntu-latest-16 — run #20451 (attempt 1)Playwright Test Results (private-cloud - depot-ubuntu-latest-16)
Details
✅ oss · depot-ubuntu-latest-16 — run #20451 (attempt 1)Playwright Test Results (oss - depot-ubuntu-latest-16)
Details
|
Visual Regression19 screenshots compared. See report for details. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #8530 +/- ##
==========================================
- Coverage 98.82% 98.81% -0.02%
==========================================
Files 1622 1619 -3
Lines 66727 66053 -674
==========================================
- Hits 65942 65268 -674
Misses 785 785 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Advanced
Run ID: 7c364438-b725-4eab-981c-d6f423688160
📒 Files selected for processing (27)
api/app/settings/common.pyapi/experimentation/constants.pyapi/experimentation/dataclasses.pyapi/experimentation/ingestion_infra_service.pyapi/experimentation/ingestion_sync_service.pyapi/experimentation/metrics.pyapi/experimentation/migrations/0014_drop_ingestion_infrastructure_and_delivery_log.pyapi/experimentation/models.pyapi/experimentation/organisation_ingestion_service.pyapi/experimentation/services.pyapi/experimentation/tasks.pyapi/experimentation/warehouse_delivery_service.pyapi/organisations/models.pyapi/tests/unit/experimentation/conftest.pyapi/tests/unit/experimentation/test_ingestion_infra_service.pyapi/tests/unit/experimentation/test_ingestion_sync_service.pyapi/tests/unit/experimentation/test_models.pyapi/tests/unit/experimentation/test_organisation_ingestion_service.pyapi/tests/unit/experimentation/test_tasks.pyapi/tests/unit/experimentation/test_warehouse_delivery_service.pyapi/tests/unit/organisations/test_unit_organisations_models.pydocs/docs/deployment-self-hosting/observability/_events-catalogue.mddocs/docs/deployment-self-hosting/observability/_metrics-catalogue.mdinfrastructure/aws/production/ecs-task-definition-admin-api.jsoninfrastructure/aws/production/ecs-task-definition-task-processor.jsoninfrastructure/aws/staging/ecs-task-definition-admin-api.jsoninfrastructure/aws/staging/ecs-task-definition-task-processor.json
💤 Files with no reviewable changes (16)
- api/experimentation/dataclasses.py
- api/organisations/models.py
- api/tests/unit/organisations/test_unit_organisations_models.py
- infrastructure/aws/staging/ecs-task-definition-admin-api.json
- api/experimentation/ingestion_infra_service.py
- api/app/settings/common.py
- api/tests/unit/experimentation/conftest.py
- docs/docs/deployment-self-hosting/observability/_metrics-catalogue.md
- api/tests/unit/experimentation/test_organisation_ingestion_service.py
- api/experimentation/services.py
- infrastructure/aws/production/ecs-task-definition-admin-api.json
- api/experimentation/organisation_ingestion_service.py
- infrastructure/aws/production/ecs-task-definition-task-processor.json
- api/tests/unit/experimentation/test_ingestion_infra_service.py
- infrastructure/aws/staging/ecs-task-definition-task-processor.json
- api/experimentation/metrics.py
Included review availability: Your plan provides up to 8 included reviews per hour; 7 remain after this review.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to GitHub limitations.
🟠 Major · Backfill existing external warehouse destinations before removing the old infrastructure. · api/experimentation/models.py:91-95
91-95: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winBackfill existing external warehouse destinations before removing the old infrastructure.
Before this change, the
AFTER_CREATEhook provisioned per-organisation infrastructure and stored its stream name in Redis. ExistingWarehouseConnectionrows do not run that hook again, so they retain that destination.Migration
0014_drop_ingestion_infrastructure_and_delivery_log.pyremoves the old infrastructure models but does not update Redis. The deployment paths only run migrations. If the old stream is unavailable, those connections can stop delivering external events.Set
external_warehouse_eventsfor every existing external connection before removing the old infrastructure. The new creation hook already sets this destination for new connections, so no separate caller fix is required.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Advanced
Run ID: ae0eb8d9-213e-4ed6-92f2-015ed2d54a0d
📒 Files selected for processing (4)
api/experimentation/constants.pyapi/tests/unit/experimentation/test_ingestion_sync_service.pyapi/tests/unit/experimentation/test_models.pyapi/tests/unit/experimentation/test_tasks.py
Included review availability: Your plan provides up to 8 included reviews per hour; 6 remain after this review.
github-actions
Bot
added
feature
…tion Creating and deleting a connection enqueued independent write and remove tasks, and the task processor does not order tasks per environment, so recreating a connection could leave the remove task running last and wipe the new keys. One task now reads the active connection and converges Redis on it, so a delete-then-create pair ends up correct whichever task runs last.
ac7139c to
f1e55ed
Compare
github-actions
Bot
added
docs
Zaimwa9
left a comment
Zaimwa9
left a comment
There was a problem hiding this comment.
Overall looking good but 2 minors:
- resync on warehouse switch
- managing environment soft deletes
…environment deletion Changing a connection's warehouse type left the old Redis destination in place, and deleting an environment left its keys behind because the sync task could no longer find the soft-deleted environment.
github-actions
Bot
added
docs
2 participants
Thanks for submitting a PR! Please check the boxes below:
docs/if required so people know about the feature.Changes
The ingestion server now produces events to Kafka, so external warehouse connections no longer need a per-organisation S3 bucket and Firehose stream.
sync_environment_ingestiontask, which reads the active connection and converges Redis on it: external connections get the staticexternal_warehouse_eventstopic as destination before their keys are whitelisted, Flagsmith ones follow the default pipeline, and an environment with no connection has its keys and destination removedOrganisationIngestionInfrastructureandWarehouseDeliveryLogmodels, and the S3 to ClickHouse delivery tasks.INGESTION_FIREHOSE_DELIVERY_ROLE_ARNsetting and its ECS task definition entries.Deploy notes:
sync_environment_ingestion(environment_id)for each affected environment.How did you test this code?
Unit tests under
api/tests/unit/experimentationandapi/tests/unit/organisations, plusmypy,ruffand the docgen pre-commit hook.