Skip to content

feat: retry RUM start-up with backoff and fix user attribution - #18

Open
fateeand wants to merge 10 commits into
mainfrom
17-rum-sink-one-failed-broker-request-at-start-up-turns-rum-off-for-the-whole-page
Open

fateeand wants to merge 10 commits into
mainfrom
17-rum-sink-one-failed-broker-request-at-start-up-turns-rum-off-for-the-whole-page

Conversation

@fateeand

@fateeand fateeand commented Oct 23, 2026 •

Copy link
Copy Markdown
Collaborator

Overview

A failed broker request at start-up no longer switches RUM off for the page. The sink retries with backoff and jitter, keeping events buffered. The RUM client and the sink's timers now run outside Angular's zone. User attribution is fixed: signing out while nobody is signed in no longer starts a new session, events no longer go out under the wrong user around a sign-in or sign-out, and every event says whether a user is signed in.

Key Changes

  • Start-up retry (UwtRumTelemetrySink):
    • when load() throws, the sink waits a random time up to a limit that starts at 1 s and doubles after each failure, up to 60 s, keeping events buffered (at most 100);
    • after 5 minutes of failures it gives up and switches RUM off, as before;
    • it warns once on the first failure and once when giving up (dev mode only);
    • a failed SDK download or client construction is still not retried, because browsers remember a failed module import;
    • credential refresh retries stay at a fixed 30 s, since the client keeps running on valid credentials meanwhile.
  • Zone-neutral start: init() runs inside NgZone.runOutsideAngular, so the SDK's dispatch interval and the sink's retry and refresh timers no longer keep a zone-based app from becoming stable. This fixes an older bug.
  • Sign-out only on a real sign-out: setUserId(undefined) or setUserId('') starts a new session only when a user was signed in. Repeating it, or an auth stream's initial undefined, does nothing. A real sign-out made before the client starts is applied once it does; a sign-in in the meantime cancels it.
  • No events under the wrong user: the SDK reads the user id when it sends a batch, not when it records an event. On a user change, the sink first sends what's waiting, under the old id. It skips that when the client already has the new id, as after a reload.
  • signedIn on every event: event metadata carries signedIn: true | false, so a real user id can be told from an anonymous one; both are UUIDs. It is absent until the app's first setUserId call.
  • Docs: JSDoc, DESIGN.md and README cover the retry behaviour, sign-in and sign-out, signedIn, the UUID requirement for user ids, cookies and the load() contract.

Impact on consumers

  • No API change.
  • Fewer sessions: apps that call setUserId(undefined) at startup or on every auth emission no longer get a new session and a new anonymous user id each time.
  • One new event metadata key, signedIn. Call setUserId early, e.g. right after MSAL initializes, so no event goes out without it.
  • User ids must be UUIDs: RUM's API accepts nothing else. With MSAL, use the account's localAccountId, not homeAccountId. This was always the case; it's now documented.
  • Providers that throw on failure (like the setup skill's template) get start-up retries.
  • Providers that return null on a failed request behave as before: RUM off for the page. Switch them to throwing to get the retry.

Closes #17

@github-actions

github-actions Bot commented Oct 23, 2026 •

Copy link
Copy Markdown
Contributor

Coverage report

St.❔
Category Percentage Covered / Total
🟢 Statements
97.22% (+0.11% 🔼)
1330/1368
🟢 Branches
95.35% (+0.08% 🔼)
738/774
🟢 Functions
97% (+0.05% 🔼)
323/333
🟢 Lines
97.57% (+0.09% 🔼)
1286/1318

Test suite run success

763 tests passing in 24 suites.

Report generated by 🧪jest coverage report action from 48f09e2

@fateeand fateeand changed the title fix: retry RUM start-up after a failed broker request and run the client outside Angular's zone` fix: retry RUM start-up after a failed broker request and run the client outside Angular's zone Oct 23, 2026
@fateeand
fateeand requested a lite review from Copilot October 23, 2026 13:40

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Documentation contains a broken startup-method reference and an outdated claim that load() is called only once.

1 open issue
What changed in this PR

This pull request improves AWS RUM startup resilience and Angular integration by retrying failed credential requests, running initialization outside Angular’s zone, and preserving sign-out intent until startup completes.

Changes:

  • Adds bounded startup retries with event buffering and failure handling.
  • Runs RUM initialization and related timers outside Angular’s zone.
  • Applies pending sign-outs when the client starts.
  • Updates tests and documentation for retry, session, cookie, and provider behavior.
File Description
projects/​ngx-ui-watchtower/​rum/​src/​lib/​uwt-rum-telemetry.sink/​uwt-rum-telemetry.sink.ts Adds startup retries, zone-neutral initialization, and deferred sign-out handling.
projects/​ngx-ui-watchtower/​rum/​src/​lib/​uwt-rum-telemetry.sink/​uwt-rum-telemetry.sink.spec.ts Tests retry behavior, retry limits, destruction, zone execution, and deferred sign-out.
projects/​ngx-ui-watchtower/​rum/​src/​lib/​uwt-rum-credentials/​uwt-rum-credentials.ts Updates session, cookie, and credential-provider contract documentation.
projects/​ngx-ui-watchtower/​README.md Updates the credentials-provider example to throw on failed requests.
projects/​ngx-ui-watchtower/​DESIGN.md Documents retry, session, cookie, sign-out, and zone behavior.

Review effort: Lite

Comment thread projects/ngx-ui-watchtower/DESIGN.md Outdated
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Approval recommended

The reviewed changes have no unresolved issues and all approval assessments indicate readiness.

No open issues 🎉

1 resolved since last review

Review effort: Lite

Comment thread projects/ngx-ui-watchtower/DESIGN.md Outdated
left collecting with stale credentials.

Throwing means the request failed, not that telemetry is unwanted. The sink
tries again every 30 seconds — at startup, keeping events buffered meanwhile,

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn’t exponential backoff with jitter be better than a constant 30 s retry interval? The SDK only starts capturing errors and failed HTTP calls once the client exists, so a one-second broker blip currently costs at least 30 secs of that data. Jitter would also keep tabs from retrying in lockstep after a mass reload during an outage.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great suggestion. Now we retry with exponential backoff from 1 to 60 seconds, and give up after 5 minutes.

if (this.awsRum) {
this.awsRum.startSession({ userId: uwtUuid() });
} else {
this.signOutPending = true;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This also fires on the initial undefined most auth streams emit (user$.pipe(map(u => u?.id))), so every page load ends in startSession(). That means an extra session_start, re-rolled sampling and no session continuity.

Could we only start a new session on a real signed-in → signed-out transition (no-op if there was no previous userId)? Plus a test: setUserId(undefined) before init, then init() → startSession not called.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch. setUserId(undefined) / '' now starts a new session only on a real signed-in → signed-out transition; with no previous userId it's a no-op, so an auth stream's initial undefined (and any repeat) leaves the session alone. A real sign-out made before init is still applied once the client starts.

Also added two related updates:

  • The SDK reads the user id when it sends a batch, not when it records, so on a user change the sink first sends what's waiting under the old id (skipped when the client already has the new id, e.g. after a reload).
  • Every event now carries signedIn property in its metadata (absent until the app's first setUserId), since anonymous ids and MSAL ids are both UUIDs and can't otherwise be distinguished.

@fateeand fateeand changed the title fix: retry RUM start-up after a failed broker request and run the client outside Angular's zone feat: retry RUM start-up with backoff and fix user attribution Oct 29, 2026

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

A critical active finding remains because the user-ID transition flushes only one queued SDK batch, allowing older events to be attributed to the next user.

1 open issue

Review effort: Lite

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Critical attribution bugs remain: buffered events can inherit later authentication state, and a restored SDK user ID may survive initial sign-out, misattributing telemetry across sessions.

2 open issues
1 resolved since last review

Review effort: Lite

@fateeand
fateeand requested a lite review from Copilot October 29, 2026 16:59

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

Human assessment is needed because constructor-time SDK events can bypass the metadata hook, creating a potential conflict with the documented guarantee for early signed-in state propagation.

No open issues 🎉

2 resolved since last review

Review effort: Lite

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

RUM sink: one failed broker request at start-up turns RUM off for the whole page

3 participants