feat(logs): emit signals on alert state changes#60637
Merged
jonmcwest merged 2 commits intoJun 1, 2026
Merged
Conversation
Contributor
|
Size Change: +2.19 kB (0%) Total Size: 80.9 MB 📦 View Changed
ℹ️ View Unchanged
|
Contributor
Prompt To Fix All With AIFix the following 2 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 2
products/logs/backend/alert_signal_emitter.py:54-55
The `alert_id` parameter is accepted but never used — the URL produced is always the generic logs page regardless of which alert fired. This is a superfluous parameter per simplicity rule #4. Either remove it (and update the call site in `build_signal_extra`) or incorporate it into the URL if a per-alert deep-link is available.
```suggestion
def _alert_url(team_id: int) -> str:
return f"{settings.SITE_URL}/project/{team_id}/logs"
```
### Issue 2 of 2
products/logs/backend/test/test_logs_alerting_activities.py:124-147
**Prefer parameterised tests over bundled cases**
`test_build_notified_from_saved_includes_only_notified` collapses four distinct filter predicates (FIRE/not-failed → included; NONE/not-failed → excluded; RESOLVE/not-failed → excluded; FIRE/failed → excluded) into a single list with a single count assertion. If a regression silently changes which predicate is broken, the test output won't identify which case failed. Splitting these into individual `@parameterized.expand` cases — one for the included case and one each for each exclusion reason — gives a failing test name that immediately names the broken contract.
Reviews (1): Last reviewed commit: "chore: update OpenAPI generated types" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+124
to
+147
| window_minutes=5, | ||
| filters={"serviceNames": ["checkout"]}, | ||
| ) | ||
|
|
||
| saved = [ | ||
| self._dispatched(alert, NotificationAction.FIRE, False), # included | ||
| self._dispatched(alert, NotificationAction.NONE, False), # excluded: no notification | ||
| self._dispatched(alert, NotificationAction.RESOLVE, False), # excluded: not signalable | ||
| self._dispatched(alert, NotificationAction.FIRE, True), # excluded: notification failed | ||
| ] | ||
| notified = _build_notified_from_saved(saved) | ||
|
|
||
| assert len(notified) == 1 | ||
| assert notified[0].action == "firing" | ||
| assert notified[0].alert_id == str(alert.id) | ||
| assert notified[0].team_id == self.team.id | ||
| assert notified[0].result_count == 250 | ||
| assert notified[0].filters == {"serviceNames": ["checkout"]} | ||
|
|
||
|
|
||
| class TestEmitAlertSignalsActivity(NonAtomicBaseTest): | ||
| def _notified( | ||
| self, alert_id: str, action: str, result_count: int | None, consecutive_failures: int | ||
| ) -> NotifiedAlert: |
Contributor
There was a problem hiding this comment.
test_build_notified_from_saved_includes_only_notified collapses four distinct filter predicates (FIRE/not-failed → included; NONE/not-failed → excluded; RESOLVE/not-failed → excluded; FIRE/failed → excluded) into a single list with a single count assertion. If a regression silently changes which predicate is broken, the test output won't identify which case failed. Splitting these into individual @parameterized.expand cases — one for the included case and one each for each exclusion reason — gives a failing test name that immediately names the broken contract.
Prompt To Fix With AI
This is a comment left during a code review.
Path: products/logs/backend/test/test_logs_alerting_activities.py
Line: 124-147
Comment:
**Prefer parameterised tests over bundled cases**
`test_build_notified_from_saved_includes_only_notified` collapses four distinct filter predicates (FIRE/not-failed → included; NONE/not-failed → excluded; RESOLVE/not-failed → excluded; FIRE/failed → excluded) into a single list with a single count assertion. If a regression silently changes which predicate is broken, the test output won't identify which case failed. Splitting these into individual `@parameterized.expand` cases — one for the included case and one each for each exclusion reason — gives a failing test name that immediately names the broken contract.
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
jonmcwest
force-pushed
the
05-29-feat_logs_emit_signals_on_alert_state_changes
branch
from
7a8a4c1 to
f4d0144
Compare
Contributor
Migration SQL ChangesHey 👋, we've detected some migrations on this PR. Here's the SQL output for each migration, make sure they make sense:
|
Contributor
🔍 Migration Risk AnalysisWe've analyzed your migrations for potential risks. Summary: 0 Safe | 1 Needs Review | 0 Blocked
|
DanielVisca
approved these changes
May 29, 2026
DanielVisca
approved these changes
May 29, 2026
jonmcwest
force-pushed
the
05-29-feat_logs_emit_signals_on_alert_state_changes
branch
from
f4d0144 to
b0ac5c6
Compare
Contributor
Merge activity
|
![veria-ai[bot]](https://avatars.githubusercontent.com/in/1723776?s=60&v=4){kind=small}
| "window_minutes": na.window_minutes, | ||
| "result_count": na.result_count, | ||
| "consecutive_failures": na.consecutive_failures, | ||
| "filters": na.filters, |
There was a problem hiding this comment.
Medium: Prompt injection through signal extras
filters can contain arbitrary filterGroup values from a logs alert, and signal extras are later rendered into the LLM prompt via _render_extra_to_text. The buffer safety filter only receives s.description, so an authenticated user who can create a logs alert can put instructions in a filter value and have them reach the downstream signals agent unscreened; either omit arbitrary filter values from extra or run the safety filter over the full rendered signal including extras.
PR overviewThis PR adds log alert signal emission when alert state changes, including alert metadata and filter details in the signal payload for downstream processing. There is one open security concern around untrusted log alert filter values being included in signal extras that are later rendered into an LLM prompt. An authenticated user who can create a logs alert could influence downstream agent instructions through those fields unless the extras are omitted or screened as part of the full rendered signal. No issues have been fixed or addressed yet, so the PR still carries a moderate prompt-injection risk. Open issues (1)
Fixed/addressed: 0 · PR risk: 6/10 |
jonmcwest
force-pushed
the
05-29-feat_logs_emit_signals_on_alert_state_changes
branch
from
b0ac5c6 to
bca0318
Compare
Generated-By: PostHog Code Task-Id: 345c67dc-7f02-4e33-bdbd-024582d15de2
jonmcwest
force-pushed
the
05-29-feat_logs_emit_signals_on_alert_state_changes
branch
from
bca0318 to
402f8eb
Compare
Contributor
Query snapshots: Backend query snapshots updatedChanges: 1 snapshots (0 modified, 1 added, 0 deleted) What this means:
Next steps:
|
Contributor
|
🎭 Playwright report · View test results →
These issues are not necessarily caused by your changes. |
2 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Logs alerting can fire and auto-disable (break) alerts, but those state changes were invisible to the signals pipeline. Users and AI agents had no way to correlate a logs alert firing with other signals in the system.
Changes
Introduces a new
logs/alert_state_changesignal type that is emitted whenever a logs alert transitions to firing or broken (auto-disabled). The signal carries enough context for downstream consumers — alert name, threshold, observed count, window, filters, and a direct URL to the logs view.Signal schema
LogsAlertStateChangeSignalExtraandLogsAlertStateChangeSignalInputto the TypeScript schema, the generatedschema.json, andposthog/schema.py.logsas aSignalSourceProductandalert_state_changeas aSignalSourceTypein both the frontend enums and the Django model choices.SignalSourceConfigfield choices.Emitter (
alert_signal_emitter.py)NotifiedAlertis a frozen dataclass that carries everything needed to build a signal and crosses the Temporal activity boundary safely (all primitives).build_signal_extra/build_signal_descriptionproduce the payload and human-readable description respectively.emit_alert_state_change_signalwrapsemit_signal, swallowing exceptions so a signal failure never propagates to the alert cycle.FIREandBROKENnotifications are signalled;RESOLVEandERRORare intentionally excluded.Temporal integration
evaluate_cohort_batch_activitynow collectsNotifiedAlertinstances from each cohort and returns them inEvaluateCohortBatchOutput.emit_alert_signals_activityfans out signal emission with bounded concurrency (EMIT_SIGNAL_CONCURRENCY, default 20) and periodic heartbeats.notifiedacross all batch results and dispatchesemit_alert_signals_activityin chunks (EMIT_SIGNAL_BATCH_SIZE, default 500) after evaluation completes. Emission failures are swallowed — the alert cycle always completes.How did you test this code?
Automated tests cover:
test_alert_signal_emitter.py—signal_action_and_weightmapping,build_signal_extraschema validation againstLogsAlertStateChangeSignalExtra, description content, andemit_alert_state_change_signalsuccess/failure isolation.test_logs_alerting_activities.py—_build_notified_from_savedfiltering logic (only FIRE/BROKEN with successful notification dispatch are included), andemit_alert_signals_activityend-to-end with real team resolution and empty-input no-op.test_logs_alerting_workflow.py— workflow forwards notified alerts to the emission activity, and skips the emission activity entirely when no alerts were notified in a cycle.