feat(workflows): richer observability for push notifications - #70913
Merged
Conversation
Push logging and metrics were much thinner than email. On failure the workflow
log just dumped the raw FCM/APNs JSON error at the user, send metrics were
mislabeled as `metric_kind: 'other'` instead of the existing `'push'` kind, and
the only ops signal was a per-platform "sent" counter with no view into why sends
failed.
- Add a provider-agnostic failure-reason normalizer (push-notification-errors.ts)
that maps FCM error codes / APNs reasons / HTTP status to a small bounded set
(unregistered, invalid_token, auth_error, rate_limited, invalid_payload,
provider_error, network_error, unknown) with a plain-language, action-guiding
message and a severity (config problems the user must fix are errors; transient
or expected ones are warnings).
- Rewrite the send logs to use it: a readable one-line explanation at the right
level instead of a raw payload dump; the raw provider response is kept at debug
level for support. Success reads "accepted by FCM/APNs" (honest — we only know
the provider took it, not that the device showed it).
- Label ops metrics by reason: new push_notification_failed_total{platform,reason}
and push_notification_skipped_total{platform}, plus a
push_notification_send_duration_ms{platform} histogram. Existing sent/pruned
counters unchanged.
- Emit push business metrics under the correct metric_kind: 'push'.
Delivery/open/click have no equivalent here: FCM/APNs respond synchronously and
there is no delivery-receipt webhook like SES, so this focuses on what we can
actually observe at send time. Surfacing a push summary in the workflow Metrics
tab (email has one, push has none) is the intended follow-up.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Bbav6vE7Tm24KkdRhZmvNM
dmarchuk
force-pushed
the
claude/push-observability
branch
from
July 15, 2026 10:15
5b29bb7 to
438a5b9
Compare
dmarchuk
marked this pull request as ready for review
July 15, 2026 11:11
Contributor
|
Reviews (1): Last reviewed commit: "feat(messaging): richer observability fo..." | Re-trigger Greptile |
Addresses two review findings on the push observability change: - FCM: a bare HTTP 404 was classified as `unregistered` and pruned the device token. A generic 404 from a proxy or misconfigured endpoint would then permanently drop a still-valid token. Require FCM's own signal (the `UNREGISTERED` errorCode or the `NOT_FOUND` canonical status) before pruning; an unexplained 404 now surfaces as a failure instead. - The unregistered/pruned outcome incremented only the token-pruned counter, so it was absent from the new reason-labeled skip metric. Give push_notification_skipped_total a `reason` label (`no_token` vs `unregistered`) and record the prune there too, so the skip metric accounts for every non-delivery. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Bbav6vE7Tm24KkdRhZmvNM
mayteio
approved these changes
Jul 15, 2026
mayteio
left a comment
Contributor
There was a problem hiding this comment.
Nothing major, solid PR 😎
This was referenced Jul 15, 2026
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
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
Push logging and metrics are much thinner than email, so when a customer's push notifications don't land they can't easily tell what happened. Three concrete gaps:
FCM send error. Status: 400. Body: {...}). Fine for us, useless for a customer.metric_kind: 'other'even though a dedicated'push'kind already exists (email correctly uses'email').Push has real limits vs email: FCM/APNs respond synchronously and there is no delivery-receipt webhook like SES, so there's no honest
delivered/opened/clicked. This focuses on what we can actually observe at send time.Changes
A failure-reason normalizer (
push-notification-errors.ts) maps FCM error codes / APNs reasons / HTTP status into a small bounded set —unregistered,invalid_token,auth_error,rate_limited,invalid_payload,provider_error,network_error,unknown— each with:auth_errortells the user which credential to check), andLogging now uses it. Instead of a raw payload dump, the workflow log shows a readable one-liner at the right level. The raw provider response is kept at
debuglevel for support. Success reads "accepted by FCM/APNs" rather than "sent" — honest, since we only know the provider accepted it, not that the device displayed it.Metrics. Business metrics now use the correct
metric_kind: 'push'. Ops metrics gain reason/skip/latency dimensions:push_notification_failed_totalplatform, reasonpush_notification_skipped_totalplatformpush_notification_send_duration_msplatformExisting
push_notification_sent_totalandpush_notification_token_pruned_totalare unchanged.The channel send throws a typed
PushSendErrorcarrying the reason/level/platform, so the top-level loop logs once at the right severity and labels the failure metric without re-deriving anything or double-logging.Note
Surfacing a push summary in the workflow Metrics tab (email has
EmailMetricsSummary; push has no UI at all) is the intended immediate follow-up — this PR is the backend foundation it needs.How did you test this code?
push-notification-errors.test.ts: parameterized over representative FCM and APNs error shapes, asserting the reason classification, theunregisteredflag that drives token pruning, the raw code passthrough, and the severity mapping. Guards against a mapping regression silently breaking pruning or mislabeling metrics.push-notification.service.test.tsto assert themetric_kind: 'push'fix.Independent verification against this branch (master merged in for a current tree):
tsc --noEmitandprettier --checkclean on the changed files.Push notification accepted by FCM.(the wording changes from "sent via" to "accepted by", since we only know the provider accepted the message, not that the device displayed it).The richer provider-error messages (auth rejected, rate-limited, invalid payload) can't be forced against a live provider on demand, so those stay covered by the normalizer unit tests. CI covers the kafka/clickhouse-backed integration suites.
Automatic notifications
Docs update
No user-facing docs change — this improves the observability surface (workflow logs + internal metrics) with no API or config change.
🤖 Agent context
Autonomy: Human-driven (agent-assisted). Directed by the requester (session owner); left unassigned only because I couldn't verify their GitHub handle this session — a maintainer should set the DRI.
Authored with Claude Code. Two parallel research agents first mapped email's observability end-to-end (metrics/logging/SES webhook) as the target and the CDP metrics/logging infrastructure + current push state, which surfaced the constraints that shaped scope: app-metrics carry no label dimensions (fixed 6-field schema), so platform/reason granularity lives in Prometheus and logs rather than business metrics; and push has no delivery-receipt channel, so open/click parity with email isn't possible. Invoked the
/writing-testsskill before writing the normalizer tests to keep them aimed at real regressions. Chose to keep this PR backend-only (fully testable here) and tee up the Metrics-tab UI as a separate follow-up rather than ship unverified frontend wiring.🤖 Generated with Claude Code
Generated by Claude Code