feat(data-warehouse): throttling consumer for person-property updates - #70039
Conversation
|
Reviews (1): Last reviewed commit: "feat(data-warehouse): throttling consume..." | Re-trigger Greptile |
PR overviewAll previously flagged issues have been addressed. No open security concerns remain on this pull request. Security reviewNo open security issues remain on this pull request. Fixed/addressed: 4 · PR risk: 0/10 |
🤖 CI report
|
| File | Patch | Uncovered changed lines |
|---|---|---|
products/warehouse_sources/backend/management/commands/run_person_property_update_consumer.py |
0.0% | 1, 3, 6, 12–13, 15–16, 22, 29, 33–35 |
products/warehouse_sources/backend/temporal/data_imports/person_property_update_consumer.py |
97.0% | 103, 161, 163, 168–169 |
🤖 Agents: add a test covering the lines above, or note why under "How did you test this code?". Machine-readable gap list: the patch-coverage artifact on this run (gh run download 29419471806 -n patch-coverage), or the coverage-data block at the end of this comment.
Per-product line coverage (touched products)
| Product | Coverage | Lines |
|---|---|---|
platform_features |
██░░░░░░░░░░░░░░░░░░ 12.1% |
7 / 58 |
batch_exports |
████████░░░░░░░░░░░░ 39.7% |
8,416 / 21,220 |
demo |
███████████░░░░░░░░░ 56.2% |
1,497 / 2,663 |
warehouse_sources_queue |
████████████░░░░░░░░ 59.2% |
148 / 250 |
tasks |
█████████████░░░░░░░ 67.4% |
25,449 / 37,779 |
data_tools |
██████████████░░░░░░ 70.0% |
63 / 90 |
ai_gateway |
███████████████░░░░░ 75.0% |
9 / 12 |
signals |
████████████████░░░░ 79.1% |
19,070 / 24,106 |
data_modeling |
████████████████░░░░ 80.0% |
4,834 / 6,045 |
cdp |
████████████████░░░░ 80.7% |
3,118 / 3,864 |
wizard |
████████████████░░░░ 82.5% |
772 / 936 |
agent_platform |
█████████████████░░░ 84.2% |
3,112 / 3,695 |
notebooks |
█████████████████░░░ 84.3% |
6,323 / 7,501 |
cohorts |
█████████████████░░░ 86.0% |
3,989 / 4,639 |
actions |
█████████████████░░░ 86.6% |
717 / 828 |
product_tours |
█████████████████░░░ 87.5% |
1,266 / 1,447 |
exports |
██████████████████░░ 88.3% |
6,891 / 7,800 |
visual_review |
██████████████████░░ 88.5% |
5,565 / 6,287 |
business_knowledge |
██████████████████░░ 88.5% |
4,400 / 4,969 |
conversations |
██████████████████░░ 88.9% |
16,126 / 18,130 |
dashboards |
██████████████████░░ 89.1% |
5,653 / 6,345 |
mcp_analytics |
██████████████████░░ 89.1% |
2,502 / 2,807 |
error_tracking |
██████████████████░░ 89.5% |
9,683 / 10,816 |
engineering_analytics |
██████████████████░░ 90.0% |
5,065 / 5,625 |
streamlit_apps |
██████████████████░░ 90.4% |
2,499 / 2,764 |
slack_app |
██████████████████░░ 90.6% |
9,511 / 10,503 |
links |
██████████████████░░ 90.6% |
183 / 202 |
marketing_analytics |
██████████████████░░ 90.8% |
11,514 / 12,684 |
alerts |
██████████████████░░ 90.9% |
3,416 / 3,760 |
product_analytics |
██████████████████░░ 91.1% |
5,527 / 6,068 |
managed_migrations |
██████████████████░░ 91.9% |
908 / 988 |
workflows |
██████████████████░░ 92.0% |
4,795 / 5,210 |
data_warehouse |
██████████████████░░ 92.1% |
17,885 / 19,428 |
mcp_store |
██████████████████░░ 92.1% |
3,665 / 3,981 |
notifications |
███████████████████░ 92.7% |
1,026 / 1,107 |
web_analytics |
███████████████████░ 92.7% |
13,607 / 14,674 |
ai_observability |
███████████████████░ 92.8% |
14,771 / 15,923 |
surveys |
███████████████████░ 92.9% |
5,660 / 6,094 |
posthog_ai |
███████████████████░ 93.2% |
1,312 / 1,408 |
tracing |
███████████████████░ 93.3% |
2,435 / 2,611 |
approvals |
███████████████████░ 93.3% |
3,395 / 3,640 |
reminders |
███████████████████░ 93.4% |
468 / 501 |
early_access_features |
███████████████████░ 93.8% |
848 / 904 |
legal_documents |
███████████████████░ 94.1% |
1,568 / 1,667 |
endpoints |
███████████████████░ 94.1% |
8,606 / 9,143 |
skills |
███████████████████░ 94.4% |
2,827 / 2,995 |
revenue_analytics |
███████████████████░ 94.5% |
3,598 / 3,809 |
messaging |
███████████████████░ 94.5% |
2,530 / 2,677 |
review_hog |
███████████████████░ 94.6% |
6,475 / 6,848 |
logs |
███████████████████░ 95.3% |
9,525 / 9,991 |
growth |
███████████████████░ 95.5% |
2,734 / 2,864 |
experiments |
███████████████████░ 95.6% |
24,138 / 25,244 |
replay_vision |
███████████████████░ 95.7% |
13,300 / 13,896 |
feature_flags |
███████████████████░ 96.1% |
14,750 / 15,354 |
warehouse_sources |
███████████████████░ 96.1% |
220,810 / 229,682 |
annotations |
███████████████████░ 96.2% |
732 / 761 |
user_interviews |
███████████████████░ 96.4% |
2,242 / 2,325 |
access_control |
███████████████████░ 96.8% |
849 / 877 |
data_catalog |
███████████████████░ 97.1% |
2,035 / 2,095 |
customer_analytics |
███████████████████░ 97.2% |
7,480 / 7,698 |
analytics_platform |
████████████████████ 98.0% |
2,102 / 2,145 |
metrics |
████████████████████ 98.3% |
2,363 / 2,405 |
live_debugger |
████████████████████ 99.2% |
613 / 618 |
field_notes |
████████████████████ 99.4% |
158 / 159 |
Report-only. Patch coverage = changed backend lines covered vs origin/master. Sorted lowest first.
Known gaps: lines covered only by Temporal tests show as uncovered; core line numbers may drift if master changed the same file.
✅ ClickHouse migration SQL — none
No ClickHouse migrations in the latest push.
7963871 to
62345fe
Compare
a118fbd to
6dc2162
Compare
959eb8b to
491eb9d
Compare
|
⏭️ Skipped snapshot commit because branch advanced to The new commit will trigger its own snapshot update workflow. If you expected this workflow to succeed: This can happen due to concurrent commits. To get a fresh workflow run, either:
|
af8b1c0 to
ba68628
Compare
|
⏭️ Skipped snapshot commit because branch advanced to The new commit will trigger its own snapshot update workflow. If you expected this workflow to succeed: This can happen due to concurrent commits. To get a fresh workflow run, either:
|
ba68628 to
b500b92
Compare
|
⏭️ Skipped snapshot commit because branch advanced to The new commit will trigger its own snapshot update workflow. If you expected this workflow to succeed: This can happen due to concurrent commits. To get a fresh workflow run, either:
|
b500b92 to
ba448b8
Compare
ba448b8 to
dc04399
Compare
CI note: the one red backend check is an unrelated, order-dependent shard flake (not from this PR)The failing check is
This is not a regression from this PR (which only touches the customer-analytics person-property consumer + its settings):
The proper fix belongs with the |
|
Follow-up on the red Django Core check: re-running the workflow reuses the fixed
Neither is related to this PR (which only touches the customer-analytics person-property consumer + its setting), both pass on the base branch, and this PR's own tests pass. Only a fresh push recomputes the shard matrix; the failures are non-required and do not block merge ( |
estefaniarabadan
left a comment
There was a problem hiding this comment.
Reviewed for correctness/reliability (proper review, not a merge gate — the code is dormant until charts#13077 + the flag land). The design is genuinely solid: shared Redis budget so replicas don't multiply the rate, commit-after-success with retry-in-place, DLQ for poison, heartbeat during waits. Inline notes below, roughly by severity. The one I'd most want addressed before enabling is the rate-setting read crashing the loop (line 75). Nice touch confirming CONSUMER_GROUP + the topic name line up with the KEDA scaler in the charts PR.
Note: I verified capture_internal's contract — permanent rejections come back via result.dropped (handled) and raise_for_status() is opt-in (you correctly don't call it), so the exception path is only for whole-request/infra errors. That makes the retry-as-transient choice correct today; see the line 218 note for the one edge to keep in mind.
dd1cfe9 to
a55c6d3
Compare
3e89cbb to
070ef93
Compare
f6899c2 to
1eb9b44
Compare
070ef93 to
8163ef5
Compare
Drains the warehouse person-property topic and sends each row as a $set through capture-internal, at a global rate ops can retune live. - PersonPropertyUpdateConsumer: token-bucket rate limiter (rate from a constance dynamic setting, default 5000/s, re-read each acquire), $set via capture_internal with process_person_profile=True, commit-after-success, poison -> DLQ, prometheus metrics. Kafka lag is the backpressure; team-keyed topic gives rough per-team isolation without per-team rate limiting in v1 - run_person_property_update_consumer management command - tests: token-bucket math, message->capture mapping, sent/retry/dlq outcomes Charts deploy + frontend + docs are the remaining follow-ups. Part of RFC PostHog/requests-for-comments-internal#1195.
Address reviewer feedback on the throttling consumer: - DLQ non-object JSON (`[]`, `"foo"`, `null`) that parses but can never map to a $set, instead of crashing build_capture_kwargs and wedging the partition. - Treat a terminal capture drop (invalid/stale token, validation) as poison and DLQ it, rather than redelivering forever; transient outcomes still retry. - Confirm DLQ delivery before committing the source offset — a failed DLQ write now returns RETRY so the poison message isn't lost from both topics. - Reuse a single lazily-created Kafka DLQ producer instead of constructing one per poison message. Generated-By: PostHog Code Task-Id: 652d5991-61f5-4d1b-92cc-a29757e9932d
Resolve type-checker and lint failures on the consumer: - Join KAFKA_HOSTS into the comma-separated string confluent expects, and skip tombstone (None-value) messages, fixing two mypy errors in the run loop. - Resolve the DLQ producer through posthog.kafka_client.routing.get_producer instead of constructing _KafkaProducer directly, satisfying the no-direct-kafka-producer-construction rule while still reusing one routed singleton producer across messages. Generated-By: PostHog Code Task-Id: 652d5991-61f5-4d1b-92cc-a29757e9932d
The product structure check only recognizes canonical backend directories (facade, presentation, logic, models, ...); backend/consumers/ was not one, so it bypassed import-linter enforcement and failed Repo checks. Move the consumer and its tests into backend/logic/ (alongside the existing person-property sync logic, matching how other products place Kafka consumers) and update the management command import. Generated-By: PostHog Code Task-Id: 652d5991-61f5-4d1b-92cc-a29757e9932d
The consumer connected using a bare KAFKA_HOSTS bootstrap string, ignoring the topic profile's transport security. Resolve the input topic through get_profile_settings so the consumer honours the profile's security.protocol and SASL credentials (and any KAFKA_TOPIC_ROUTING_OVERRIDES), matching how the other Confluent consumers connect and closing the plaintext-exposure gap. The DLQ produce path already routes through get_producer, so both sides now resolve the same profile. Generated-By: PostHog Code Task-Id: 652d5991-61f5-4d1b-92cc-a29757e9932d
On a RETRY outcome the run loop advanced to the next message, but Kafka offset commits are a high-water mark, so committing a later message's offset silently skipped the transiently-failed one (only reprocessed on restart/rebalance, if at all). Reprocess the same message in place with a short backoff until it reaches a terminal outcome (Kafka lag is the backpressure), and stop retrying on shutdown so the offset stays uncommitted and the message is redelivered on the next start. Generated-By: PostHog Code Task-Id: 652d5991-61f5-4d1b-92cc-a29757e9932d
NaN/Infinity floats survive json.loads but serialize to non-compliant JSON that the capture transport rejects as a non-terminal failure, which would retry the same record forever and wedge the partition. Reject them up front in build_capture_kwargs so they route to the DLQ like other poison messages. Generated-By: PostHog Code Task-Id: 1acc567f-f215-4b57-9b67-20859b35293f
… replicas Replace the per-process token bucket with the shared, Redis-backed egress limiter (posthog/egress/limiter) so the WAREHOUSE_PERSON_PROPERTY_SET_RATE_PER_SEC budget holds across every replica at once, rather than being multiplied by the replica count. The send now blocks against the shared budget (honoring shutdown) so Kafka lag stays the backpressure and offsets still commit only after a send.
… gated Start the shared health server (liveness/readiness/metrics on --health-port, default 8090) from the run command and heartbeat it each loop turn and during throttle/retry waits, so a merely rate-limited pod stays live. Readiness gating is what makes running multiple replicas safe, and it matches the probes the charts app already expects.
…de crash modes Address review feedback on the throttling consumer: - Guard the live rate setting: a non-numeric/empty constance value now falls back to the default and clamps to a floor instead of propagating a ValueError through the rate policy and crash-looping every replica at once. - Treat an unexpected grant (limiter/Redis) error as transient: back off and retry the acquire rather than letting it escape and crash the loop. - Bound the DLQ flush with a timeout and heartbeat around it so an unavailable DLQ broker can't block past the liveness timeout and get the pod killed mid-write. - Route permanent capture rejections (client-side/4xx) to the DLQ whether capture returns them as a whole-request error result or raises them, so a 4xx no longer retries forever and wedges the partition. Generated-By: PostHog Code Task-Id: 6776b9e1-0c18-48aa-a624-c56377390b9f
A request timeout (408) or rate-limit (429) from capture is a 4xx but transient, so it must retry, not go to the DLQ — otherwise a client driving the shared capture service into timeouts could get queued property updates dropped and committed. Route only permanent 4xx (bad/stale token, validation) to the DLQ, via a shared _is_permanent_client_error helper used by both the returned-result and raised-exception branches. Also fix the grant-error test's return type so mypy is satisfied. Generated-By: PostHog Code Task-Id: 6776b9e1-0c18-48aa-a624-c56377390b9f
8163ef5 to
6aa83af
Compare
Generated-By: PostHog Code Task-Id: ae74ae4f-c172-4384-945a-18126818b59e
When KAFKA_BASE64_KEYS is enabled the shared producer forces security.protocol=SSL and installs the decoded client cert/key/CA via helper.ssl_cert_config(). The person-property consumer only copied the profile protocol/SASL fields, so on self-hosted certificate-auth deployments it could fall back to plaintext or fail to join the secured cluster. Mirror the producer's base64 branch in a testable _build_consumer_config() and cover the SSL, SASL_SSL, and plaintext paths. Generated-By: PostHog Code Task-Id: ae74ae4f-c172-4384-945a-18126818b59e
Note
Final PR in the warehouse → person properties stack — #69979, #69983, and #70025 have all merged; rebased onto master so this diff is only the consumer. The consumer and its management command live in
products/warehouse_sources/(moved from customer_analytics, matching the sync workflow's move in #70025: team warehouse owns and operates the machinery). Themanage.py run_person_property_update_consumerentrypoint is unchanged, so the charts deployment (PostHog/charts#13077) needs no update.Problem
PR #70025 produces person-property
$setintents to a Kafka topic. This consumes them and sends each to capture-internal, but paced: these writes ride the shared real-time ingestion path, and an unpaced first sync of a big table would spike it. Ingestion asked for a capped, tunable send rate. This is the piece they care most about.Changes
PersonPropertyUpdateConsumer(Python, run as therun_person_property_update_consumermanagement command):WAREHOUSE_PERSON_PROPERTY_SET_RATE_PER_SEC, default 5000), re-read on each acquire so ops can retune live with no deploy.capture_internal(event_name="$set", process_person_profile=True, ...).team_id, so a big team's backlog only delays itself; no per-team rate limiting in v1.Warning
This is the component that actually writes to persons via the shared ingestion path. Before enabling the feature flag broadly it should be load-tested against a design-partner-sized table (first-run burst draining at the configured rate), watching consumer lag and our contribution to the person-write rate.
How did you test this code?
Automated tests I (Claude) ran locally, all green (12):
TokenBucket: waits ~1/rate when empty; a burst after idle accrual doesn't sleep. (Deterministic — injected clock + sleep, no real waiting.)build_capture_kwargs: maps to a$setwithprocess_person_profile=True; rejects messages missing token / distinct_id / properties.process_record: success commits (returnssent); capture failure and capture exception both returnretry(uncommitted); invalid JSON and poison shape returndlqand never call capture.The Kafka poll/commit loop and the real capture call are the boundaries and are mocked here; the loop itself (
run) is marked no-cover and will be exercised in dogfood once deployed (needs Kafka + capture). No charts deploy is in this PR.Automatic notifications
🤖 Agent context
Autonomy: Human-driven (agent-assisted)
Built with Claude Code (Opus 4.8), directed by me (Tom). Consumer modelled on the existing
products/data_warehouse/.../webhook_consumer(confluent-kafka, manual commit). Skill invoked:/writing-tests. Key decisions: the rate lives in constance (live-tunable, the thing ingestion cares about) rather than an env var needing a deploy; the message->capture mapping and the token bucket are pure and fully tested, while the poll loop is a thin boundary shell; retry-vs-DLQ is coarse in v1 (shape-invalid -> DLQ, everything else -> retry) — finer classification fromCaptureInternalResultis a noted follow-up. Remaining work from the RFC: the charts Deployment for this consumer, the Kafka topic provisioning, the frontend config UI, and docs.