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
The reason will be displayed to describe this comment to others. Learn more.
🔵 Needs a closer look
The broad telemetry, credential, routing, and cross-realm changes contain unresolved correctness and concurrency issues.
Pull request overview
Introduces a reusable Angular telemetry library and integrates it into the composition application for logging, scenarios, BI events, broadcast forwarding, and AWS RUM.
Changes:
Adds telemetry APIs, sinks, configuration, safety/redaction utilities, and tests.
Instruments composition navigation, demos, searches, uploads, themes, and diagnostics.
Adds routing, build, test, and CI support for telemetry.
The reason will be displayed to describe this comment to others. Learn more.
🔵 Needs a closer look
Successful upload scenarios can incorrectly remain active until timeout, alongside unresolved API and telemetry classification issues.
Review details
Suppressed comments (5)
Previously missed (5) — in code that hasn't changed since the last review.
projects/composition/src/app/app.module.ts:39
This labels telemetry from every build and deployment as production. The workspace has no environment replacement/config source, so local development or a non-production deployment that can reach /rum/init will contaminate production dimensions. Supply the deployment environment from runtime or build configuration instead of hard-coding it. projects/composition/src/app/pages/file-upload-page/file-upload-page.component.ts:94
The upload component wraps this callback in take(1) (cps-file-upload.component.ts:256). After the first value reaches that operator, it unsubscribes upstream before the source's complete notification reaches traceScenario, so a successful upload never calls scenario.complete; the map entry is then deleted and the scenario remains active until timeout. Settle the scenario in the successful next path before returning the value (and fail it in the error path) rather than relying on source completion here. projects/cps-telemetry/src/lib/services/cps-bi-telemetry.service/cps-bi-telemetry.service.ts:145
Updating an existing Map key does not refresh its insertion order. Consequently, a key re-emitted after its dedup window can have a fresh timestamp but still be selected as oldestKey at capacity; deleting it allows an immediate duplicate through. Refresh the insertion order whenever a non-duplicate key is recorded. projects/cps-telemetry/src/lib/sinks/cps-rum/cps-rum-credentials/cps-rum-credentials.ts:163
This callback type has the same strict-function-variance problem avoided by fetchFunction: a real SDK ClientBuilder with specific parameter types is not assignable to a function claiming it accepts arbitrary unknown arguments. Consumers therefore cannot use the documented escape hatch without a cast. Mirror the SDK's callable signature with local structural types (to keep the peer optional), or expose a type that accepts a typed function safely. projects/cps-telemetry/src/lib/sinks/cps-telemetry/cps-noop-telemetry.sink/cps-noop-telemetry.sink.ts:16
The concrete exported class narrows record, recordError, setUserId, and flush to zero-argument methods. Although TypeScript permits these implementations as overrides, consumers typed as CpsNoopTelemetrySink cannot call the normal sink API (for example, new CpsNoopTelemetrySink().record(type, payload) fails type checking). Preserve the abstract method parameters on each override.
If anchor.click() throws, anchor.remove() is skipped and the temporary download link remains in document.body; the existing failure-path test only checks URL revocation. Create the anchor before the try and remove it in finally together with the object URL cleanup. projects/cps-telemetry/src/lib/scenario/cps-scenario-operators/cps-scenario-operators.ts:74
Unsubscription never reaches either callback here. Common cancellation paths such as switchMap, takeUntilDestroyed, or a manual unsubscribe therefore leave the scenario active until its timeout, recording a cancellation as a timeout. Add teardown handling (for example, the tap observer's unsubscribe hook) that cancels an otherwise-unsettled scenario, and cover that path with a test. projects/cps-telemetry/src/lib/services/cps-bi-telemetry.service/cps-bi-telemetry.service.ts:122
The delimiter-based key is ambiguous: for example, eventType: 'x|y', feature: 'z' produces the same key as eventType: 'x', feature: 'y|z'. Those are different emitted events, but the second is incorrectly dropped during the dedup window. Encode the fields structurally (for example, as a JSON array) instead of concatenating them.
Lexicographic comparison is only chronological when both timestamps use the same canonical offset. CpsLogQuery accepts general ISO-8601 bounds, so a valid offset such as 2024-01-02T01:00:00+02:00 incorrectly excludes a record at 2024-01-02T00:00:00Z, even though the record is later. Parse the bounds and record timestamps before comparing them.
NavigationSkipped is a terminal router outcome (notably for same-URL navigation), but this handler ignores it. A sidebar click can therefore leave navigationIntentAt pending and incorrectly backdate the next unrelated navigation within two seconds; handle this event by clearing the pending intent (and any scenario for its id if one exists).
CodeExampleComponent now injects AppTelemetryService, which injects Router. This test module doesn't provide a router, so TestBed.createComponent(CodeExampleComponent) will fail with a DI error. Add provideRouter([]) (or stub AppTelemetryService) so the component can be instantiated in unit tests.
Revoking the object URL synchronously after click() can invalidate it before some browsers begin consuming the download. Defer revocation until the next task, as cpsDownloadJson() already does, so the log export works reliably.
Normalize undefined JSON.stringify results to satisfy string contract
JSON.stringify can return undefined for a top-level undefined, function, or symbol, so this function does not always satisfy its declared string return contract. Normalize that result here rather than relying on individual callers to compensate.
This group-level Enter handler also receives Enter presses used to open or select options in the nested autocomplete and select controls. If the other fields already make canAdd() true, keyboard interaction with those controls can submit a filter unintentionally. Remove the group handler and limit Enter-to-add to the value input or the Add button.
Reschedule cleanup fallback timers exceeding maximum timeout
markCleanupFallbackMs is not honored above 2,147,483,647 ms. CpsScenario.scheduleMarkCleanupFallback() clamps the one timer with Math.min(fallbackMs, MAX_TIMEOUT_MS) but never reschedules, so a longer configured fallback fires early. Reschedule the remaining duration as scheduleTimeout() does, or explicitly reject values above the supported maximum.
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.
No description provided.