-
Notifications
You must be signed in to change notification settings - Fork 3.5k
fix(signals): stop pganalyze re-emitting open issues every sync #108417
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from all commits
b3763d4
c9bf7d9
c2ecc5a
9b4700d
e7d9687
c685668
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -5,6 +5,8 @@ | |
| from datetime import datetime | ||
| from typing import Any | ||
|
|
||
| from django.utils import timezone | ||
|
|
||
| import structlog | ||
| import posthoganalytics | ||
| from anthropic import AsyncAnthropic | ||
|
|
@@ -25,6 +27,7 @@ | |
| ) | ||
| from products.signals.backend.emission.steering import apply_steering, steering_from_config | ||
| from products.signals.backend.facade.api import emit_signal | ||
| from products.signals.backend.models import SignalEmissionRecord | ||
| from products.signals.backend.temporal import metrics | ||
| from products.signals.backend.temporal.drop_telemetry import summarize_drop_error | ||
| from products.signals.backend.temporal.llm import effort_kwargs | ||
|
|
@@ -465,11 +468,28 @@ | |
| ) | ||
|
|
||
|
|
||
| async def _record_processed_outputs(team: Team, outputs: list[SignalEmitterOutput]) -> None: | ||
| await SignalEmissionRecord.objects.abulk_create( | ||
| [ | ||
| SignalEmissionRecord( | ||
| team=team, | ||
| source_product=output.source_product, | ||
| source_type=output.source_type, | ||
| source_id=output.source_id, | ||
| emitted_at=timezone.now(), | ||
| ) | ||
| for output in outputs | ||
| ], | ||
| ignore_conflicts=True, | ||
| ) | ||
|
|
||
|
|
||
| async def _emit_signals( | ||
| team: Team, | ||
| organization: Organization, | ||
| outputs: list[SignalEmitterOutput], | ||
| extra: dict[str, Any], | ||
| record_processed_outputs: bool = False, | ||
| ) -> int: | ||
| semaphore = asyncio.Semaphore(EMIT_CONCURRENCY_LIMIT) | ||
| _safe_heartbeat() | ||
|
|
@@ -506,10 +526,13 @@ | |
| description=output.description, | ||
| weight=output.weight, | ||
| extra=output.extra, | ||
| idempotency_key=output.source_id if record_processed_outputs else None, | ||
| ) | ||
| if record_processed_outputs: | ||
| await _record_processed_outputs(team, [output]) | ||
|
Comment on lines
+531
to
+532
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win 🔎 Supported by static analysis🏁 Script executed: #!/bin/bash
# Inspect the pipeline entrypoint and the facade's pre-dispatch return paths.
rg -n -C 12 'def run_signal_pipeline|_emit_signals\(|is_ai_data_processing_approved|is_source_enabled' \
products/signals/backend/emission/pipeline.py \
products/signals/backend/facade/api.pyRepository: PostHog/posthog Length of output: 13500 🏁 Script executed: sed -n '487,653p' products/signals/backend/emission/pipeline.pyRepository: PostHog/posthog Length of output: 7888 🏁 Script executed: rg -n -C 8 'SignalEmissionRecord|pganalyze|processed_outputs' products/signalsRepository: PostHog/posthog Length of output: 42502 Do not record outputs that When AI data processing is unapproved or the source is disabled, |
||
| return True | ||
| except Exception as e: | ||
| # Fetchers record emission optimistically, so a record lost here is lost for good. | ||
| # Sources that record at fetch time cannot retry a record lost here. | ||
| # Close the funnel (entered - summarized - filtered - emit_failed = emitted) and | ||
| # count the drop, or the loss is invisible outside logs. | ||
| error_type, _ = summarize_drop_error(e) | ||
|
|
@@ -539,7 +562,7 @@ | |
| return succeeded | ||
|
|
||
|
|
||
| async def run_signal_pipeline( | ||
|
Check warning on line 565 in products/signals/backend/emission/pipeline.py
|
||
| team: Team, | ||
| config: SignalSourceTableConfig, | ||
| records: list[dict[str, Any]], | ||
|
|
@@ -606,6 +629,10 @@ | |
| context_fields=config.actionability_context_fields, | ||
| ) | ||
| post_filter_ids = {o.source_id for o in outputs} | ||
| if config.record_processed_outputs: | ||
| await _record_processed_outputs( | ||
| team, [output for source_id, output in pre_filter_by_id.items() if source_id not in post_filter_ids] | ||
| ) | ||
| for source_id, output in pre_filter_by_id.items(): | ||
| if source_id not in post_filter_ids: | ||
| capture_pipeline_stage( | ||
|
|
@@ -621,6 +648,7 @@ | |
| organization=organization, | ||
| outputs=outputs, | ||
| extra=extra, | ||
| record_processed_outputs=config.record_processed_outputs, | ||
| ) | ||
| logger.info(f"Emitted {signals_emitted} signals for {source_label}", **extra) | ||
| return {"status": "success", "signals_emitted": signals_emitted} | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Prevent persistent failures from blocking later issue IDs.
If the first
config.max_recordsIDs fail emission on every sync, none entersSignalEmissionRecord. The next sync starts at the same IDs and returns them again, so later valid issues never reach the pipeline. Preserve retries for failed IDs, but rotate or separately queue retries so a permanently failing batch cannot block the backlog.