Skip to content

fix(flags): measure backfill consumer lag from the Kafka message time - #111071

Merged
trunk-io[bot] merged 2 commits into
masterfrom
haacked/flag-evaluations-backfill-lag-kafka-time
Oct 2, 2026
Merged

trunk-io[bot] merged 2 commits into
masterfrom
haacked/flag-evaluations-backfill-lag-kafka-time

Conversation

@haacked

@haacked haacked commented Oct 2, 2026

Copy link
Copy Markdown
Contributor

Problem

  • The flag_evaluations backfill stops copying when the Kafka consumer is more than an hour behind, so it never inserts rows that Kafka delivers a second time. It measures that lag from inserted_at.
  • flag_evaluations is moving to the events approach: the MV will stamp inserted_at with the time ClickHouse consumes the message, as the events MV does. The current Kafka-time inserted_at lets the last-called sync skip flag calls when the ClickHouse consumer stalls.
  • After that change, a consumer working through a backlog writes rows stamped now. The lag check would read it as caught up, and the backfill would copy rows that Kafka then delivers again.
  • This PR lands first so the MV change can follow. It replaces the written_at column approach in feat(flags): add written_at column to flag_evaluations #110412.

Changes

  • The lag check measures each partition's lag from _timestamp, the Kafka message time.
  • Every Kafka row's inserted_at equals its _timestamp today, so the check's result does not change until the MV change lands.
  • The inserted_at filters stay. The 7-day lookback prunes on the inserted_at skip index, and inserted_at != timestamp still excludes copied rows.
  • No user-visible change.

Next, each its own PR:

  1. A migration-only PR, after this deploys, changes flag_evaluations_mv to select now64() AS inserted_at.
  2. After that deploys in both regions, the last-called sync drops its separate 15-minute flag_evaluations buffer and uses the replication buffer (feat(flags): read last_called_at sync calls from flag_evaluations behind a setting #110150 or a follow-up).

How did you test this code?

  • Added a case to test_backfill_fails_without_copying_when_a_safety_check_fails: a consumer writing a two-hour-old backlog now. With the old max(inserted_at) query the backfill copies rows and the case fails. With this change the backfill stops.
  • The Kafka-path seed helper now sets _timestamp, and a case can write a row later than its Kafka time.

Test rationale: The new case catches the lag check moving back to a write-time column. No existing case separates a row's write time from its Kafka time, so the existing cases pass with either query.

Release status

  • No feature flag controls this change
  • This change is behind a feature flag and is not available to users
  • This change makes a previously flagged feature available to everyone

Automatic notifications

  • Publish to changelog?

Docs update

None.

🤖 Agent context

Autonomy: Human-driven (agent-assisted)

Agent: Claude Code, Opus 5.5

  • Claude Code wrote the change through the /go pipeline from a plan the author approved.
  • The author chose to follow the events approach for inserted_at after reviewing the written_at column design in feat(flags): add written_at column to flag_evaluations #110412. That PR closes in favor of this sequence.
  • Production checks, run with the author's approval: the producer never sets inserted_at, and the backfill job is the only tagged reader of flag_evaluations.inserted_at.
  • Skills invoked: /writing-tests, /writing-code-comments, /simplify, /comment-cleanup, /reviewing-with-coderabbit, /writing-pr-descriptions, /metabase-prod-query.
  • CodeRabbit CLI ran with --deep against master: 0 findings.
  • Duplicate search for open PRs on the flag_evaluations backfill lag found none.

https://claude.ai/code/session_01PjQA3YgRBZqNdm7qCpBVck

@haacked haacked self-assigned this Oct 2, 2026
@trunk-io

trunk-io Bot commented Oct 2, 2026 •

Copy link
Copy Markdown

😎 Merged successfully - details.

@haacked
haacked requested a balanced review from Copilot October 2, 2026 18:44
@posthog

posthog Bot commented Oct 2, 2026 •

Copy link
Copy Markdown
Contributor

🦔 PostHog Review reviewed this pull request

Nothing worth raising this time. Enjoy the moment:

A panda relaxing and waving

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟢 Approval recommended

The query preserves existing filtering behavior while the focused regression test verifies the intended backlog safety check.

Review effort: Balanced
Findings: None

What changed in this PR

Updates flag-evaluation backfill safeguards to measure Kafka consumer lag using message time rather than ClickHouse write time.

Changes:

  • Calculates per-partition lag from _timestamp.
  • Extends test fixtures and coverage for delayed backlog consumption.
File Description
posthog/​dags/​flag_evaluations_backfill.py Uses Kafka message timestamps for consumer lag.
posthog/​dags/​tests/​test_flag_evaluations_backfill.py Tests rows written later than their Kafka timestamp.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

@greptile-apps

greptile-apps Bot commented Oct 2, 2026 •

Copy link
Copy Markdown
Contributor

Retrigger

[Medium risk] Changes how consumer lag is measured in a data pipeline.

The PR appears safe to merge; no actionable issue was identified.

Reviews (1) · Last reviewed commit: "fix(flags): measure backfill consumer la..."

@coderabbitai

coderabbitai Bot commented Oct 2, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

🧰 Additional context used
📚 Code guidelines (5)
.agents/security.md — configured
.agents/skills/sending-notifications/SKILL.md — configured
docs/internal/person-data-access.md — configured
.claude/commands/conventions.md — configured
.agents/skills/writing-code-comments/SKILL.md — configured
📝 Walkthrough

Walkthrough

The partition lag query now compares the latest Kafka message timestamp with the current time, instead of using row insertion time. Tests now create Kafka rows with separate message and insertion timestamps and include a safety-check case with a two-hour-old message and a two-hour consumer delay.

Priority: ➖ Normal

Merge Risk: 🔵 Low · up to c6a1d

The lag check has a possible ordering limitation, but a duplicate-copy failure has not been established. The change is mergeable with owner awareness of that limitation.

Architecture Summary

Architecture risk: 🔵 Low · up to c6a1d

The change affects 1 system.

Changed systems: posthog

Architecture concerns
No architecture-level concerns identified.

Review details

Systems and components

  • observed — posthog (service) was modified; 2 changed files map to changed impact.

Before / after behavior

  • observed — Modified behavior in posthog/dags/tests/test_flag_evaluations_backfill.py: seed_kafka_path_row adds a consumer_delay parameter defaulting to zero. It derives kafka_time from now - age, sets timestamp one second before it, and sets inserted_at to kafka_time + consumer_delay. The insert now includes _timestamp; previously it used the same inserted_at value as the event time and did not write _timestamp.
  • observed — Modified behavior in posthog/dags/tests/test_flag_evaluations_backfill.py: The safety-check test adds consumer_delay to its parameter cases and helper call. A new case uses a two-hour-old Kafka row with a two-hour consumer delay, alongside the existing free-space and consumer-lag scenarios.
  • observed — Modified behavior in posthog/dags/flag_evaluations_backfill.py: The comments now identify _timestamp as Kafka message time, distinguish it from row-write time during backlog processing, and state that the lookback uses inserted_at because it has a skip index.
  • observed — Modified behavior in posthog/dags/flag_evaluations_backfill.py: The partition-lag calculation now uses the maximum _timestamp and now() instead of the maximum inserted_at and now64(6).
🚥 Pre-merge checks | ✅ 1
✅ Passed checks (1 passed)
Check name Status Explanation
Description check ✅ Passed The description is complete and follows the repository template. It explains the problem, changes, testing rationale, release status, documentation status, agent context, skills used, review results, …
✨ Finishing Touches
📝 Generate docstrings
  • Commit to this branch
  • Create a new PR
🧪 Generate unit tests (beta)
  • Commit to this branch
  • Create a new PR
  • Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Autopilot is currently an internal CodeRabbit preview.


Comment @coderabbitai help to get the list of available commands.

@github-actions

github-actions Bot commented Oct 2, 2026 •

Copy link
Copy Markdown
Contributor

🤖 CI report

⚠️ Trunk lane — backend Python lane

This PR is assigned to the backend Python lane. It runs backend Python tests and may merge in parallel with PRs in other lanes.

✅ Duplication (Python) — clean

New Python code duplication introduced by this branch. Fails at 70+ tokens in app code, or 150+ tokens when both copies live in test files. Advisory while the gate proves itself: extract a shared helper instead of copying.

✅ Duplication (TypeScript) — clean

New TypeScript code duplication introduced by this branch. Fails at 70+ tokens in app code, or 150+ tokens when both copies live in test files. Advisory while the gate proves itself: extract a shared helper instead of copying.

✅ Playwright — all passed

All tests passed.

View test results →

@haacked
haacked marked this pull request as ready for review October 2, 2026 20:00
@parameterai

parameterai Bot commented Oct 2, 2026 •

Copy link
Copy Markdown

Risk: No findings

The only change since the last review is one explanatory comment line above _KAFKA_POSITION_QUERY; the query text and all behavior are unchanged. No security surface is added or altered, and the lag check still derives consumer lag from the Kafka _timestamp with the 7-day inserted_at lookback as previously reviewed.

Sentinel reviewed c6a1daa · Review settings

@pr-assigner-resolver-posthog
pr-assigner-resolver-posthog Bot requested a review from a team October 2, 2026 20:01
@haacked haacked added the stamphog Request AI approval (no full review) label Oct 2, 2026
stamphog[bot]

This comment was marked as outdated.

@stamphog
stamphog Bot dismissed their stale review October 2, 2026 20:27

A new stamphog review started for this PR — the fresh verdict replaces this approval.

@trunk-io

trunk-io Bot commented Oct 2, 2026 •

Copy link
Copy Markdown

Static Badge   Static Badge   Static Badge

View Full Report ↗︎ ⋅ Docs

@stamphog stamphog Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved.

Small, contained fix to a backfill safety check in an op the author knows well (strong familiarity, owning team). The lag value is unchanged today and the new test covers the case. _timestamp is already a column the backfill copies.

  • Author wrote 100% of the modified lines and has 4 merged PRs in these paths (familiarity STRONG).
  • 👍 on the PR from greptile-apps[bot].
Gate mechanics and policy version
Gate Result
prerequisites ✓ all clear
deny-list ✓ no deny categories matched
size ✓ 5L, 1F substantive, 48L/2F incl. docs/generated/snapshots — within ceiling
tier ✓ T1-agent / T1b-small (48L, 2F, single-area, fix)
stamphog 2.3.1 .stamphog/policy.yml @ c6a1daa · reviewed head c6a1daa

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caution

Some comments are outside the diff and can’t be posted inline due to GitHub limitations.

⚠️ Outside diff range comments (1)

🟡 Minor · Use Kafka positions for the lag check. · flag_evaluations_backfill.py:69

posthog/dags/flag_evaluations_backfill.py:69
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win

Use Kafka positions for the lag check.

queueMessages sends the batch in parallel, and each producer call sets the Kafka timestamp with Date.now(). Kafka can therefore append a lower-timestamp message after a higher-timestamp message in the same partition. max(_timestamp) can then hide an older offset, allowing the backfill to insert a duplicate that the plain MergeTree retains.

Use partition offset positions, or another position-based watermark, instead of relying on timestamp ordering.


ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository: PostHog/posthog/.coderabbit.yaml

Review profile: QUIET

Plan: Enterprise

Run ID: d8b91905-356d-4d08-b69a-63f1becc80bf

📥 Commits

Reviewing files that changed from the base of the PR and between 8a9febe and c6a1daa.

📒 Files selected for processing (1)
  • posthog/dags/flag_evaluations_backfill.py

Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 9 remain after this review.

@hosthog

hosthog Bot commented Oct 2, 2026 •

Copy link
Copy Markdown

HostHog preview — posthog-desktop-web

The previews for this PR have been torn down and no longer serve.

@trunk-io
trunk-io Bot deleted the haacked/flag-evaluations-backfill-lag-kafka-time branch October 2, 2026 22:14
@deployment-status-posthog

deployment-status-posthog Bot commented Oct 2, 2026 •

Copy link
Copy Markdown

Deploy status

Environment Status Deployed At Workflow
dev ✅ Deployed 2026-10-02 22:42 UTC Run
prod-us ✅ Deployed 2026-10-02 22:57 UTC Run
prod-eu ✅ Deployed 2026-10-02 23:12 UTC Run

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

feature/feature-flags Feature Tag: Feature flags stamphog Request AI approval (no full review) team/feature-flags

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

2 participants