Skip to content

fix(alerts): shard platform logs checks by query shape to share queries - #116691

Open
jzhu13 wants to merge 4 commits into
masterfrom
posthog/alerts-platform-logs-team-shard
Open

jzhu13 wants to merge 4 commits into
masterfrom
posthog/alerts-platform-logs-team-shard

Conversation

@jzhu13

@jzhu13 jzhu13 commented Oct 14, 2026 •

Copy link
Copy Markdown
Contributor

Problem

  • The alerts platform runs more ClickHouse queries than production logs alerting for the same logs alerts.
  • #109850 moved production logs to one shard slot per team, so a team's alerts on one cadence share cohort queries.
  • The platform still spreads each configuration over its own slot. Alerts that could share one query land in different minutes, and each minute runs its own checkpoint and cohort queries.
  • This matters more as the parallel run widens, and it becomes the production cost once the platform owns logs alerts.

Changes

  • The platform schedules logs alerts that can share a cohort query into the same slot. Nothing a user sees changes.
  • A source can now set schedule_group on PlatformAlertOutcome. The platform puts a team's checks with one group in one slot, and spreads checks without a group by configuration, as before.
  • Logs sets the group to the query shape: window, evaluation periods, and projection eligibility. _cohort_key reads the same helper, so the group and the cohort cannot drift apart.
  • Different shapes spread across slots instead of sharing the team's slot. One slot per team would put a team with many shapes over MAX_COHORTS_PER_CYCLE (4), and its extra cohorts would run a tick late on every cycle.
  • Insight sets no group and keeps the per-configuration spread. It runs one query per alert, so a shared slot would only bunch a team's checks into one minute.
  • compute_team_shard_offset_seconds moves into the platform facade with the existing uuid5 namespace. Production logs calls it without a group, so its slots do not change.
  • Existing platform logs alerts move to their group's slot on their next check. During that one move, a check can wait up to about two intervals, the same transition fix(logs): shard alert checks by team so a team's alerts share a query #109850 accepted.

Note

The platform no longer checks a team in the same minute as production logs. The comparison harness already classifies that phase skew separately from real disagreements.

How did you test this code?

Test rationale:

  • test_platform_lifecycle.py: two configurations whose per-configuration offsets differ land in one slot with a shared group and in two without one. It fails if record_outcomes ignores schedule_group. No existing test checked where record_outcomes puts a team's alerts.
  • test_alert_evaluate.py: two same-shape logs alerts carry one group, while a longer window and more evaluation periods each carry their own. It fails if logs stops setting the group, which would silently fall back to the per-configuration spread.
  • Both tests fail with their side of the change removed.
  • Ran locally: test_platform_lifecycle.py, test_alert_evaluate.py, test_scheduling.py, test_grid_scheduling.py, test_shard_offset.py.
  • Not run: the full logs and alerts suites, or repo-wide mypy. CI covers them.

Release status

  • No feature flag controls this change

Docs update

Updated docs/internal/alerts-platform-temporal.md.

🤖 Agent context

Autonomy: Human-driven (agent-assisted)

Agent: Claude Code, Claude Opus 5.5 (claude-opus-5-5)

  • Came out of read-only monitoring of the logs parallel run: the platform issued noticeably more logs queries per team than production, while each query cost about the same.
  • #114799 bounds concurrent platform logs batches. It does not change how many batches a team splits into, so the two PRs are independent.
  • The first version sharded every logs alert by team alone. PostHog Review pointed out that this pushes a team with more than four query shapes over the per-batch cohort cap. Sharding by team and shape keeps the query sharing and keeps batches small. Raising the cap was rejected: under a slow ClickHouse, later cohorts get shorter time limits and their alerts record errors instead of waiting.
  • Skills invoked: /writing-tests, /writing-code-comments, /simplify, /code-review high --fix, /writing-pr-descriptions, /reviewing-with-coderabbit, /merging-prs.
  • /code-review also raised the one-cycle transition delay. It is accepted, as in fix(logs): shard alert checks by team so a team's alerts share a query #109850.
  • CodeRabbit CLI (--deep) did not run: "You are not a member of the requested organization". The bot reviews the PR instead.

🤖 Generated with Claude Code

jzhu13 and others added 3 commits October 13, 2026 17:31
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@jzhu13 jzhu13 self-assigned this Oct 14, 2026
@trunk-io

trunk-io Bot commented Oct 14, 2026

Copy link
Copy Markdown

Merging to master in this repository is managed by Trunk.

  • To merge this pull request, check the box to the left or comment /trunk merge below.

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

github-actions Bot commented Oct 14, 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.

🚨 Comment density — 8% of added code lines are comments (8 of 99)

This section warns when comments are more than 3% of the code lines a PR adds, and alerts above 6%. Before agent-assisted PRs, the typical share was about 2%. Only full-line comments count. Docstrings, generated files, snapshots, migrations, and workflow files are left out.

Comments that restate the code, record how the change came about, or narrate the next line add noise for the next reader. Keep the comments that explain a reason the code cannot show, and remove the rest. See .agents/skills/writing-code-comments/SKILL.md for the house rules.

Files with the most added comment lines:

File Comment lines Added lines
products/alerts_platform/backend/facade/contracts.py 2 3
products/alerts_platform/backend/facade/scheduling.py 2 22
products/logs/backend/alert_source_cycle.py 2 16
products/alerts_platform/backend/tests/test_platform_lifecycle.py 1 37
products/logs/backend/alert_utils.py 1 4

This check does not block merging. It updates on every push and clears when the share drops.

@posthog posthog 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.

PostHog Review · Standard · reviewed 3672b21: 1 Should fix.

Comment thread products/alerts_platform/backend/logic/platform_lifecycle.py Outdated
@jzhu13
jzhu13 marked this pull request as ready for review October 14, 2026 00:39
@jzhu13 jzhu13 added the stamphog Request AI approval (no full review) label Oct 14, 2026
@parameterai

parameterai Bot commented Oct 14, 2026 •

Copy link
Copy Markdown

Risk: No findings

This increment replaces the TEAM_BATCHED_SOURCES set with a schedule_group field on PlatformAlertOutcome: logs computes the group from its query shape (window_minutes, evaluation_periods, projection eligibility), and the platform shards a team's checks by team+group instead of team alone. I traced every surface this touches and found no new security issues.

Sentinel reviewed f396747 · Review settings

@pr-assigner-resolver-posthog
pr-assigner-resolver-posthog Bot requested a review from a team October 14, 2026 00:40

@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.

Not approved — this change needs a human reviewer.

Re-add the stamphog label to request another review once you have addressed this.

The current-head review has an unresolved scheduling-correctness concern confirmed by the code. Address @PostHog's comment in platform_lifecycle.py by ensuring all due cohorts are handled within the scheduled tick, then request another review from @PostHog/team-apm before re-requesting.

  • Author wrote 19% of the modified lines and has 15 merged PRs in these paths (familiarity MODERATE).
  • PostHog Review (Standard) reviewed the current head: 1 finding, posted on the PR.
  • posthog[bot] reviewed the current head.
  • @PostHog on products/alerts_platform/backend/logic/platform_lifecycle.py: The cohort-cap concern remains unaddressed. Logs configurations now share a team scheduling slot, but products/logs/backend/alert_source_cycle.py:637–655 still groups checks by cohort and evaluates only the first four cohorts. Evaluation periods are part of the cohort key, so five alerts with distinct periods can leave one unchecked until a later tick even without exhausting the query budget.
Gate mechanics and policy version
Gate Result
prerequisites ✓ all clear
deny-list ✓ no deny categories matched
size ✓ 60L, 4F substantive, 107L/6F incl. docs/generated/snapshots — within ceiling
tier ✓ T1-agent / T1c-medium (107L, 6F, two-areas, fix)
stamphog 2.5.0 .stamphog/policy.yml @ 3672b21 · reviewed head 3672b21

@stamphog stamphog Bot removed the stamphog Request AI approval (no full review) label Oct 14, 2026
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@jzhu13 jzhu13 changed the title fix(alerts): shard platform logs checks by team so they share a query fix(alerts): shard platform logs checks by query shape to share queries Oct 14, 2026
@jzhu13 jzhu13 added the stamphog Request AI approval (no full review) label Oct 14, 2026

@posthog posthog 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.

PostHog Review · Standard · reviewed f396747: 1 Must fix.

query_duration_ms=query_duration_ms,
muted_notification=muted_notification,
disable=disable,
schedule_group=_schedule_group(check),

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.

P1 · Avoid deriving query shapes for broken configurations

When a configuration has filterGroup.values = null, _triage detects the invalid filter and calls _held to record BROKEN. The new _schedule_group call invokes is_projection_eligible, which iterates that null value and raises TypeError. This aborts the entire batch, leaving valid checks and the broken configuration due on every retry; skip query-shape grouping for broken configurations.

@stamphog stamphog Bot removed the stamphog Request AI approval (no full review) label Oct 14, 2026

@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.

Not approved — this change needs a human reviewer.

Re-add the stamphog label to request another review once you have addressed this.

@PostHog's substantive concern remains unfixed and can abort a logs alert batch. Skip query-shape grouping for broken configurations, add a regression test covering null filter values alongside valid checks, and address that thread before re-requesting.

  • Author wrote 32% of the modified lines and has 15 merged PRs in these paths (familiarity MODERATE).
  • PostHog Review (Standard) reviewed the current head: 1 finding, posted on the PR.
  • posthog[bot] reviewed the current head.
  • @PostHog's unresolved concern in products/logs/backend/alert_source_cycle.py is confirmed: _held calls _recorded for invalid configurations, but _recorded unconditionally computes a schedule group. That calls is_projection_eligible in products/logs/backend/alert_check_query.py, which iterates filterGroup.values without handling null, raising TypeError and aborting batch triage instead of recording the broken configuration and processing valid checks.
Gate mechanics and policy version
Gate Result
prerequisites ✓ all clear
deny-list ✓ no deny categories matched
size ✓ 82L, 5F substantive, 138L/8F incl. docs/generated/snapshots — within ceiling
tier ✓ T1-agent / T1c-medium (138L, 8F, two-areas, fix)
stamphog 2.5.0 .stamphog/policy.yml @ f396747 · reviewed head f396747

@trunk-io

trunk-io Bot commented Oct 14, 2026 •

Copy link
Copy Markdown

Static Badge   Static Badge   Static Badge

View Full Report ↗︎ ⋅ Docs

This branch has not been deployed

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant