Skip to content

refactor(signals): route emit_signal through backend facade#60848

Merged
webjunkie merged 1 commit into
masterfrom
refactor/signals-emit-signal-facade
Jun 1, 2026
Merged

refactor(signals): route emit_signal through backend facade#60848
webjunkie merged 1 commit into
masterfrom
refactor/signals-emit-signal-facade

Conversation

@webjunkie
Copy link
Copy Markdown
Contributor

@webjunkie webjunkie commented Jun 1, 2026

Problem

Part of a broader effort to cut django.setup() import time (which took it from ~11.8s to ~5s overall). emit_signal lived in products/signals/backend/api.py, which imported the signals temporal workflows at module scope. That formed a latent circular import — api -> temporal package -> reingestion -> api.emit_signal — that only held together by import-order luck and dragged the whole signals temporal workflow stack onto the import path of anything that touched emit_signal (which is a lot: other products, core temporal workers, the scout harness). Any shift in import order would both expose the cycle as a hard ImportError and re-pay that startup cost.

It's also a cross-boundary API that should sit behind a facade per the product-isolation guidelines, so the cleanup and the isolation step are the same move.

Changes

Move emit_signal to products/signals/backend/facade/api.py and route all callers through it. The temporal workflow imports it needs are deferred into the function body, so the facade imports nothing temporal at module scope — the cycle is gone, importing the facade is cheap, and the signals workflow stack no longer rides onto startup via this path. Pure rename plus deferred imports; no behaviour change.

How did you test this code?

I'm an agent (Claude Code). Automated only: products/signals/backend/test/test_api.py (9 passed), ruff/ty clean via lint-staged, and I verified the facade, signals.views, and the temporal package all import without the old circular-import error. No manual testing; relying on CI for the full suite.

Docs update

No user-facing change.

🤖 Agent context

Surfaced while profiling Django startup import cost (python -X importtime plus an __import__-hook tracer to find what drags heavy modules onto the startup path). This latent cycle was masked by import order and would bite the moment the order shifted — deferring the temporal imports behind the facade removes the cycle and completes a planned isolation step in one go. Kept separate from the larger startup-perf PR (the lazy API router and friends) because it's a self-contained refactor the signals owners can review on its own. Agent-assisted, human-reviewed — not self-merged.

Move emit_signal from products/signals/backend/api.py to
products/signals/backend/facade/api.py so cross-boundary callers
(endpoints product, core temporal workers, scout harness) go through a
real facade per product-isolation guidelines.

The temporal workflow imports emit_signal needs are deferred into the
function body. At module scope they formed a latent circular import
(api -> temporal package -> reingestion -> api) that only resolved by
import-order luck and dragged the temporal workflow stack onto the
Django startup path. The facade now imports nothing temporal at module
scope, so importing it from the API/URL hot path is cheap and safe.

Pure rename + deferred imports; no behavior change.
@webjunkie webjunkie marked this pull request as ready for review June 1, 2026 10:15
Copilot AI review requested due to automatic review settings June 1, 2026 10:15
@assign-reviewers-posthog assign-reviewers-posthog Bot requested review from a team June 1, 2026 10:15
@assign-reviewers-posthog
Copy link
Copy Markdown

👥 Auto-assigned reviewers

Skipped a review request for @PostHog/team-data-modeling (products/endpoints/**), @PostHog/team-warehouse-sources (posthog/temporal/data_imports/**) because they only have minor changes here. These are soft owners (CODEOWNERS-soft / each product's product.yaml), so nothing blocks merge — self-assign if you'd like a look. (Generated files and lockfiles are ignored when deciding ownership.)

@webjunkie webjunkie requested a review from a team June 1, 2026 10:16
@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented Jun 1, 2026

Reviews (1): Last reviewed commit: "refactor(signals): route emit_signal thr..." | Re-trigger Greptile

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Refactors Signals emission so emit_signal is accessed via the Signals backend facade and no longer pulls Signals Temporal workflow modules into the import graph at module import time, eliminating a latent circular import and reducing Django startup/import cost.

Changes:

  • Updated all call sites and tests to import/patch emit_signal from products.signals.backend.facade.api.
  • Deferred Signals Temporal workflow imports inside emit_signal (instead of importing workflows at module scope).
  • Updated Temporal activities/workflows and misc utilities (mgmt command, scout harness, endpoints) to use the facade entrypoint.

Reviewed changes

Copilot reviewed 16 out of 17 changed files in this pull request and generated no comments.

Show a summary per file
File Description
products/signals/backend/views.py Switches emit_signal import to the facade entrypoint.
products/signals/backend/test/test_scout_harness_tools.py Updates patch target to facade path for harness tool tests.
products/signals/backend/test/test_scout_harness_api.py Updates patch target to facade path for API tests.
products/signals/backend/test/test_api.py Updates imports/patch targets to facade path for emit_signal unit tests.
products/signals/backend/temporal/reingestion.py Routes re-ingestion emissions through facade emit_signal.
products/signals/backend/temporal/emit_eval_signal.py Updates local import to facade emit_signal.
products/signals/backend/temporal/backfill_error_tracking.py Updates local import to facade emit_signal.
products/signals/backend/scout_harness/tools/emit.py Uses facade emit_signal for harness emission (kept deferred import).
products/signals/backend/management/commands/ingest_signals_json.py Updates management command import to facade emit_signal.
products/signals/backend/facade/api.py Removes module-scope workflow imports; adds lazy imports inside emit_signal.
products/signals/backend/facade/init.py Ensures facade is a package for stable import path.
products/endpoints/backend/tests/test_endpoint_execution.py Updates patch target to facade path for endpoint failure-signal test.
products/endpoints/backend/api.py Uses facade emit_signal for endpoint failure signal emission.
posthog/temporal/session_replay/session_summary/activities/video_based/a7b_emit_session_problem_signals.py Updates import to facade emit_signal.
posthog/temporal/data_imports/signals/pipeline.py Updates import to facade emit_signal.
posthog/temporal/ai_observability/eval_reports/test/test_emit_signal.py Updates patch/import of _SIGNAL_VARIANT_LOOKUP and emit_signal to facade path.
posthog/temporal/ai_observability/eval_reports/emit_signal.py Updates local import to facade emit_signal.
Comments suppressed due to low confidence (1)

products/signals/backend/facade/api.py:92

  • The deferred Temporal workflow imports still run before the early-return gates (AI data processing approval + source enabled). This means callers that are immediately skipped will still pay the cost (and side effects) of importing the full signals Temporal stack. Move these imports to after the gate checks so Temporal modules are only imported when we actually intend to start workflows.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@webjunkie webjunkie merged commit 4f2a1be into master Jun 1, 2026
279 of 281 checks passed
@webjunkie webjunkie deleted the refactor/signals-emit-signal-facade branch June 1, 2026 15:12
@deployment-status-posthog
Copy link
Copy Markdown

deployment-status-posthog Bot commented Jun 1, 2026

Deploy status

Environment Status Deployed At Workflow
dev ✅ Deployed 2026-06-01 15:42 UTC Run
prod-us ✅ Deployed 2026-06-01 15:57 UTC Run
prod-eu ✅ Deployed 2026-06-01 16:00 UTC Run

lricoy added a commit that referenced this pull request Jun 1, 2026
PR #60848 moved products/signals/backend/api -> facade/api but the
alert_signal_emitter import wasn't updated, breaking Django test
collection on master. Carrying the fix here so this PR can pass CI.
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.

3 participants