You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
fix: drop RUM config values that would make aws-rum-web throw - #23
A string where aws-rum-web expects a RegExp or a function (typically from a broker's JSON) no longer breaks the host app. The RUM sink checks those settings before loading the SDK and fails closed: if one has the wrong type, RUM doesn't start for the page, and a console warning names the setting and the type it expects.
Key Changes
Config check (UwtRumTelemetrySink.performInit):
checks pagesToInclude/pagesToExclude (array of RegExp) and, inside telemetries, urlsToInclude/urlsToExclude (array of RegExp), addXRayTraceIdHeader (boolean or array of RegExp), and ignore/interactionId (function);
fails closed: any wrong type means RUM stays off for the page, as with a broker returning null. Dropping just the bad value would fail open: the filters and ignore decide what is not recorded, so RUM would record pages and URLs the app meant to keep out;
warns in every environment, not just dev mode, naming each setting and its expected type, never the value;
treats an explicit undefined as unset, and strips undefined telemetry options before they reach the SDK, whose Object.assign merge would otherwise replace a default with undefined and throw.
Tests (uwt-rum-telemetry.sink.spec.ts):
wrong-typed telemetry options and page filters stop RUM, with the exact warning;
valid RegExps, functions and booleans are passed through, with no warning;
undefined options are treated as unset (strict equality).
API docs:UwtRumAppMonitorConfig.telemetries, pagesToInclude and pagesToExclude describe the check.
README and DESIGN: the credentials and advanced-config examples say that settings needing a RegExp or a function belong in code. DESIGN explains why the sink fails closed.
Impact on consumers
No API change. Valid configuration behaves exactly as before.
Apps sending one of these settings with the wrong type used to break: requests or navigation threw. Now RUM stays off for that page, and the console says which setting to fix and what type it needs. Set it in code.
This PR stops a common configuration failure in the RUM (real user monitoring) sink. Broker JSON cannot carry RegExp or function values, and a string in their place made the AWS RUM SDK throw on requests or navigation. The sink now drops such values before the SDK sees them, so the SDK's default applies, and it logs a dev-mode warning naming what was dropped. The PR also corrects several README and DESIGN examples.
Changes:
The sink checks telemetries options, pagesToInclude and pagesToExclude before passing them on, and drops values of the wrong type.
Tests cover dropped string options, kept valid values, and a dropped string page filter.
API docs, the README, and DESIGN.md explain what is dropped, and update the logging, batching, and fragment-channel examples.
The PR description and issue #22 describe dropping the invalid value so the SDK default applies, with a dev-mode-only warning. The code instead keeps RUM off for the page and warns in every environment. The README, DESIGN.md, the API docs and the review-thread replies all match the code, so update the PR description's Key Changes, Tests and Impact on consumers sections, and the issue's expected behavior, to describe this fail-closed behavior.
null is treated as invalid here and at line 155, but the rest of this sink treats null as unset (config.endpoint ??, config.telemetries ??, config.allowCookies ?? true). A broker that sends unset optional fields as JSON null will turn RUM off for every page, and omitUndefined keeps null rather than dropping it. This needs a maintainer decision. If null means unset, skip it in this check and drop it in omitUndefined and withoutUndefinedOptions. If null is invalid, state in the API docs and DESIGN.md that brokers must omit the key.
Correct keepalive comment for hidden background tabs
projects/ngx-ui-watchtower/DESIGN.md:1709
The comment says keepalive is used "only while the page is going away", but the condition document.visibilityState === 'hidden' also matches a backgrounded tab that is not unloading. Reword the comment to say keepalive applies only while the page is hidden. Same issue in README.md:113-115.
telemetries is never checked for its shape. A non-iterable value (a number, true, or a JSON object) makes this for...of throw, and performInit()'s catch only reports through reportFailure, which returns early outside dev mode, so production stays silently off. A string such as "errors" iterates character by character without being flagged, and buildRumConfig() then passes it to the SDK unchanged. Check the shape up front and report it like the other settings.
The reason will be displayed to describe this comment to others. Learn more.
🔵 Needs a closer look
It changes whether production RUM starts and which pages it records, and the tests and type check could not be run in this review.
No open issues 🎉
2 previously missed issues
In code that hasn't changed since last review
Check current window before traversing parent windows
projects/ngx-ui-watchtower/DESIGN.md:2362
The new shellChannelName() never checks the fragment's own window. The loop starts at window but moves to w.parent before reading, so when the fragment runs in the shell's own window, the loop never runs and the shell's channel is never read. The previous window.top lookup did find it. Check the current window first, then climb through parent until it stops changing. This is a docs-only bug; the library code is unaffected.
Correct setting problem output documentation and parameter name
The JSDoc says only setting names are printed, but each entry also includes its expected type, for example pagesToExclude (expected an array of RegExp). The parameter names has the same mismatch. Say that each setting's name and expected type are printed, never its value, and rename the parameter to problems.
The reason will be displayed to describe this comment to others. Learn more.
🔵 Needs a closer look
It changes RUM startup gating and privacy-sensitive filter behavior, so a person should confirm the behavior change for apps with misconfigured settings.
No open issues 🎉
2 previously missed issues
In code that hasn't changed since last review
Correct invalid page filter wording to session scope
Invalid pagesToInclude or pagesToExclude values keep RUM from starting "for the page", but the sink disables RUM for the rest of the session, as it does for a null from the provider. The same wrong wording appears in uwt-rum-credentials.ts:109, uwt-rum-telemetry.sink.ts:268-271, and the README and DESIGN additions. Change all of them to "for the session".
Clarify buildRumConfig documentation covers only checked settings
The buildRumConfig doc says "Values the SDK can't use never get here", but performInit() only checks the page filters, the telemetries shape, and the telemetry options in TELEMETRY_OPTION_CHECKS. Other settings, such as sessionSampleRate, pass through unchecked. Name the checked settings in the doc so it doesn't promise more than the check does.
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
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.
Overview
A string where
aws-rum-webexpects aRegExpor a function (typically from a broker's JSON) no longer breaks the host app. The RUM sink checks those settings before loading the SDK and fails closed: if one has the wrong type, RUM doesn't start for the page, and a console warning names the setting and the type it expects.Key Changes
UwtRumTelemetrySink.performInit):pagesToInclude/pagesToExclude(array ofRegExp) and, insidetelemetries,urlsToInclude/urlsToExclude(array ofRegExp),addXRayTraceIdHeader(boolean or array ofRegExp), andignore/interactionId(function);null. Dropping just the bad value would fail open: the filters andignoredecide what is not recorded, so RUM would record pages and URLs the app meant to keep out;undefinedas unset, and stripsundefinedtelemetry options before they reach the SDK, whoseObject.assignmerge would otherwise replace a default withundefinedand throw.uwt-rum-telemetry.sink.spec.ts):RegExps, functions and booleans are passed through, with no warning;undefinedoptions are treated as unset (strict equality).UwtRumAppMonitorConfig.telemetries,pagesToIncludeandpagesToExcludedescribe the check.RegExpor a function belong in code. DESIGN explains why the sink fails closed.Impact on consumers
Closes #22