Repository navigation
Conversation
|
Merging to
After your PR is submitted to the merge queue, this comment will be automatically updated with its status. If the PR fails, failure details will also be posted here |
github-actions
Bot
added
feature/feature-flags
🤖 CI report
|
🦔 PostHog Review reviewed this pull requestFound 4 must fix, 4 should fix, 2 consider. Published 10 findings (view the review). Resolved comments: 9 fixed, 6 already settled, 4 left for you |
👀 Auto-assigned reviewersThese soft owners were skipped because they only have minor changes here. Nothing blocks merge, so self-assign if you'd like a look:
Soft owners come from each directory's |
![veria-ai[bot]](https://avatars.githubusercontent.com/in/1723776?s=60&v=4){kind=small}
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: 1 · PR risk: 0/10 |
Prompt To Fix All With AI### Issue 1
posthog/dags/flag_evaluations_parity.py:125-128
**Eligibility predicate is missing**
When a `$feature_flag_called` event has a missing, empty, or non-string `$feature_flag`, the fork intentionally skips it, but this query still counts it as `only_in_events`, causing a false deficit alert.
### Issue 2
posthog/dags/flag_evaluations_parity.py:118-128
**Activation-day events create deficits**
When a team is enabled partway through the target day, its post-enable rows place it in the enabled set, but this query compares all events from that day. Pre-enable events therefore become false deficits even though the fork could not have written them.
### Issue 3
posthog/dags/flag_evaluations_parity.py:178-181
**Alert omits deployment identity**
When prod-US and prod-EU send alerts for overlapping team IDs to this shared channel, the message includes no deployment identifier, so responders cannot determine which regional deployment lost rows.
### Issue 4
posthog/dags/flag_evaluations_parity.py:64-65
**Historical rechecks target yesterday**
When an operator reruns the job to re-check the date named in an earlier alert, `_target_day()` unconditionally selects the previous UTC day and the config exposes no override, so the run silently checks the wrong date.
### Issue 5
posthog/dags/flag_evaluations_parity.py:236-242
**Schedule permits overlapping runs**
If a scheduled run remains active at the next tick or overlaps a manual run, this plain schedule has no active-run guard or concurrency key, causing duplicate ClickHouse load and potentially duplicate Slack alerts.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Reviews (1): Last reviewed commit: "Merge remote-tracking branch 'origin/mas..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
Adds an ingestion-monitoring Dagster job that checks daily parity between $feature_flag_called events in ClickHouse and rows written to the flag_evaluations fork, with Slack alerting for deficits and coverage for batching/truncation behaviors.
Changes:
- Introduces a new Dagster job + schedule to measure
flag_evaluationsvseventsparity per enabled team and alert to#alerts-ingestion. - Adds automated tests for alert triggering logic and per-team attribution across batched ClickHouse queries.
- Adds Prometheus instrumentation in the ingestion fork step (ack duration histogram) and registers the new job in the ingestion code location + owners mapping.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| posthog/dags/flag_evaluations_parity.py | New Dagster ops/job/schedule to query ClickHouse parity and post Slack alerts. |
| posthog/dags/tests/test_flag_evaluations_parity.py | New tests covering alert predicate behavior and batched attribution correctness. |
| posthog/dags/locations/ingestion.py | Registers the new job and schedule in the ingestion Dagster location. |
| posthog/dags/owners.yaml | Adds ownership routing patterns for the new job and its tests. |
| nodejs/src/ingestion/common/steps/event-processing/fork-flag-evaluations-step.ts | Adds a new Prometheus histogram for produce→ack duration and times the ack promise. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| /** | ||
| * How long a produce takes to be acknowledged. The pending-acks gauge above only | ||
| * separates "stalled" from "not stalled", so it moves once the lane is already | ||
| * holding up offset commits. This shows the lane slowing down while it still | ||
| * works, which is what gates widening the fork onto a lane other teams share. | ||
| */ | ||
| export const flagEvaluationsAckDuration = new Histogram({ | ||
| name: 'ingestion_flag_evaluations_ack_duration_seconds', | ||
| help: 'Time from queueing a flag_evaluations produce to the broker acknowledging it', | ||
| buckets: [0.01, 0.05, 0.1, 0.25, 0.5, 1, 2.5, 5, 10, 30], | ||
| }) |
There was a problem hiding this comment.
This needs the author to decide — I haven't changed code or the PR description. The code and the PR description genuinely disagree, and both look deliberate:
- The code adds
ingestion_flag_evaluations_ack_duration_seconds(it isn't on master) with a docstring that justifies it as the signal that 'gates widening the fork onto a lane other teams share.' - The PR description says that histogram was 'added and then removed' because
ingestion_outputs_latency_seconds/kafka_producer_produce_latency_secondsalready time queue-to-ack for this output and topic with finer buckets, and that 'the ingestion path is not touched.'
Both can't be the final intent. The decision:
- Keep the histogram — then correct the description's 'added and then removed' and 'ingestion path is not touched / unchanged' statements, since the PR does change ingestion instrumentation.
- Remove the histogram and rely on the existing per-output/topic latency metrics — then the ingestion path really is unchanged, matching the description.
One dependency to note: earlier in this review pass I made this histogram observe only on a successful ack (so a failed produce no longer skews the ack-latency signal). If you choose option 2, that change goes away with the metric; if option 1, it stays. I'd suggest deciding the intent first, since it determines whether the fork step should carry any bespoke ack-latency metric at all.
|
PostHog Review alpha 🦔 If you find any issues helpful - please reply "valid", "invalid", etc., for evaluation purposes 🙏 |
![posthog[bot]](https://avatars.githubusercontent.com/in/1203049?s=60&v=4){kind=small}
| def _target_day() -> date: | ||
| return (datetime.now(tz=UTC) - timedelta(days=1)).date() | ||
|
|
||
|
|
||
| @dagster.op | ||
| def measure_flag_evaluations_parity( | ||
| context: dagster.OpExecutionContext, | ||
| config: FlagEvaluationsParityConfig, | ||
| cluster: dagster.ResourceParam[ClickhouseCluster], | ||
| ) -> ParityResults: | ||
| """Compare flag_evaluations against events for every team the fork is writing for. | ||
|
|
||
| The produce-side counters answer "did we hand the row to Kafka", which is not the | ||
| question. Everything past the ack -- the Kafka engine table, the MV, the writable | ||
| Distributed, the shard -- is unobserved, and kafka_flag_evaluations sets | ||
| kafka_skip_broken_messages, so a malformed row is discarded after a successful produce. | ||
| Only comparing the two tables catches that. | ||
| """ | ||
|
|
||
| day = _target_day() |
There was a problem hiding this comment.
The target day depends on the run start time
Why we think it's a valid issue
- Checked:
_target_dayand its single call site, the schedule definition, the Dagster scheduler in the installed venv, and how other daily jobs in this repo derive their day. - Found:
posthog/dags/flag_evaluations_parity.py:63-64reads the wall clock, and line 82 binds it once per op execution. The op accepts no day.FlagEvaluationsParityConfig(lines 19-44) exposesenabled_team_lookback_days,teams_per_query, andmax_teamsonly, so a run cannot be pointed at a chosen date. The schedule at lines 236-242 passes no run config. - Found: replayed ticks are a real mechanism, not a hypothesis. The installed Dagster launches runs for missed ticks up to
DEFAULT_MAX_CATCHUP_RUNS = 5(.venv/lib/python3.13/site-packages/dagster/_core/scheduler/scheduler.py:211, applied in_scheduler/scheduler.py). A schedule that falls behind — a paused schedule, a daemon outage, an unavailable code location — resumes and submits runs for past ticks. Each of those runs computesnow() - 1 dayand checks yesterday, so the missed days are never checked and several runs repeat the same day. - Found: this repo already solves the same problem the way the suggestion describes, and records the reason.
products/signals/dags/inbox_ranking/dataset/dag.py:937-939states: "Derived from the tick rather than wall-clock now() so a delayed or replayed tick still builds the partition it was scheduled for."products/revenue_analytics/dags/exchange_rate.py:322andproducts/signals/dags/inbox_ranking/training/dag.py:466use the samecontext.scheduled_execution_time.date() - timedelta(days=1)form. The pattern is therefore idiomatic here, not an added abstraction. - Impact: after a schedule outage the job reports successful runs that cover a day it already covered, so gap days go unverified while the coverage looks complete. An operator who reads a deficit alert for a named date also cannot re-run that date, and must write the query by hand — the manual step this job exists to replace.
Issue description
day comes from the wall clock when the op starts. A delayed run after midnight checks the wrong day. Operators also cannot rerun an alert's date.
Suggested fix
Add a target_day config field. Have the schedule set it from context.scheduled_execution_time. Use yesterday only when no value exists.
Prompt to fix with AI (copy-paste)
## Context
@posthog/dags/flag_evaluations_parity.py#L63-82
<issue_description>
`day` comes from the wall clock when the op starts. A delayed run after midnight checks the wrong day. Operators also cannot rerun an alert's date.
</issue_description>
<issue_validation>
- **Checked:** `_target_day` and its single call site, the schedule definition, the Dagster scheduler in the installed venv, and how other daily jobs in this repo derive their day.
- **Found:** `posthog/dags/flag_evaluations_parity.py:63-64` reads the wall clock, and line 82 binds it once per op execution. The op accepts no day. `FlagEvaluationsParityConfig` (lines 19-44) exposes `enabled_team_lookback_days`, `teams_per_query`, and `max_teams` only, so a run cannot be pointed at a chosen date. The schedule at lines 236-242 passes no run config.
- **Found:** replayed ticks are a real mechanism, not a hypothesis. The installed Dagster launches runs for missed ticks up to `DEFAULT_MAX_CATCHUP_RUNS = 5` (`.venv/lib/python3.13/site-packages/dagster/_core/scheduler/scheduler.py:211`, applied in `_scheduler/scheduler.py`). A schedule that falls behind — a paused schedule, a daemon outage, an unavailable code location — resumes and submits runs for past ticks. Each of those runs computes `now() - 1 day` and checks yesterday, so the missed days are never checked and several runs repeat the same day.
- **Found:** this repo already solves the same problem the way the suggestion describes, and records the reason. `products/signals/dags/inbox_ranking/dataset/dag.py:937-939` states: "Derived from the tick rather than wall-clock now() so a delayed or replayed tick still builds the partition it was scheduled for." `products/revenue_analytics/dags/exchange_rate.py:322` and `products/signals/dags/inbox_ranking/training/dag.py:466` use the same `context.scheduled_execution_time.date() - timedelta(days=1)` form. The pattern is therefore idiomatic here, not an added abstraction.
- **Impact:** after a schedule outage the job reports successful runs that cover a day it already covered, so gap days go unverified while the coverage looks complete. An operator who reads a deficit alert for a named date also cannot re-run that date, and must write the query by hand — the manual step this job exists to replace.
</issue_validation>
## Task
Investigate the issue and solve it
<potential_solution>
Add a `target_day` config field. Have the schedule set it from `context.scheduled_execution_time`. Use yesterday only when no value exists.
</potential_solution>
There was a problem hiding this comment.
This is a real bug and worth fixing, but it needs a human to land and verify because it changes how the job is scheduled and launched — deploy-time Dagster wiring I can't exercise unattended here (no Dagster daemon in this environment).
What a human needs to decide/do:
- Add a
target_day: str | None = Nonefield (ISOYYYY-MM-DD) toFlagEvaluationsParityConfig. - In
measure_flag_evaluations_parity, usedate.fromisoformat(config.target_day)when set, else fall back to_target_day()(yesterday, UTC). - Replace the
dagster.ScheduleDefinitionwith a@dagster.schedulefunction that derivestarget = context.scheduled_execution_time.date() - timedelta(days=1)and returnsdagster.RunRequest(run_key=target.isoformat(), run_config=dagster.RunConfig(ops={"measure_flag_evaluations_parity": FlagEvaluationsParityConfig(target_day=target.isoformat())})). This is the same tick-derived pattern already used inproducts/revenue_analytics/dags/exchange_rate.pyandproducts/signals/dags/inbox_ranking/..., adapted to inject run config since this job is not partitioned.
Why it needs eyes on a real Dagster run: the two behaviors this fixes — a catchup/replayed tick checking the day it was scheduled for (not wall-clock yesterday), and run_key deduping a re-evaluated tick — only surface when the daemon launches the runs, and a wrong run_config shape or op-name key would silently break the daily alert rather than fail a test. A dagster.build_schedule_context(scheduled_execution_time=...) unit test asserting the emitted run_config, plus one op test passing target_day, would lock it in. I did not change any code for this thread.
| enabled_rows = client.execute( | ||
| f""" | ||
| SELECT team_id, count() AS rows | ||
| FROM {settings.CLICKHOUSE_DATABASE}.{FLAG_EVALUATIONS_TABLE} | ||
| WHERE toDate(timestamp) >= %(lookback_start)s AND toDate(timestamp) <= %(day)s | ||
| GROUP BY team_id | ||
| ORDER BY rows DESC | ||
| """, | ||
| {"lookback_start": day - timedelta(days=config.enabled_team_lookback_days), "day": day}, | ||
| settings=query_settings, | ||
| ) | ||
|
|
||
| ranked_teams = [int(row[0]) for row in enabled_rows] |
There was a problem hiding this comment.
Do not infer enablement from successful writes
Why we think it's a valid issue
- Checked: the enabled-team query, the lookback config, the no-alert path, the fork's gate, and where the allowlist actually lives.
- Found: the discovery signal and the monitored signal are the same.
posthog/dags/flag_evaluations_parity.py:89-101buildsranked_teamsonly from teams that already have aflag_evaluationsrow in the window. A team whose rows never land — the topic cleared, the lane's mode off, or loss downstream from the first row — never enters the list, soparity_alert_linesreturnsNone(lines 175-176), the op logs "Parity holds" (line 214), and nothing is posted. Silence is exactly the signal an operator reads as a pass when gating a team. - Found: the reverse half produces false alerts, and its trigger is a designed operation. The window keeps a team for
enabled_team_lookback_days(default 7, lines 22-29, 97). After a team leaves the allowlist, it still has rows in the window, so the comparison runs for a day where the fork wrote nothing while$feature_flag_calledevents still land. Every one of those events counts asonly_in_events, and any non-zero deficit alerts (lines 170-176). The alert then fires at full day size, each day, for a week. - Found: disablement is an operational action, not a hypothesis.
nodejs/src/ingestion/common/flag-evaluations/flag-evaluations-service.ts:14-16documentsexcludedTeamsas an "Escape hatch: teams never forked", and lines 29-33 describe shedding the fork during an incident by clearing the mode and the topic. Shedding the fork therefore starts a week of maximum-size deficit alerts right after an incident. - Found: the suggested remedy is a cross-system change, not an edit to this file. The allowlist is deployment env config read by the Node ingestion service —
INGESTION_FLAG_EVALUATIONS_TEAMS,INGESTION_FLAG_EVALUATIONS_EXCLUDED_TEAMS, andINGESTION_FLAG_EVALUATIONS_MODE(nodejs/src/ingestion/config.ts:247-249,flag-evaluations-service.ts:47-54). No queryable store holds it, so a shared source has to be built first. - Impact: the monitor cannot see the worst case it exists to catch, a team that loses every row, and it raises loud false deficits for a week after any deliberate disablement.
- Priority: lowered to
should_fix. Both effects are confirmed, but they degrade an internal monitor rather than break production, lose data, or change a contract, and closing the blind spot needs an enablement source that does not exist in queryable form today.
Issue description
The query discovers a team only after flag_evaluations accepts a row. A total outage for a newly enabled team leaves that team outside ranked_teams. The run then reports clean parity. Historical rows also keep disabled teams in the enabled set until the lookback expires.
Suggested fix
Read current enablement and activation time from a source that does not depend on successful target writes. Share this source with ingestion. Use table rows only for ranking or validation.
Prompt to fix with AI (copy-paste)
## Context
@posthog/dags/flag_evaluations_parity.py#L89-101
<issue_description>
The query discovers a team only after `flag_evaluations` accepts a row. A total outage for a newly enabled team leaves that team outside `ranked_teams`. The run then reports clean parity. Historical rows also keep disabled teams in the enabled set until the lookback expires.
</issue_description>
<issue_validation>
- **Checked:** the enabled-team query, the lookback config, the no-alert path, the fork's gate, and where the allowlist actually lives.
- **Found:** the discovery signal and the monitored signal are the same. `posthog/dags/flag_evaluations_parity.py:89-101` builds `ranked_teams` only from teams that already have a `flag_evaluations` row in the window. A team whose rows never land — the topic cleared, the lane's mode off, or loss downstream from the first row — never enters the list, so `parity_alert_lines` returns `None` (lines 175-176), the op logs "Parity holds" (line 214), and nothing is posted. Silence is exactly the signal an operator reads as a pass when gating a team.
- **Found:** the reverse half produces false alerts, and its trigger is a designed operation. The window keeps a team for `enabled_team_lookback_days` (default 7, lines 22-29, 97). After a team leaves the allowlist, it still has rows in the window, so the comparison runs for a day where the fork wrote nothing while `$feature_flag_called` events still land. Every one of those events counts as `only_in_events`, and any non-zero deficit alerts (lines 170-176). The alert then fires at full day size, each day, for a week.
- **Found:** disablement is an operational action, not a hypothesis. `nodejs/src/ingestion/common/flag-evaluations/flag-evaluations-service.ts:14-16` documents `excludedTeams` as an "Escape hatch: teams never forked", and lines 29-33 describe shedding the fork during an incident by clearing the mode and the topic. Shedding the fork therefore starts a week of maximum-size deficit alerts right after an incident.
- **Found:** the suggested remedy is a cross-system change, not an edit to this file. The allowlist is deployment env config read by the Node ingestion service — `INGESTION_FLAG_EVALUATIONS_TEAMS`, `INGESTION_FLAG_EVALUATIONS_EXCLUDED_TEAMS`, and `INGESTION_FLAG_EVALUATIONS_MODE` (`nodejs/src/ingestion/config.ts:247-249`, `flag-evaluations-service.ts:47-54`). No queryable store holds it, so a shared source has to be built first.
- **Impact:** the monitor cannot see the worst case it exists to catch, a team that loses every row, and it raises loud false deficits for a week after any deliberate disablement.
- **Priority:** lowered to `should_fix`. Both effects are confirmed, but they degrade an internal monitor rather than break production, lose data, or change a contract, and closing the blind spot needs an enablement source that does not exist in queryable form today.
</issue_validation>
## Task
Investigate the issue and solve it
<potential_solution>
Read current enablement and activation time from a source that does not depend on successful target writes. Share this source with ingestion. Use table rows only for ranking or validation.
</potential_solution>
There was a problem hiding this comment.
This is a real gap in the monitor, but it needs a human decision because the fix is a cross-system infrastructure change, not an edit to this file — so I have not changed code for it.
What's confirmed: the job derives its enabled-team set from the flag_evaluations table itself, so (a) a team that loses every row is never discovered and the run reports clean parity — the exact worst case the monitor exists to catch reads as a pass — and (b) because the enabled set keeps a team for the lookback window (default 7 days), a team taken off the allowlist keeps historical rows and its $feature_flag_called events read as a full-size deficit every day for a week, including right after a deliberate incident shed of the fork.
Why a human must decide: the fork's allowlist lives only in Node deployment env config (INGESTION_FLAG_EVALUATIONS_TEAMS / _EXCLUDED_TEAMS / _MODE), read by FlagEvaluationsService. There is no queryable store of enablement today. Reading current enablement and activation time from a source independent of successful writes — and sharing that source with ingestion — means building that shared source first. It also reverses a deliberate choice recorded in the PR description (deriving the team list from the table 'so the two cannot drift').
The decision to make: either accept and document this as a known limitation of a v1 internal monitor (the table-derived set avoids allowlist drift but is blind to total loss and noisy for a week after disablement), or invest in a shared, queryable enablement source that both ingestion and this job read, using the table only for ranking/validation. My separately-landed activation-day skip does not cover this — it only handles a team switched on during the checked day.
| SELECT team_id, uuid, 0 AS is_fe, 1 AS is_ev | ||
| FROM {settings.CLICKHOUSE_DATABASE}.events | ||
| WHERE team_id IN %(teams)s AND toDate(timestamp) = %(day)s | ||
| AND event = %(event)s |
There was a problem hiding this comment.
Exclude events from lanes that never run the fork
Why we think it's a valid issue
- Checked: the fork's lane gate, the lane sets, the events-side predicate in the parity query, the capture rerouting rule and its threshold, and the
$feature_flag_calleddedup service that reasons about the same lanes. - Found: the premise is exact.
FLAG_EVALUATIONS_ALLOWED_LANESis[...REALTIME_INGESTION_LANES, null](nodejs/src/ingestion/common/flag-evaluations/flag-evaluations-service.ts:66), andcreateFlagEvaluationsServicereturnsundefinedfor any other lane (lines 88-92). The delayed lanes arehistoricalandasync(nodejs/src/ingestion/config.ts:88).overflowis a real-time lane, so it does fork. - Found: the events side applies no matching restriction.
posthog/dags/flag_evaluations_parity.py:125-128selects everyeventsrow for the day withevent = '$feature_flag_called'. A row from a delayed lane therefore lands inonly_in_events, and any non-zero deficit alerts (lines 170-176), so one such event is enough to fire. - Found:
$feature_flag_calledreally does traverse delayed lanes in this system.nodejs/src/ingestion/common/feature-flag-called-dedup/feature-flag-called-dedup-service.ts:160-171excludes the same two lanes for this event type, with the reason that "a backfilled event is not a duplicate of the live event that claimed its tuple an hour ago". - Found: a concrete timing window exists at the normal cadence. Capture reroutes when the event is older than
historical_rerouting_threshold_days, default 1 (rust/capture/src/config.rs:280-281). The job compares day D at 04:00 on D+1, so the reachable age is at most 28 hours. Events timestamped in the first hours of D that arrive more than 24 hours late satisfy both, take the historical lane, are never forked, and count as deficits. - Found: two limits on how often this bites, which I could not resolve from this repo.
enable_historical_reroutingdefaults tofalse(rust/capture/src/config.rs:277-278) and the production value lives in deployment config, and theasynclane's routing rule is not defined here. Explicit batch imports are not a source at this cadence, becausevalidate-historical-migration.ts:30requires an age of at least 48 hours, beyond the 28-hour reach. - Impact: the comparison counts events the fork was never going to write, so a deficit alert can name a team whose fork is working. With no threshold on the deficit, a low but steady rate of late events produces a recurring false alarm, which is the outcome the excess rule was written to avoid.
Issue description
createFlagEvaluationsService enables the fork only for real-time lanes. This query reads events from all lanes. An event from the historical or async lane can therefore create a false deficit.
Suggested fix
Carry a fork-eligible or ingestion-lane marker into events and filter on it. Alternatively, compare against a source that only real-time lanes produce.
Prompt to fix with AI (copy-paste)
## Context
@posthog/dags/flag_evaluations_parity.py#L125-128
<issue_description>
`createFlagEvaluationsService` enables the fork only for real-time lanes. This query reads events from all lanes. An event from the `historical` or `async` lane can therefore create a false deficit.
</issue_description>
<issue_validation>
- **Checked:** the fork's lane gate, the lane sets, the events-side predicate in the parity query, the capture rerouting rule and its threshold, and the `$feature_flag_called` dedup service that reasons about the same lanes.
- **Found:** the premise is exact. `FLAG_EVALUATIONS_ALLOWED_LANES` is `[...REALTIME_INGESTION_LANES, null]` (`nodejs/src/ingestion/common/flag-evaluations/flag-evaluations-service.ts:66`), and `createFlagEvaluationsService` returns `undefined` for any other lane (lines 88-92). The delayed lanes are `historical` and `async` (`nodejs/src/ingestion/config.ts:88`). `overflow` is a real-time lane, so it does fork.
- **Found:** the events side applies no matching restriction. `posthog/dags/flag_evaluations_parity.py:125-128` selects every `events` row for the day with `event = '$feature_flag_called'`. A row from a delayed lane therefore lands in `only_in_events`, and any non-zero deficit alerts (lines 170-176), so one such event is enough to fire.
- **Found:** `$feature_flag_called` really does traverse delayed lanes in this system. `nodejs/src/ingestion/common/feature-flag-called-dedup/feature-flag-called-dedup-service.ts:160-171` excludes the same two lanes for this event type, with the reason that "a backfilled event is not a duplicate of the live event that claimed its tuple an hour ago".
- **Found:** a concrete timing window exists at the normal cadence. Capture reroutes when the event is older than `historical_rerouting_threshold_days`, default 1 (`rust/capture/src/config.rs:280-281`). The job compares day D at 04:00 on D+1, so the reachable age is at most 28 hours. Events timestamped in the first hours of D that arrive more than 24 hours late satisfy both, take the historical lane, are never forked, and count as deficits.
- **Found:** two limits on how often this bites, which I could not resolve from this repo. `enable_historical_rerouting` defaults to `false` (`rust/capture/src/config.rs:277-278`) and the production value lives in deployment config, and the `async` lane's routing rule is not defined here. Explicit batch imports are not a source at this cadence, because `validate-historical-migration.ts:30` requires an age of at least 48 hours, beyond the 28-hour reach.
- **Impact:** the comparison counts events the fork was never going to write, so a deficit alert can name a team whose fork is working. With no threshold on the deficit, a low but steady rate of late events produces a recurring false alarm, which is the outcome the excess rule was written to avoid.
</issue_validation>
## Task
Investigate the issue and solve it
<potential_solution>
Carry a fork-eligible or ingestion-lane marker into events and filter on it. Alternatively, compare against a source that only real-time lanes produce.
</potential_solution>
There was a problem hiding this comment.
This is a real false-alert path, but I have not changed code because the fix needs a signal that does not exist in queryable form today — a human has to decide how to source it.
Confirmed: the fork only runs on real-time lanes (main, overflow, turbo, team2), while delayed lanes (historical, async) never fork. The parity query's events side counts every $feature_flag_called row for the day regardless of lane, so a delayed-lane event lands in only_in_events. With no deficit threshold, a low but steady rate of late events that get rerouted to the historical lane produces a recurring false deficit that can name a team whose fork is working — the exact noise the excess rule was written to avoid.
Why it needs a human: the ingestion lane is a consumer/env concept (INGESTION_LANE), not a property on the event. The events table has no lane column — only historical_migration, which flags explicit >=48h batch imports, not the age-rerouted historical-lane events in the reachable window here. So closing this cleanly means either (a) carrying a fork-eligible / ingestion-lane marker onto events and filtering on it (a change across Node ingestion, the events schema/materialization, and this query), or (b) comparing against a source only real-time lanes produce. Both are design changes, not an edit to this file.
I considered and rejected an in-file approximation by event age (inserted_at vs timestamp): the rerouting threshold and enablement live in deployment config not readable here, the async lane's routing isn't defined in this repo, and an age filter could wrongly drop a genuinely-late real-time event that was forked — hiding a real deficit, which is worse for a loss monitor than the false positive it fixes.
This shares a root cause with the separately-raised enablement-source finding: the job has no queryable view of what the fork actually processed, only the table it writes to and the raw events. A decision on that shared source would resolve both.
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
| from posthog.dags.common import JobOwners, settings_with_log_comment | ||
| from posthog.models.flag_evaluations.sql import FLAG_EVALUATIONS_SOURCE_EVENT, FLAG_EVALUATIONS_TABLE | ||
|
|
||
| FLAG_EVALUATIONS_SLACK_CHANNEL = "#alerts-ingestion" |
There was a problem hiding this comment.
The Slack channel is hardcoded as #alerts-ingestion but the PR description states the job should post to #alerts-feature-flags. This mismatch means alerts will go to the wrong channel, potentially causing them to be missed by the team responsible for feature flags.
FLAG_EVALUATIONS_SLACK_CHANNEL = "#alerts-feature-flags"| FLAG_EVALUATIONS_SLACK_CHANNEL = "#alerts-ingestion" | |
| FLAG_EVALUATIONS_SLACK_CHANNEL = "#alerts-feature-flags" |
Spotted by Graphite
Is this helpful? React 👍 or 👎 to let us know.
| }) | ||
| const ack = outputs.queueMessages(FLAG_EVALUATIONS_OUTPUT, messages) | ||
| flagEvaluationsPendingAcks.inc(messages.length) | ||
| const stopAckTimer = flagEvaluationsAckDuration.startTimer() |
There was a problem hiding this comment.
Timer is started after queueMessages() has already been called (line 107), not before. This means the histogram measures only the time between starting the timer and the promise settling, missing the initial queueing time. The timer should be started before line 107 to capture the full produce-to-ack duration:
const stopAckTimer = flagEvaluationsAckDuration.startTimer()
const ack = outputs.queueMessages(FLAG_EVALUATIONS_OUTPUT, messages)
flagEvaluationsPendingAcks.inc(messages.length)Spotted by Graphite
Is this helpful? React 👍 or 👎 to let us know.
Adds a daily Dagster job that compares flag_evaluations with events per team and posts to Slack when a row the fork wrote never reached events. The existing counters increment on the Kafka ack, so nothing past it is observed: the Kafka engine table, the MV, the writable Distributed, and the shard. kafka_flag_evaluations sets kafka_skip_broken_messages, so a malformed row is discarded after a successful produce. Only a table to table comparison sees that. The job takes its team list from flag_evaluations itself rather than a mirror of the deployment allowlist, so the two cannot drift, and reads a lookback window so a day where the fork wrote nothing for a team reads as a deficit rather than as the team not being enabled. Excess never alerts on its own. flag_evaluations carries a permanent surplus because clients re-send event uuids and its plain MergeTree cannot collapse them, so alerting on any difference would fire daily for every team. A deficit alerts at any size, because it has measured zero. Also adds an ack-duration histogram to the fork. The pending-acks gauge only separates stalled from not stalled, which moves once the lane is already holding up offset commits. Generated-By: PostHog Desktop Task-Id: 125630eb-acdf-45c7-9574-ae8c1088540d
A failed produce called stopAckTimer(), so it recorded a sample into the flag_evaluations ack-latency histogram. Fast local rejects like MessageSizeTooLarge settle before any broker round trip, so those near-zero samples pull the reported ack latency down. That histogram gates widening the fork onto a shared lane, so a failure should not read as a healthy ack. Observe the histogram only in the success arm. Generated-By: PostHog Desktop Task-Id: 67542d94-107a-43d2-81da-16cea3b8b99e
The events side of the parity comparison counted every $feature_flag_called row, but the fork only writes a flag_evaluations row when $feature_flag is a non-empty string (hasFlagKey). An event the fork skips on purpose therefore read as a deficit and would fire a false loss alert. Add AND JSONExtractString(properties, '$feature_flag') != '' to the events branch, mirroring the fork's typeof-string check. Update the ClickHouse-backed test so matching events carry a valid $feature_flag, and add missing, empty, and non-string cases that must not read as a deficit. Generated-By: PostHog Desktop Task-Id: 67542d94-107a-43d2-81da-16cea3b8b99e
prod-US and prod-EU post the parity alert to the same channel, and team ids come from a per-region sequence, so team `12345` names a different team in each region. An operator reading a deficit alert could not tell which region lost rows. Append an Environment line with settings.CLOUD_DEPLOYMENT in the report op, matching the pattern in the sibling dags alert senders. The early non-prod guard above guarantees the value is set. parity_alert_lines stays a pure function with no settings access. Generated-By: PostHog Desktop Task-Id: 67542d94-107a-43d2-81da-16cea3b8b99e
The enabled-team query kept every team with a flag_evaluations row in the lookback window, including a team whose first row lands on the checked day. The fork is gated per team at run time, so a team switched on mid-day has $feature_flag_called events from before the allowlist reached the pods that reach events with no flag_evaluations row. The comparison counted those as a deficit, so the rollout gate this job provides read red on every activation. Return min(toDate(timestamp)) per team and drop a team whose earliest row is the checked day, before applying max_teams. Log how many were skipped. Add a test team switched on during the day whose would-be deficit must not surface, and give the compared teams an earlier-day row so they stay in the enabled set. Generated-By: PostHog Desktop Task-Id: 67542d94-107a-43d2-81da-16cea3b8b99e
The parity comparison's outer GROUP BY team_id emits a row only for a team that had rows on the day, so an idle team that was queried but wrote nothing that day is dropped from `checked`. The Slack summary used len(checked) as the checked-team count, which understated coverage and could contradict the "N enabled teams were not checked" truncation line beside it. Report teams_enabled - teams_truncated as the checked count and state how many teams had rows on the day, matching the alert format in the PR description. Split the Dagster run metadata the same way. Add a unit test that pins the summary to the queried count. Generated-By: PostHog Desktop Task-Id: 67542d94-107a-43d2-81da-16cea3b8b99e
The shared Dagster cluster resource disables both ClickHouse ceilings (max_execution_time and max_memory_usage are "0"), so the per-team parity comparison ran with no memory limit and no deadline on the cluster that serves customer queries. Its group-by builds one aggregate state per uuid across both tables for a whole day, and the batch order puts the highest-volume teams first, so as the fork widens the query threatens shared memory. The retry policy re-runs a total-memory or socket-timeout failure up to 8 times, so a heavy query repeated at full cost. Cap the comparison query with max_execution_time, max_memory_usage of 100 GB, and distributed_aggregation_memory_efficient, following the pattern in posthog/dags/sessions.py. A per-query memory or execution-time breach is not retried by the policy, so a heavy batch now fails once. The ceilings are generous guardrails for a teams_per_query-sized batch; that knob still lowers per-query scope. Generated-By: PostHog Desktop Task-Id: 67542d94-107a-43d2-81da-16cea3b8b99e
The parity schedule was a bare dagster.ScheduleDefinition, so every tick submitted a run with no active-run check. After a schedule outage Dagster replays up to 5 catch-up ticks close together, which would run several comparisons at once against the shared events cluster and post duplicate alerts for the same day. Convert it to a @dagster.schedule function guarded by @skip_if_already_running, the documented helper already used by peers on the same 0 4 * * * cron. The run behavior is otherwise unchanged. The separate execution-time cap bounds a single run's duration but does not prevent concurrent catch-up runs, which this guard does. Generated-By: PostHog Desktop Task-Id: 67542d94-107a-43d2-81da-16cea3b8b99e
The op derived the checked day only from the wall clock (yesterday, UTC) with no override, so an operator rerunning to re-check the date a past alert named silently checked yesterday instead. Add a target_day config field (YYYY-MM-DD) and resolve the day through a small helper: the explicit override when set, else yesterday. A manual run can now re-check a named date via run config. Scheduled runs pass no config, so their behavior is unchanged. Deriving the day from the scheduled tick, so a delayed or replayed scheduled run checks the day it was scheduled for, is a separate change to the schedule and is not included here. Generated-By: PostHog Desktop Task-Id: 67542d94-107a-43d2-81da-16cea3b8b99e
The parity alert posted only blocks with no top-level text, so Slack had no notification or accessibility fallback and could drop the message more easily when it rejects the rich blocks. Pass text alongside blocks, reusing the same joined lines, so the fallback carries the full message including the deployment and the truncation clause. Both the text and the block now reference one message value, so they cannot drift. Matches the pattern in posthog/dags/slack_alerts.py. The deployment identifier the same review raised is already present in the alert lines from an earlier commit this review pass. Generated-By: PostHog Desktop Task-Id: 67542d94-107a-43d2-81da-16cea3b8b99e
The events branch kept any $feature_flag whose JSON value was not an empty string, but JSONExtractString renders a number, boolean, object or array as its text, so 123 came back as '123' and passed the non-empty check. The fork writes a row only for a non-empty string, so each of those events had no flag_evaluations row by design and was counted as a lost one. A deficit alerts at any size, so a single such event a day would alert every morning for a fork that is working. Measured against ClickHouse 26.6.2.158: only null, "" and a missing key return ''. JSONType now rejects the other types before the empty-string check. Removing the JSONType line makes the seeded numeric row read as a deficit, so the existing ClickHouse case covers the regression. Also alert when no team is enabled, rather than reporting the run as clean. A cleared allowlist stops the fork writing, and once the lookback window passes those teams leave the query entirely, which produces no deficit and no message. Log the alert body before posting, because a Slack failure is logged and swallowed, and the per-team deficit lines exist nowhere else. Cover the max_teams truncation counts, which had no test. The excess docstring now cites a uuid-level production measurement. The re-sent-uuid explanation it replaces cannot hold, because the comparison groups by (team_id, uuid) before counting.
771eb23 to
2552e5f
Compare
|
Closing this unmerged. A daily job is more machinery than this rollout gate needs. Parity gets checked by hand instead, through Metabase, before each widening of the fork. A follow-up PR adds the query as a runbook next to the table definition, with the reasons behind the parts that are easy to get wrong. What an always-on check needed, and a spot check does not:
The ack-latency histogram goes too. Worth revisiting once the allowlist reaches |
Problem
Nothing tells us when a row the flag-evaluations fork wrote never reaches the events table.
kafka_flag_evaluationssetskafka_skip_broken_messages, so a malformed row is discarded after a successful produce.Changes
#alerts-ingestionwhen an enabled team has an event with no matching flag_evaluations row.$feature_flagto be a non-empty JSON string, matching the fork's own condition.JSONExtractStringalone did not do that. It renders a number, boolean, object or array as its text, so123returned'123'and passed an empty-string check.JSONTyperejects those types before the empty-string check.A deficit alert reads like this:
The job alerts through Slack rather than a metric because
posthog/dags/has no metrics-push path. The oneprometheus_clientcounter there is documented as dying with its pod.How did you test this code?
Automated tests in
posthog/dags/tests/test_flag_evaluations_parity.py, each aimed at a named regression:$feature_flagthat is missing, empty, or a number, and a team switched on mid-day. It also asserts themax_teamscounts.Deleting the
JSONTypeline makes that case report two deficit rows where one is correct, so the numeric row covers the regression.The excess figure comes from a 7-day prod-US query, grouped by
(team_id, uuid)with the job's own filter.Excess and deficit by day, prod-US
The two large deficits are the fork's own rollout. The job skips a team on its enable day.
Note
That same query read 3.44 TiB and peaked at 30.7 GiB on one coordinating host, against a 40 GB cap. It covered 7 days and every team at once where the job covers 1 day in batches of 20, so a real run is smaller. Whether the comparison should instead run shard-local is still open.
Not run: repo-wide mypy, and the wider
posthog/dagssuites. No manual testing.Automatic notifications
Docs update
None.
🤖 Agent context
Autonomy: Human-driven (agent-assisted)
Written by Claude Code in PostHog Desktop. Skills invoked:
/writing-tests,/writing-pr-descriptions,/writing-code-comments,/review-code,/explain-open, and/metabase-prod-query.The first plan put a
team_idlabel on the fork's outcome counter. That was dropped: the rollout ends with the allowlist set to*, which would make the label roughly nine thousand teams wide per pod. The parity job answers per-team questions instead.A review pass found the events branch counted every
$feature_flag_calledrow. The first fix usedJSONExtractString(...) != '', which still kept non-string values. Measuring against ClickHouse 26.6.2.158 showed onlynull,""and a missing key return''. The same pass found the stated reason for ignoring excess did not survive the query's ownGROUP BY uuid.Two branches of this work ran in parallel and reached several of the same fixes independently. This branch keeps both, with the duplicate work merged rather than force-pushed over.
Nothing in this PR carries material from the session that is not already public. The tests use invented team IDs and UUIDs.