feat(notify): delivery observability via ctx.log + ctx.metrics - #183
Merged
Conversation
Thread the request's ctx.log / ctx.metrics into the notify facade (new optional CreateNotifyOptions.log / .metrics) so every send emits into the durable log + metric pipelines instead of a bespoke store: - notify.send counter on push, chat/in-app/webhook, and multi-channel send, dimensioned by channel / provider / status (low-cardinality). - log.warn only on failure, carrying error + push subscription/user ids (trace-correlated, durably archived). - Stable status vocabulary accepted | failed | gone — honest to edge push (Web Push / FCM give no delivery/open receipts). - notify.skipped counter for the "sent 0 because…" signal (no-subscriptions-matched, channel-not-configured). Bounded retention is satisfied by NOT adding a store — everything rides the existing sampled/TTL'd log + metric history. Structural NotifyLogger / NotifyMetrics avoid an @lunora/server dependency edge. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NjFm7WWd7aqnKduj2thnNU
Relocate the createNotify build fragment to after log/metrics are in scope in
the shard context builder and pass { log, metrics }, so the generated ctx.notify
emits the notify.send / notify.skipped observability signals. Non-notify apps
are unaffected (empty fragment); golden fixture + assertion updated.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01NjFm7WWd7aqnKduj2thnNU
Regenerate the notify-demo example's _generated/shard.ts after the createNotify codegen change. Generated output only; not test-gated. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NjFm7WWd7aqnKduj2thnNU
✅ Deploy Preview for lunorash ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Contributor
|
Thank you for following the naming conventions! 🙏 |
Contributor
|
Thank you for confirming the Contributor License Agreement! 🙏 |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughNotification delivery now exposes typed observability contracts, records send and skip outcomes across notification paths, logs failures, and wires request-scoped logging and metrics into generated notify contexts. Documentation and benchmarks cover the resulting behavior. ChangesNotify delivery observability
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant NotifyFacade
participant DeliveryProvider
participant SubscriptionStore
participant CtxMetrics
participant CtxLog
NotifyFacade->>DeliveryProvider: send notification
DeliveryProvider-->>NotifyFacade: return receipt
NotifyFacade->>SubscriptionStore: update subscription status
NotifyFacade->>CtxMetrics: count notify.send or notify.skipped
NotifyFacade->>CtxLog: warn on failed delivery
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Merging this PR will not alter performance
Comparing 
Footnotes
|
Add a "Delivery metrics (notify)" section to the observability concept doc and a "Delivery observability" section to the notify README covering the notify.send / notify.skipped series, the accepted|failed|gone status vocabulary, and the failure log line — plus the honest no-delivery-receipt boundary. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NjFm7WWd7aqnKduj2thnNU
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/notify/src/notify.ts (1)
14-17: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConstrain skip reasons to the exported vocabulary.
observeSkipaccepts any string, allowing metric-label drift outsideNotifySkipReason.Proposed change
NotifyLogger, NotifyMetrics, + NotifySkipReason, - const observeSkip = (channel: string, reason: string): void => { + const observeSkip = (channel: string, reason: NotifySkipReason): void => {As per path instructions, ensure proper TypeScript types.
Also applies to: 212-215
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/notify/src/notify.ts` around lines 14 - 17, Update observeSkip to accept only the exported NotifySkipReason type instead of an arbitrary string, importing that type where needed. Apply the same TypeScript constraint to both observeSkip declarations or usages identified in the diff, while preserving existing metric behavior.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/notify/src/notify.ts`:
- Around line 313-315: The receipt status handling in notify must preserve gone
for push receipts: update the loop around observeSend to use
pushDeliveryStatus(receipt, receiptError(receipt)) when receipt.channel is
"push", while retaining the accepted/failed mapping for other channels. In
packages/notify/README.md lines 103-110 and
apps/docs/src/content/docs/concepts/observability.mdx lines 379-394, clarify
that automatic pruning applies only to stored ctx.push.send/broadcast
subscriptions; generic direct push targets may report gone but cannot be pruned.
---
Nitpick comments:
In `@packages/notify/src/notify.ts`:
- Around line 14-17: Update observeSkip to accept only the exported
NotifySkipReason type instead of an arbitrary string, importing that type where
needed. Apply the same TypeScript constraint to both observeSkip declarations or
usages identified in the diff, while preserving existing metric behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 51a52e81-c078-4fe5-8a80-c4a08924a3e9
⛔ Files ignored due to path filters (4)
examples/notify-demo/lunora/_generated/shard.tsis excluded by!**/_generated/**and included by nonepackages/codegen/__tests__/fixtures/simple/expected/_generated/shard.tsis excluded by!**/_generated/**,!**/__tests__/**and included bypackages/**packages/codegen/__tests__/run-codegen.test.tsis excluded by!**/__tests__/**,!**/*.test.tsand included bypackages/**packages/notify/__tests__/notify.test.tsis excluded by!**/__tests__/**,!**/*.test.tsand included bypackages/**
📒 Files selected for processing (6)
apps/docs/src/content/docs/concepts/observability.mdxpackages/codegen/src/emit.tspackages/notify/README.mdpackages/notify/src/index.tspackages/notify/src/notify.tspackages/notify/src/types.ts
Comment on lines
+313
to
+315
| for (const receipt of receipts) { | ||
| observeSend(receipt.channel ?? "unknown", receipt.provider, receipt.successful ? "accepted" : "failed", { error: receiptError(receipt) }); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Preserve gone for generic push receipts.
ctx.notify.send() labels all failed push receipts as failed, bypassing pushDeliveryStatus; unregistered endpoints therefore disappear from the gone metric series.
packages/notify/src/notify.ts#L313-L315: forreceipt.channel === "push", derive status withpushDeliveryStatus(receipt, receiptError(receipt)); retainaccepted/failedmapping for other channels.packages/notify/README.md#L103-L110: scope automatic pruning to storedctx.push.send/broadcastsubscriptions; generic direct push targets can be reported asgonebut cannot be pruned.apps/docs/src/content/docs/concepts/observability.mdx#L379-L394: make the same pruning distinction.
Proposed status mapping
for (const receipt of receipts) {
- observeSend(receipt.channel ?? "unknown", receipt.provider, receipt.successful ? "accepted" : "failed", { error: receiptError(receipt) });
+ const error = receiptError(receipt);
+ const status =
+ receipt.channel === "push"
+ ? pushDeliveryStatus(receipt, error)
+ : receipt.successful
+ ? "accepted"
+ : "failed";
+
+ observeSend(receipt.channel ?? "unknown", receipt.provider, status, { error });
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| for (const receipt of receipts) { | |
| observeSend(receipt.channel ?? "unknown", receipt.provider, receipt.successful ? "accepted" : "failed", { error: receiptError(receipt) }); | |
| } | |
| for (const receipt of receipts) { | |
| const error = receiptError(receipt); | |
| const status = | |
| receipt.channel === "push" | |
| ? pushDeliveryStatus(receipt, error) | |
| : receipt.successful | |
| ? "accepted" | |
| : "failed"; | |
| observeSend(receipt.channel ?? "unknown", receipt.provider, status, { error }); | |
| } |
📍 Affects 3 files
packages/notify/src/notify.ts#L313-L315(this comment)packages/notify/README.md#L103-L110apps/docs/src/content/docs/concepts/observability.mdx#L379-L394
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/notify/src/notify.ts` around lines 313 - 315, The receipt status
handling in notify must preserve gone for push receipts: update the loop around
observeSend to use pushDeliveryStatus(receipt, receiptError(receipt)) when
receipt.channel is "push", while retaining the accepted/failed mapping for other
channels. In packages/notify/README.md lines 103-110 and
apps/docs/src/content/docs/concepts/observability.mdx lines 379-394, clarify
that automatic pruning applies only to stored ctx.push.send/broadcast
subscriptions; generic direct push targets may report gone but cannot be pruned.
The codegen ctx-splice, notify_send_outside_action advisor lint, and Studio Notifications page listed as "remaining" are all shipped; the referenced plan file is gone. Rewrite Status to reflect what's shipped (incl. delivery observability) and note the deferred per-delivery activity feed. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NjFm7WWd7aqnKduj2thnNU
Address thermo review findings: - Pin the exact provider dimension on the channel-send metric assertion (was objectContaining, hiding the receipt-provider fallback). - Add a multi-channel notify.send test covering the per-receipt loop and the `?? "unknown"` channel/provider fallback (only reachable via an unlabeled receipt) plus the failure warn on that path. - Trim a redundant inline comment that restated observeSend's JSDoc. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NjFm7WWd7aqnKduj2thnNU
Add CodSpeed benches for the observability critical path, motivated by a trace finding: every ctx.metrics.count does a synchronous SQLite select+upsert+prune per call (recordMetricHistory in ShardDOBase.recordMetric), so a broadcast that emits notify.send per recipient pays that cost N times. - do/__bench__/normalize-log-fields.bench.ts — the fn every metric attr + log field routes through (flat/nested/merge/empty). - do/__bench__/metrics-emit.bench.ts — ctx.metrics.count CPU-only vs durable; the durable path measures ~112x the CPU cost (~19us/call). - notify/__bench__/broadcast-observability.bench.ts — facade overhead, instrumented vs no-handles, ok vs fail; confirms the facade adds ~0 (the cost is downstream in the per-count SQLite write). Wires notify's test:bench script + vitest.bench.config.ts and adds __bench__ to its tsconfig include. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NjFm7WWd7aqnKduj2thnNU
Each ctx.metrics.count is a synchronous SQLite upsert (~19us — see the
metrics-emit bench), so emitting notify.send per recipient made a broadcast pay
that durable write O(N) times. Fold a broadcast's outcomes into one count per
(kind, status) bucket — at most kinds×3 emits — with the bucket total as the
metric value.
- deliver() returns { receipt, status } and counts inline only for a single
push.send; a broadcast passes countInline=false and emits aggregated buckets.
- Failure LOGS stay per-recipient (no durable write; they carry the ids).
- Split observeSend into countSend (with a count arg) + warnFailedSend, the
latter called only inside a failed guard so its fields never allocate on the
hot success path.
- BroadcastResult shape unchanged; reuses the derived status instead of
re-deriving via isGoneError.
Adds a test asserting a mixed 6-recipient broadcast emits exactly 3 aggregated
counts + 2 per-recipient warns. Docs updated.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01NjFm7WWd7aqnKduj2thnNU
The new bench config sits at the package root, outside the tsconfig project, so
type-aware eslint failed to parse it ("parserOptions.project ... not found").
Add it to the ignores list next to vitest.config.ts, matching @lunora/do.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01NjFm7WWd7aqnKduj2thnNU
prisis
added a commit
that referenced
this pull request
Jul 24, 2026
Sync the cloud observability + security-hardening branch with alpha (OTLP resource attributes #182, notify observability #183, do perf #184, workerd custom-spans test #180, ai-gateway correlation #181, and release bumps). Clean auto-merge — no conflicts. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0197KjhmBDB3PXAc6TXiPqZ9
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
1 participant
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
Makes
@lunora/notifydeliveries observable through the pillars that already have durable storage and Studio surfaces — no bespoke store, admin RPC, or Studio tab. This is "Tier 1" from the design discussion, reusing the structured log sink (#147/#154/#155) and durable metric history (#149/#158) rather than a parallel D1 table.Design cross-checked against Novu's own approach (they migrated their activity feed to a batched, TTL'd ClickHouse event/rollup pipeline — the same shape as our log sink + metric history), and its status vocabulary + "why 0 sent" reasons are borrowed here.
Changes
@lunora/notifyfacade — threads the request'sctx.log/ctx.metricsin via new optionalCreateNotifyOptions.log/.metrics:notify.sendcounter on push (deliver), the chat/in-app/webhook helpers, and multi-channelsend()— dimensionschannel/provider/status, all low-cardinality.log.warnonly onfailed, carryingerror+ (push)subscriptionId/userId— trace-correlated, durably archived. Successes/prunes stay off the log.accepted | failed | gone— honest to edge push (Web Push/FCM give no delivery/open receipts, so nodelivered/opened).notify.skippedcounter — the "sent 0 because…" signal:no-subscriptions-matched,channel-not-configured.NotifyLogger/NotifyMetrics(theD1Likepattern) avoid an@lunora/serverdependency edge. No new store ⇒ bounded retention rides the existing sampled/TTL'd pipelines.@lunora/codegen— relocates thecreateNotify(...)build to afterlog/metricsare in scope and passes{ log, metrics }. Non-notify apps get zero churn; golden fixture +notify-demoregenerated (2-line diffs).Scope boundary
Web Push and FCM provide no delivery/open callbacks, so the feed truthfully reports only accepted / failed / gone at send time. A filterable per-delivery feed / per-device history (Novu-style "activity" drill-down) is deliberately out of scope (would need a field-level predicate on the log reader or a dedicated store) and can follow if demanded.
Verification
@lunora/notify: 44/44 tests (7 new observability cases) ·lint:typesclean · eslint clean@lunora/codegen: 142/142 tests ·lint:typesclean🤖 Generated with Claude Code
Summary by CodeRabbit
ctx.metricsandctx.log, including low-cardinalitynotify.send(channel,provider,status:accepted,failed,gone) andnotify.skipped(channel,reason:no-subscriptions-matched,channel-not-configured).acceptedreflects provider acceptance (not delivery/open receipts); failed sends also emit warning logs with delivery context.test:benchscript.