Repository navigation
Skip telemetry reconstruction for unrelated notification listeners - #2785
Conversation
Check for the matching sent, discarded or retry callback before rebuilding notification items. Preserve fresh snapshots and existing storage behavior, with regression coverage for dynamic listeners, retries and persisted items. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 865183c5-b401-4f7c-b41d-7d8384f0bba8
There was a problem hiding this comment.
🟢 Approval recommended
The narrow guards preserve notification behavior, with focused regression coverage and no identified blocking issues.
No open issues 🎉
What changed in this PR
Optimizes channel notifications by skipping telemetry reconstruction when no listener handles the relevant callback, while preserving matching-listener behavior.
Changes:
- Adds callback-specific guards for sent, discarded, and retry notifications.
- Adds regression coverage for listener changes, metadata preservation, mutation isolation, and persisted buffers.
- Documents the optimization in release notes.
| File | Description |
|---|---|
| RELEASES.md | Documents the notification optimization. |
| channels/applicationinsights-channel-js/Tests/Unit/src/Sender.tests.ts | Adds five notification regression cases. |
| channels/applicationinsights-channel-js/src/Sender.ts | Checks for matching callbacks before reconstructing telemetry. |
Review effort: Balanced
Let managers opt into callback-specific reconstruction checks without suppressing custom handlers. Add notification compatibility and SDK Stats regression coverage. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 4272788f-4337-461b-bf8c-be1ff308e19e
Hector Hernandez (hectorhdzg)
left a comment
Hector Hernandez (hectorhdzg)
left a comment
There was a problem hiding this comment.
Nice work overall. canNotify lines up with how dispatch actually works, and the tests cover a lot of ground. A few things before this goes in:
- The build is red. See the inline comment.
- The PR description still reads like the first commit (Sender-only, "public notification contracts are unchanged"). The second commit adds a public method to core, a new test file and a size bump, so could you update it? The Copilot review summary is based on the old version too.
- Small design question: the first commit got the same win just by checking listeners inside Sender. Is handling custom managers worth adding new core API and bundle size? I'm fine either way, I just want it to be a deliberate choice.
| * @param name - The telemetry notification method to query. | ||
| * @returns False only for an unmodified built-in handler with no matching listeners. | ||
| */ | ||
| canNotify(name: "eventsSent" | "eventsDiscarded" | "eventsRetry"): boolean { |
There was a problem hiding this comment.
This is what's breaking CI. It's optional on INotificationManager but required here, so INotificationManager can no longer be assigned to NotificationManager. 1DS PostChannel fails with TS2322 (PostChannel.ts(163)), and any customer code that types getNotifyMgr() as NotificationManager would break the same way. Making it optional here too (canNotify?(...)) should fix it.
There was a problem hiding this comment.
This would be better to just be name: string so that the signature doesn't change for any future events that you may want to add / test for
There was a problem hiding this comment.
nit: and it's probably better renamed as hasListener (or a shortened name), as technically you can always send a notification event if no one is listening 😄
There was a problem hiding this comment.
Renamed this to hasListener, changed name to string, and made it optional on both the class and interface to fix the assignment issue.
Resolve release-note and Sender test import conflicts while preserving upstream changes. Rename the optional notification hint to hasListener(name: string), retain custom-manager compatibility, and extend regression coverage. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Raise only the uncompressed dist and browser budgets by 1 KB to account for the optional notification listener hint. Keep both 74 KB deflate limits unchanged. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
/azp run AppInsights-JS-SDK-PR |
|
No pipelines are associated with this pull request. |
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Only reconstruct telemetry for sent, discarded, or retry notifications when a registered listener handles that callback. The existing no-listener fast path stays unchanged, and unrelated listeners no longer cause a notification array and one object per item to be allocated.
This is a small follow-up to microsoft/ApplicationInsights-JS#2707. Matching listeners, including SDK Stats, still use fresh notification objects. Deferred serialization is not part of this change.
Adds five regression cases for callback-specific extraction, dynamic listener changes, original names, SDK Stats self-filtering, retry mutation isolation, partial success, and persisted-session compatibility. The storage format and public notification contracts are unchanged.
Validation: all 100 channel unit tests pass, along with the TypeScript/browser builds and ES5 checks. The new extraction cases fail against upstream and pass with this change.
npm run lint-fixcompleted with the repository's Node 24 ESLint skip, and a direct ESLint comparison confirmed no new diagnostics.