feat: add generic exception telemetry - #5134
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e1a0bd4a52
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| return undefined; | ||
| } | ||
|
|
||
| const collector = installGlobalUnhandledExceptionTelemetry({ |
There was a problem hiding this comment.
In browsers where localStorage is disabled or unwritable (for example privacy-restricted contexts, third-party storage blocking, or enterprise policies), PersistentEventQueue throws during its storage probe, and this call is reached by default from NewMetrics construction because the new config enables telemetry by default. That makes Webex.init() fail instead of just skipping optional exception telemetry, unlike the event-target guard above; please catch this installer failure or add a storage-availability guard before installing.
Useful? React with 👍 / 👎.
| return; | ||
| } | ||
|
|
||
| event.metadata = metadata; |
There was a problem hiding this comment.
When an app's getMetadata() returns a mutable object, assigning that object directly means the in-memory queue keeps the same reference until the delayed pump submits it. Because events wait at least the dedupe window before submission, any app-state mutation in that interval changes the metadata reported for the already-captured exception, so telemetry can describe the wrong route/meeting state; store a serialized/deep-cloned snapshot after the size check instead.
Useful? React with 👍 / 👎.
|
This pull request is automatically being deployed by Amplify Hosting (learn more). |
08cd78e to
b68bd41
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b68bd41513
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| const telemetryConfig = webex.config?.metrics?.unhandledExceptionTelemetry; | ||
|
|
||
| if (telemetryConfig?.enabled === false) { | ||
| uninstallGlobalUnhandledExceptionTelemetry(); |
There was a problem hiding this comment.
When an application uses the early collector and an exception is captured before Webex.init(), setting metrics.unhandledExceptionTelemetry.enabled: false reaches this branch, but uninstallGlobalUnhandledExceptionTelemetry() only stops listeners/deletes the global symbol and does not remove the webex-unhandled-exception:v1... localStorage entries. Those opt-out events remain on disk and will be drained if telemetry is enabled later in the same origin, so disabling telemetry should also clear the collector's persisted queue.
Useful? React with 👍 / 👎.
abeb352 to
56649f6
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 56649f6c8d
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| return !TELEMETRY_SERVICES.has(getService(options)); | ||
| }; | ||
|
|
||
| export const createNetworkFailureMetric = ( |
There was a problem hiding this comment.
As added, this helper is not wired to the SDK event that actually reports request failures: I searched the source for createNetworkFailureMetric/NETWORK_REQUEST_FAILURE_METRIC and found only this module and its unit test, while RequestEventInterceptor emits request:failure from packages/@webex/webex-core/src/interceptors/request-event.js at lines 49 and 86. In any real SDK request failure outside the test's manual webex.emit, nothing listens, so JS_SDK_NETWORK_REQUEST_FAILURE is never submitted; please register a request:failure handler in the metrics plugin and call submitClientMetrics with this metric.
Useful? React with 👍 / 👎.
56649f6 to
15b2290
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 15b229059e
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| existingCollector.updateApplicationContext({ | ||
| appVersion: options.appVersion, | ||
| getMetadata: options.getMetadata, | ||
| }); |
There was a problem hiding this comment.
When an app installs the early collector before SDK load and supplies telemetry limits only in Webex.init, this adoption branch drops every limit option (maxSerializedMetadataBytes, maxEventsPerMinute, maxEvents, maxAgeMs) and only updates appVersion/getMetadata. The existing collector keeps its constructor defaults, so future captured exceptions can include metadata or retain queued events beyond the app's configured bounds; pass these options into the existing collector or require them to be set in the bootstrap.
Useful? React with 👍 / 👎.
15b2290 to
b196322
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b196322211
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| try { | ||
| const error = { | ||
| ...details, | ||
| message: truncate(details.message, MAX_ERROR_MESSAGE_LENGTH), |
There was a problem hiding this comment.
When an exception message itself contains a URL with query parameters or fragments, this only truncates details.message before sending it in eventPayload.error.message; for example, a rejected request wrapper can throw GET https://host/path?token=... failed, and that token will be reported even though the stack/resource URL paths are sanitized. Please run messages through the URL-detail sanitizer before truncating/submitting them.
Useful? React with 👍 / 👎.
b196322 to
a0fc6db
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a0fc6dbded
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
|
||
| this.capture({ | ||
| kind: 'unhandledrejection', | ||
| message: typeof reason?.message === 'string' ? reason.message : stringifyReason(reason), |
There was a problem hiding this comment.
When an unhandled rejection uses a plain object instead of an Error or string, this path JSON-serializes the whole object into eventPayload.error.message; request libraries and app code commonly reject with objects containing URLs, headers, bodies, or IDs, so enabled telemetry can send credentials or PII without any allowlist/redaction. Please avoid stringifying arbitrary rejection reasons, or redact/allowlist object fields before submission.
Useful? React with 👍 / 👎.
a0fc6db to
c6c2972
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c6c2972e2b
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| eventTarget.addEventListener('error', (event) => this.captureError(event), true); | ||
| eventTarget.addEventListener('unhandledrejection', (event) => this.captureRejection(event)); |
There was a problem hiding this comment.
When a page creates another SDK instance with telemetry enabled, such as a re-login flow, Webex.init() returns a fresh instance (packages/webex/src/webex.js:79-88) and this constructor adds another permanent window listener without a singleton guard or removeEventListener path. Every later browser error/rejection is then captured and submitted once per previously initialized reporter, so the optional telemetry duplicates events and can keep submitting through stale SDK instances.
Useful? React with 👍 / 👎.
| } | ||
|
|
||
| function removeUrlDetailsFromText(value: string | undefined): string | undefined { | ||
| return value?.replace(/\b[a-z][a-z0-9+.-]*:\/\/[^\s)]+/gi, (url) => removeUrlDetails(url) ?? url); |
There was a problem hiding this comment.
When enabled telemetry captures an exception message or stack containing a relative request URL such as GET /api?token=secret failed, this sanitizer leaves the query/fragment intact because it only matches scheme:// URLs. Fresh evidence beyond the earlier absolute-URL concern is that the current redaction regex still bypasses same-origin relative URLs, so those secrets are copied into eventPayload.error.message or stack before submission.
Useful? React with 👍 / 👎.
c6c2972 to
24b8dd5
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 24b8dd50b3
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if (webex.config?.metrics?.unhandledExceptionTelemetry?.enabled !== true) { | ||
| return undefined; |
There was a problem hiding this comment.
When a page creates a new SDK instance with metrics.unhandledExceptionTelemetry.enabled omitted or set to false after an earlier enabled instance, this early return leaves activeTelemetry installed on window, so subsequent errors are still captured and submitted through the stale SDK even though the current configuration opted out. Please stop and clear any existing reporter before returning for disabled or unsupported telemetry.
Useful? React with 👍 / 👎.
24b8dd5 to
c932ee5
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c932ee5da9
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| this.flushTimer = undefined; | ||
| } | ||
|
|
||
| this.pendingEvents.forEach((event) => this.submit(event)); |
There was a problem hiding this comment.
When a second Webex.init() uses the default/disabled metrics.unhandledExceptionTelemetry after an earlier enabled instance has captured an error but before the 1s dedupe timer fires, startUnhandledExceptionTelemetry() calls stop() and this line submits the stale reporter’s pending event through the old SDK before returning disabled. Fresh evidence beyond the earlier stale-listener concern is that the listener is now stopped, but stop() still flushes pendingEvents; the opt-out path should clear them instead of submitting.
Useful? React with 👍 / 👎.
c932ee5 to
8eb0b65
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8eb0b65886
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| return value | ||
| ?.replace(/\b[a-z][a-z0-9+.-]*:\/\/[^\s)]+/gi, (url) => removeUrlDetails(url) ?? url) | ||
| .replace( | ||
| /(^|[\s("'=[{])((?:\/|\.\.?\/)[^\s)"'\]}]+)/g, |
There was a problem hiding this comment.
When an error message or stack contains a same-origin relative URL without a leading slash or dot, such as GET api/messages?token=secret failed or at load (scripts/app.js?token=secret:10:20), this regex does not match it because it only handles scheme URLs and paths beginning with /, ./, or ../. Those query strings/fragments therefore remain in eventPayload.error.message or stack despite the new sanitizer, so enabled exception telemetry can still leak URL secrets; please cover bare relative path references before truncating/submitting.
Useful? React with 👍 / 👎.
| constructor(webex: WebexForUnhandledExceptionTelemetry, eventTarget: BrowserEventTarget) { | ||
| this.webex = webex; | ||
| this.eventTarget = eventTarget; | ||
| eventTarget.addEventListener('error', this.errorListener, true); |
There was a problem hiding this comment.
eventTarget.addEventListener('error', this.errorListener, {capture: true});
8eb0b65 to
f6f31b5
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f6f31b527d
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| } | ||
|
|
||
| private captureError(event: any): void { | ||
| const resourceUrl = removeUrlDetails(event?.target?.src ?? event?.target?.href); |
There was a problem hiding this comment.
When a resource load failure comes from an element whose src/href is a data: URL or another very long URL path, this keeps the full value; unlike message and stack, resourceUrl is never length-capped or scheme-filtered before it is copied into eventPayload.error.resourceUrl and submitted. An invalid <img src="data:..."> can therefore enqueue arbitrary inline content or oversized payloads into client metrics, so reject non-network schemes or cap the resource URL before capture.
Useful? React with 👍 / 👎.
| (_, prefix, url) => `${prefix}${removeUrlDetails(url) ?? url}` | ||
| ) | ||
| .replace( | ||
| /(^|[\s("'=[{])((?:[a-z0-9._~%-]+\/)+(?:[a-z0-9._~%-]+)?[?#][^\s)"'\]}]+)/gi, |
There was a problem hiding this comment.
When an error message or stack contains a same-origin relative URL with no slash, such as GET api?token=secret failed or at load (app.js?token=secret:1:1), this bare-relative matcher does not run because it requires at least one / before [?#]. Those query strings/fragments remain in eventPayload.error.message or stack before submission, so please cover single-segment relative references as well.
Useful? React with 👍 / 👎.
f6f31b5 to
7a3d9cf
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7a3d9cf081
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| } | ||
|
|
||
| function removeUrlDetails(value: unknown): string | undefined { | ||
| return typeof value === 'string' && value.length > 0 ? value.split(/[?#]/, 1)[0] : undefined; |
There was a problem hiding this comment.
When an exception message, stack, filename, or resource load URL contains URL userinfo such as https://user:token@host/path, the sanitizers eventually rely on removeUrlDetails(), but this only splits on query strings and fragments. Fresh evidence beyond the prior query/fragment comments is that credentials before the host are outside [?#], so user:token@ is still copied into eventPayload.error.* and submitted; please parse/redact URL userinfo before truncating or storing these fields.
Useful? React with 👍 / 👎.
7a3d9cf to
ba7c533
Compare
|
| GitGuardian id | GitGuardian status | Secret | Commit | Filename | |
|---|---|---|---|---|---|
| - | - | Basic Auth String | ee209bf | packages/@webex/internal-plugin-metrics/test/unit/spec/unhandled-exception-telemetry.ts | View secret |
| - | - | Basic Auth String | ee209bf | packages/@webex/internal-plugin-metrics/test/unit/spec/unhandled-exception-telemetry.ts | View secret |
| - | - | Basic Auth String | ee209bf | packages/@webex/internal-plugin-metrics/test/unit/spec/unhandled-exception-telemetry.ts | View secret |
| - | - | Basic Auth String | ee209bf | packages/@webex/internal-plugin-metrics/test/unit/spec/unhandled-exception-telemetry.ts | View secret |
| - | - | Basic Auth String | ee209bf | packages/@webex/internal-plugin-metrics/test/unit/spec/unhandled-exception-telemetry.ts | View secret |
🛠 Guidelines to remediate hardcoded secrets
- Understand the implications of revoking this secret by investigating where it is used in your code.
- Replace and store your secrets safely. Learn here the best practices.
- Revoke and rotate these secrets.
- If possible, rewrite git history. Rewriting git history is not a trivial act. You might completely break other contributing developers' workflow and you risk accidentally deleting legitimate data.
To avoid such incidents in the future consider
- following these best practices for managing and storing secrets including API keys and other credentials
- install secret detection on pre-commit to catch secret before it leaves your machine and ease remediation.
🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ba7c53350c
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| }; | ||
|
|
||
| this.addMetadata(event); | ||
| this.pendingEvents.set(fingerprint, event); |
There was a problem hiding this comment.
When an enabled application produces many distinct errors within one second, every fingerprint is inserted into this map, so the deduplication logic provides no bound on memory or submissions. The subsequent flush submits every entry, and the client-metrics batcher sends at most 50 metrics per request, turning an application error cascade into a potentially large burst of telemetry requests; enforce a pending-event or per-minute limit before queuing new fingerprints.
Useful? React with 👍 / 👎.
|
|
||
| this.capture({ | ||
| column: typeof event?.colno === 'number' ? event.colno : undefined, | ||
| filename: removeUrlDetails(event?.filename), |
There was a problem hiding this comment.
When an uncaught error originates from a data: script or an unusually long source URL, ErrorEvent.filename can contain the entire inline script or an arbitrarily long path. Unlike resource URLs, this field is only stripped at ? or #, so inline source, credentials, or oversized content before those delimiters is copied into eventPayload.error.filename; apply the same scheme filtering and length cap used for resource URLs.
Useful? React with 👍 / 👎.
| } | ||
|
|
||
| private captureError(event: any): void { | ||
| const rawResourceUrl = event?.target?.src ?? event?.target?.href; |
There was a problem hiding this comment.
When an IMG loads a candidate selected from srcset, target.currentSrc identifies the resource that failed while target.src is only the fallback URL and may be empty. This lookup therefore reports the wrong resource URL, or falls through and records an Unknown uncaught error when no fallback src exists; inspect currentSrc before src so responsive-image failures are classified accurately.
Useful? React with 👍 / 👎.
Only in unit tests... |
ba7c533 to
46d79c4
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 46d79c44bf
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| error.message, | ||
| error.stack, | ||
| error.filename, | ||
| error.resourceUrl, |
There was a problem hiding this comment.
When two errors have the same name/message/filename and no stack but originate at different lines or columns within the one-second window, this fingerprint treats them as the same failure even though line and column were captured. The second exception is therefore discarded into the first event's occurrenceCount, and telemetry reports only the first source location; include the available source coordinates in the fingerprint so distinct stackless browser errors remain separate.
Useful? React with 👍 / 👎.
46d79c4 to
c21261c
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c21261cadb
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| this.capture({ | ||
| kind: 'unhandledrejection', | ||
| message: typeof reason?.message === 'string' ? reason.message : stringifyReason(reason), | ||
| name: typeof reason?.name === 'string' ? reason.name : 'UnhandledRejection', |
There was a problem hiding this comment.
When a rejection reason provides a string name, such as {name: 'GET https://host/path?token=secret', message: 'failed'}, this value bypasses the message/stack URL sanitizer and all length limits. It is then submitted in both eventPayload.error.name and the error_name metric field, potentially exposing URL credentials or producing an oversized metric; sanitize and truncate exception names before capture.
Useful? React with 👍 / 👎.
c21261c to
b736eef
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b736eefb78
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| // Examples: https://host/api?token=x -> https://host/api, ../app.js#fragment -> ../app.js, | ||
| // api/messages?token=x -> api/messages, and GET app.js?token=x -> GET app.js. | ||
| return value | ||
| ?.replace(/\b[a-z][a-z0-9+.-]*:\/\/[^\s)]+/gi, (url) => removeUrlDetails(url) ?? url) |
There was a problem hiding this comment.
When an error message or stack contains a non-hierarchical URL such as data:text/plain,inline-secret or blob:https://example.test/identifier, this matcher ignores it because it requires ://, while the relative-URL matchers do not cover schemes. The complete value is consequently copied into eventPayload.error.message or stack and submitted (up to the truncation limit), so apply the same non-network-scheme filtering or redaction used for resource URLs.
Useful? React with 👍 / 👎.
b736eef to
c578c12
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c578c1236c
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| // https://host/api, api/messages?token=x -> api/messages, and GET app.js?token=x -> GET app.js. | ||
| return value | ||
| ?.replace( | ||
| /(^|[\s("'[{,;])(([a-z][a-z0-9+.-]*):[^\s)"'\]}]+)/gi, |
There was a problem hiding this comment.
When an exception formats a URL as an assigned value, such as url=https://host/path?token=secret or payload=data:text/plain,secret, this matcher does not run because its allowed prefix characters omit =; the later relative-URL matcher cannot match these scheme URLs either. The complete query, credentials, or non-network URL is therefore copied into the submitted message/stack, so include assignment and similar value delimiters in the absolute-URL sanitizer.
Useful? React with 👍 / 👎.
c578c12 to
a69ac9a
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a69ac9afea
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
|
||
| this.capture({ | ||
| kind: 'unhandledrejection', | ||
| message: typeof reason?.message === 'string' ? reason.message : stringifyReason(reason), |
There was a problem hiding this comment.
When a promise is rejected with a Proxy or other object whose message, name, or stack getter throws, these property reads execute in the global unhandledrejection listener before entering the guarded capture() method. The reporter therefore throws its own uncaught exception while handling the original rejection, potentially generating a secondary error event and affecting the host application; wrap rejection-detail extraction in the same containment boundary as capture/submission.
Useful? React with 👍 / 👎.
| docs/samples/webex.min.js.map | ||
| docs/samples/meetings.min.js | ||
| docs/samples/meetings.min.js.map | ||
| docs/samples/contact-center.min.js |
There was a problem hiding this comment.
question: Is this part of the current commit? If not it is best added in a separate PR
There was a problem hiding this comment.
This file kept popping out in the diff, every time I run the build. I can remove it
| overrides: this.delayedClientEventsOverrides, | ||
| }); | ||
| // @ts-ignore | ||
| startUnhandledExceptionTelemetry(this.webex); |
There was a problem hiding this comment.
issue: need a test that checks that startUnhandledExceptionTelemetry is called when ready
|
|
||
| ## Unhandled exception telemetry | ||
|
|
||
| Browser exception telemetry starts after the Webex SDK emits `ready`. It does not install a |
There was a problem hiding this comment.
thought: The SDK is designed to support both browser and node-js. It is probably worth mentioning explicitly that this unhandled exception telemetry is currently only supported in a browser environment
| }; | ||
| }; | ||
|
|
||
| function truncate(value: string | undefined, maxLength: number): string | undefined { |
There was a problem hiding this comment.
suggestion: All these utilities are better tested separately with separate unit tests. You can get easier/better coverage that way
|
|
||
| try { | ||
| Promise.resolve( | ||
| submitClientMetrics.call(this.webex.internal?.metrics, UNHANDLED_EXCEPTION_METRIC_NAME, { |
There was a problem hiding this comment.
issue: This is unable to submit metrics prior to auth because it does not pass in a preloginId. If a preloginId is supplied, submitClientMetrics calls the preloginMetrics batcher rather than the logged in version. Right now, errors thrown before sign in will be silently failing to send
| }, | ||
| eventPayload: event, | ||
| }) | ||
| ).catch(() => undefined); |
There was a problem hiding this comment.
suggestion: I don't think that catching and doing nothing about errors when failing to send it necessarily the best idea. Perhaps a log message at least? We do want to know if/when this system starts failing
| const MAX_RESOURCE_URL_LENGTH = 2_048; | ||
| const MAX_STACK_LENGTH = 8_192; | ||
|
|
||
| type BrowserEventTarget = { |
There was a problem hiding this comment.
Why do you need this type? dom.d.ts has a built in EventTarget type, please use that instead.
If you must use this type, please fix it. addEventListener and removeEventListener has identical parameter list, but here the 3. param type is different.
| // Examples: data:text/plain,secret -> [redacted-url], https://host/api?token=x -> | ||
| // https://host/api, api/messages?token=x -> api/messages, and GET app.js?token=x -> GET app.js. | ||
| return value | ||
| ?.replace( |
There was a problem hiding this comment.
question: could you please add 1-2 lines of comment for each replace to se which one is handling which case.
| private readonly pendingEvents = new Map<string, UnhandledExceptionEvent>(); | ||
| private readonly eventTarget: BrowserEventTarget; | ||
| private readonly errorListener = (event: any) => this.captureError(event); | ||
| private readonly rejectionListener = (event: any) => this.captureRejection(event); |
There was a problem hiding this comment.
the event has a proper type: PromiseRejectionEvent
| class UnhandledExceptionTelemetry { | ||
| private readonly pendingEvents = new Map<string, UnhandledExceptionEvent>(); | ||
| private readonly eventTarget: BrowserEventTarget; | ||
| private readonly errorListener = (event: any) => this.captureError(event); |
| this.webex = webex; | ||
| this.eventTarget = eventTarget; | ||
| eventTarget.addEventListener('error', this.errorListener, true); | ||
| eventTarget.addEventListener('unhandledrejection', this.rejectionListener); |
There was a problem hiding this comment.
for better consistency you should use the capture phase for unhandledrejection as well. Do not forgot to update the removeEventListener as well
| */ | ||
| class UnhandledExceptionTelemetry { | ||
| private readonly pendingEvents = new Map<string, UnhandledExceptionEvent>(); | ||
| private readonly eventTarget: BrowserEventTarget; |
There was a problem hiding this comment.
error and unhandledrejection emitted only from the global object, so you can do this instead:
| private readonly eventTarget: BrowserEventTarget; | |
| private readonly eventTarget: Window | Worker; |
| this.pendingEvents.clear(); | ||
| } | ||
|
|
||
| private captureError(event: any): void { |
There was a problem hiding this comment.
| private captureError(event: any): void { | |
| private captureError(event: ErrorEvent): void { |
| column: typeof event?.colno === 'number' ? event.colno : undefined, | ||
| filename: removeUrlDetails(event?.filename), | ||
| kind: 'error', | ||
| line: typeof event?.lineno === 'number' ? event.lineno : undefined, |
There was a problem hiding this comment.
is typeof necessary here? when lineno is not number then it must be `undefined. so this should do it:
| line: typeof event?.lineno === 'number' ? event.lineno : undefined, | |
| line: event?.lineno, |
same for stack
| const now = new Date().getTime(); | ||
| let nextFlushInMs: number | undefined; | ||
|
|
||
| this.flushTimer = undefined; |
There was a problem hiding this comment.
issue: please clear the timeout before drop the timer
a69ac9a to
ee209bf
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ee209bf0e7
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| name: truncate(removeUrlDetailsFromText(details.name), MAX_ERROR_NAME_LENGTH) ?? 'Error', | ||
| message: truncate(removeUrlDetailsFromText(details.message), MAX_ERROR_MESSAGE_LENGTH), | ||
| stack: truncate(removeUrlDetailsFromText(details.stack), MAX_STACK_LENGTH), |
There was a problem hiding this comment.
When an uncaught error supplies a very large name, message, or stack, these expressions run four global regex replacements over the complete strings and allocate replacement copies before applying the advertised 256/4096/8192-character limits. A large server response embedded in an Error message can therefore stall the global error handler or cause substantial transient memory usage even though almost all of the input is discarded; bound the amount of text processed by the sanitizer as well as the submitted output.
Useful? React with 👍 / 👎.
| }, | ||
| eventPayload: event, | ||
| }, | ||
| this.webex.canAuthorize === true ? undefined : this.preLoginId |
There was a problem hiding this comment.
When an anonymous exception flushes while another submitClientMetrics(..., appPreLoginId) call is waiting in the metrics batcher, passing this reporter-specific UUID changes the identifier for the shared clientMetricsPreloginBatcher. prelogin-metrics-batcher.ts stores only one mutable preLoginId and applies it to the entire batched request, so metrics queued under the application's identifier can be sent under this unrelated UUID (or telemetry can be sent under a later caller's ID); use the application's stable pre-login identity or isolate batches by identifier.
Useful? React with 👍 / 👎.
3 participants
COMPLETES #SPARK-831221
This pull request addresses
Add the capacity to monitor exceptions
by making the following changes
< DESCRIBE YOUR CHANGES >
Change Type
The following scenarios were tested
Gather exceptions before sdk ready.
Pump and send exceptions once sdk ready
Send exceptions if sdk ready
The GAI Coding Policy And Copyright Annotation Best Practices
I certified that
Make sure to have followed the contributing guidelines before submitting.