feat: expose notification delivery status timeline - #889
Open
larryjay007 wants to merge 5 commits into
Open
Conversation
Adds a NotificationDeliveryAttempt table, recording every individual delivery attempt (not just the latest, unlike NotificationOutbox's lastAttemptAt/lastError which get overwritten on each retry). - Prisma: DeliveryAttemptOutcome enum + NotificationDeliveryAttempt model, relation to NotificationOutbox. Migration hand-verified against a real throwaway SQLite DB built from the actual migration history (via prisma migrate dev), then isolated to just our table — the auto-diff also bundled in three unrelated, already-drifted tables (ArtifactAccessToken, ImportJob, SorobanEventCorrelation) that exist in schema.prisma but have no migration; excluded those, not ours to fix. - notification-failure-classifier.ts: buckets raw errors into a small fixed category set (timeout/rate_limited/invalid_recipient/ provider_error/unknown), specifically to avoid the cardinality-explosion pattern already present in incrementCallbackFailure/ incrementTxSubmissionFailure (raw error text as a Prometheus label). - notifications.processor.ts: onCompleted/onFailed now also insert a NotificationDeliveryAttempt row and record the new metrics, alongside the existing outbox status update. - metrics.providers.ts/metrics.service.ts: two new counters, notification_delivery_attempts_total (type, outcome) and notification_delivery_failures_by_category_total (type, failure_category) — deliberately separate from the existing callback_failures_total pattern rather than reusing it. - outbox.controller.ts: GET /notifications/outbox/:id/attempts (full timeline for one record) and GET /notifications/outbox/delivery-attempts (filtered, paginated history across all records — outcome, failureCategory, type, from/to, limit/offset). Both admin/operator-role gated, matching the existing controller's guard pattern. The filtered endpoint is deliberately declared before the existing @get(':id') route in file order, since NestJS/Express match routes in declaration order and :id would otherwise capture 'delivery-attempts' as an id value. Verified: npx tsc --noEmit clean for all touched files (13 pre-existing, unrelated errors elsewhere in the repo, confirmed via git stash to predate this change). Known gap: no automated tests added yet for the new code paths (classifier, processor changes, service methods, controller endpoints). Recommend as immediate follow-up before merge if possible. Closes Pulsefy#716
|
@larryjay007 Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits. You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀 |
Contributor
|
Kindly fix workflow |
Contributor
Contributor
Author
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.
What
Adds a real delivery-attempt timeline for notifications, addressing the gap that
NotificationOutboxonly ever holds the latest attempt (lastAttemptAt/lastErrorgetoverwritten on every retry) — there was previously no way to see attempt 1 vs attempt 2 vs
attempt 3 for debugging a failed notification flow.
Schema
DeliveryAttemptOutcomeenum (success|failed) andNotificationDeliveryAttemptmodel, related to the existing
NotificationOutbox.via
prisma migrate dev, then hand-isolated the generated SQL to just our table. Theauto-diff also included three unrelated tables (
ArtifactAccessToken,ImportJob,SorobanEventCorrelation) that exist inschema.prismabut have no migration backingthem — pre-existing drift, unrelated to this change, excluded from this migration.
Failure categorization
Added
notification-failure-classifier.ts, bucketing raw errors into a small fixed set(
timeout,rate_limited,invalid_recipient,provider_error,unknown) rather thanpassing raw error text as a metric label. This deliberately diverges from the existing
incrementCallbackFailure/incrementTxSubmissionFailurepattern elsewhere in the codebase,which uses raw (truncated) error text as a Prometheus label — a cardinality-explosion risk.
Processor
onCompleted/onFailednow also insert aNotificationDeliveryAttemptrow and record thenew metrics, alongside the existing
NotificationOutboxstatus update (which remains thesource of truth for "current state"; the new table is the history).
Metrics
Two new counters:
notification_delivery_attempts_total(labels:type,outcome) andnotification_delivery_failures_by_category_total(labels:type,failure_category).Admin endpoints
Both added to the existing
OutboxController, matching itsApiKeyGuard/RolesGuard/@Roles(admin, operator)pattern:GET /notifications/outbox/:id/attempts— full timeline for one record.GET /notifications/outbox/delivery-attempts— filtered, paginated history(
outcome,failureCategory,type,from,to,limit,offset).Note: the filtered endpoint is declared before the existing
@Get(':id')route in thefile, since NestJS/Express match routes in declaration order —
:idwould otherwisesilently capture
"delivery-attempts"as an id value and make the new route unreachable.Testing
npx tsc --noEmitis clean for every file this PR touches. 13 pre-existing TypeScripterrors remain elsewhere in the repo (
artifact-token/upload-sessionspec files, tied tothe same
ArtifactAccessTokenschema drift noted above) — confirmed viagit stashtopredate this change and be unrelated.
Known gap, flagging honestly rather than overclaiming: no automated tests were added yet
for the new code paths (classifier, processor changes, service methods, controller
endpoints). Recommend this as a fast follow-up before merge, or happy to add them if there's
time before review.
Closes #716