Conversation
fateeand
changed the title
Contributor
Coverage report for library
Test suite run success3233 tests passing in 76 suites. Report generated by 🧪jest coverage report action from 2e203f4 |
Contributor
There was a problem hiding this comment.
🔵 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.
File summaries
| File | Description |
|---|---|
tsconfig.json |
Registers telemetry paths and project references. |
projects/cps-telemetry/tsconfig.spec.json |
Configures telemetry tests. |
projects/cps-telemetry/tsconfig.lib.prod.json |
Configures production compilation. |
projects/cps-telemetry/tsconfig.lib.json |
Configures library compilation. |
projects/cps-telemetry/src/public-api.ts |
Exposes the public telemetry API. |
projects/cps-telemetry/src/lib/utils/cps-user-timings.util/cps-user-timings.util.ts |
Adds browser timing helpers. |
projects/cps-telemetry/src/lib/utils/cps-user-timings.util/cps-user-timings.util.spec.ts |
Tests timing helpers. |
projects/cps-telemetry/src/lib/utils/cps-telemetry-safe.util/cps-telemetry-safe.util.ts |
Adds fail-open utilities and identifiers. |
projects/cps-telemetry/src/lib/utils/cps-telemetry-safe.util/cps-telemetry-safe.util.spec.ts |
Tests safety utilities. |
projects/cps-telemetry/src/lib/utils/cps-debug-flag.util/cps-debug-flag.util.ts |
Adds runtime debug flags. |
projects/cps-telemetry/src/lib/utils/cps-debug-flag.util/cps-debug-flag.util.spec.ts |
Tests debug flags. |
projects/cps-telemetry/src/lib/sinks/cps-telemetry/cps-telemetry-abstract.sink/cps-telemetry-abstract.sink.ts |
Defines the sink contract. |
projects/cps-telemetry/src/lib/sinks/cps-telemetry/cps-noop-telemetry.sink/cps-noop-telemetry.sink.ts |
Adds a no-op sink. |
projects/cps-telemetry/src/lib/sinks/cps-telemetry/cps-noop-telemetry.sink/cps-noop-telemetry.sink.spec.ts |
Tests the no-op sink. |
projects/cps-telemetry/src/lib/sinks/cps-rum/cps-rum-credentials/cps-rum-credentials.ts |
Defines RUM configuration and credentials. |
projects/cps-telemetry/src/lib/sinks/cps-broadcast/cps-broadcast.messages.ts |
Defines broadcast protocol and validation. |
projects/cps-telemetry/src/lib/sinks/cps-broadcast/cps-broadcast-telemetry.sink.ts |
Adds the follower broadcast sink. |
projects/cps-telemetry/src/lib/sinks/cps-broadcast/cps-broadcast-host.service.ts |
Adds the broadcast host. |
projects/cps-telemetry/src/lib/services/cps-scenario-telemetry.service/cps-scenario-telemetry.service.ts |
Manages scenario lifecycles. |
projects/cps-telemetry/src/lib/services/cps-logger.service/cps-logger.service.ts |
Adds structured logging. |
projects/cps-telemetry/src/lib/services/cps-bi-telemetry.service/cps-bi-telemetry.service.ts |
Adds BI event tracking and deduplication. |
projects/cps-telemetry/src/lib/scenario/cps-scenario/cps-scenario.spec.ts |
Tests scenario behavior. |
projects/cps-telemetry/src/lib/scenario/cps-scenario-operators/cps-scenario-operators.ts |
Adds an RxJS scenario operator. |
projects/cps-telemetry/src/lib/scenario/cps-scenario-operators/cps-scenario-operators.spec.ts |
Tests the scenario operator. |
projects/cps-telemetry/src/lib/providers/cps-telemetry-common.providers/cps-telemetry-common.providers.ts |
Adds telemetry provider functions. |
projects/cps-telemetry/src/lib/providers/cps-log-api.provider/cps-log-api.provider.ts |
Defines the logging backend contract. |
projects/cps-telemetry/src/lib/models/cps-telemetry-common.models/cps-telemetry-common.models.ts |
Defines shared telemetry models. |
projects/cps-telemetry/src/lib/models/cps-telemetry-common.models/cps-telemetry-common.models.spec.ts |
Tests event-type generation. |
projects/cps-telemetry/src/lib/models/cps-scenario.models/cps-scenario.models.ts |
Defines scenario models. |
projects/cps-telemetry/src/lib/models/cps-log.models/cps-log.models.ts |
Defines logging models. |
projects/cps-telemetry/src/lib/models/cps-bi.models/cps-bi.models.ts |
Defines BI event models. |
projects/cps-telemetry/src/lib/config/cps-telemetry-common.config/cps-telemetry-common.config.ts |
Defines shared telemetry configuration. |
projects/cps-telemetry/src/lib/config/cps-telemetry-common.config/cps-telemetry-common.config.spec.ts |
Tests shared defaults. |
projects/cps-telemetry/src/lib/config/cps-scenario.config/cps-scenario.config.ts |
Defines scenario configuration. |
projects/cps-telemetry/src/lib/config/cps-scenario.config/cps-scenario.config.spec.ts |
Tests scenario defaults. |
projects/cps-telemetry/src/lib/config/cps-log.config/cps-log.config.ts |
Defines logging configuration. |
projects/cps-telemetry/src/lib/config/cps-log.config/cps-log.config.spec.ts |
Tests logging defaults. |
projects/cps-telemetry/src/lib/config/cps-bi.config/cps-bi.config.ts |
Defines BI configuration. |
projects/cps-telemetry/src/lib/config/cps-bi.config/cps-bi.config.spec.ts |
Tests BI defaults. |
projects/cps-telemetry/package.json |
Declares library metadata and peers. |
projects/cps-telemetry/ng-package.json |
Configures telemetry packaging. |
projects/composition/tsconfig.app.json |
Enables compilation with RUM dependencies. |
projects/composition/src/app/services/telemetry.schema.ts |
Declares telemetry vocabularies. |
projects/composition/src/app/services/rum-credentials.provider.ts |
Loads RUM bootstrap credentials. |
projects/composition/src/app/services/rum-credentials.provider.spec.ts |
Tests credential loading. |
projects/composition/src/app/services/app-telemetry.service.ts |
Instruments navigation and interactions. |
projects/composition/src/app/services/app-telemetry.service.spec.ts |
Tests application telemetry. |
projects/composition/src/app/services/app-log-api.provider.ts |
Adds an in-memory log provider. |
projects/composition/src/app/services/app-log-api.provider.spec.ts |
Tests log storage and querying. |
projects/composition/src/app/pages/table-page/table-page.component.ts |
Instruments lazy table loading. |
projects/composition/src/app/pages/table-page/table-page.component.spec.ts |
Tests table scenarios. |
projects/composition/src/app/pages/file-upload-page/file-upload-page.component.ts |
Instruments file processing. |
projects/composition/src/app/pages/file-upload-page/file-upload-page.component.spec.ts |
Tests upload scenarios. |
projects/composition/src/app/pages/autocomplete-page/autocomplete-page.component.ts |
Instruments search and validation. |
projects/composition/src/app/pages/autocomplete-page/autocomplete-page.component.spec.ts |
Tests autocomplete scenarios. |
projects/composition/src/app/guards/default-tab-redirect.guard.ts |
Redirects bare component routes. |
projects/composition/src/app/guards/default-tab-redirect.guard.spec.ts |
Tests tab redirects. |
projects/composition/src/app/components/viewer/viewer.component.ts |
Removes component-level redirecting. |
projects/composition/src/app/components/viewer/viewer.component.spec.ts |
Updates viewer redirect expectations. |
projects/composition/src/app/components/theme-toggle/theme-toggle.component.ts |
Tracks theme changes. |
projects/composition/src/app/components/theme-toggle/theme-toggle.component.spec.ts |
Tests theme telemetry. |
projects/composition/src/app/components/navigation-sidebar/navigation-sidebar.component.ts |
Tracks debounced searches. |
projects/composition/src/app/components/navigation-sidebar/navigation-sidebar.component.spec.ts |
Tests search telemetry cleanup. |
projects/composition/src/app/components/code-example/code-example.component.ts |
Adds logging and copy telemetry. |
projects/composition/src/app/components/code-example/code-example.component.spec.ts |
Tests code-example diagnostics. |
projects/composition/src/app/app.prefix-title-strategy.ts |
Logs missing route titles. |
projects/composition/src/app/app.prefix-title-strategy.spec.ts |
Tests title diagnostics. |
projects/composition/src/app/app.module.ts |
Registers telemetry and RUM providers. |
projects/composition/src/app/app.component.ts |
Starts application telemetry. |
projects/composition/src/app/app.component.spec.ts |
Adds telemetry test providers. |
projects/composition/src/app/app-routing.module.ts |
Applies default-tab guards. |
package.json |
Adds telemetry scripts and RUM dependency. |
jest.config.js |
Maps the telemetry package in tests. |
angular.json |
Registers telemetry build and test targets. |
.gitignore |
Ignores generated telemetry traces. |
.github/workflows/cps-shared-ui-checkers.yml |
Builds and tests telemetry in CI. |
Review details
- Files reviewed: 83/91 changed files
- Comments generated: 16
- Review effort level: Balanced
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
Contributor
Playwright test results
Details
|
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Broadcast leadership, cancellation handling, credential caching, and telemetry delivery have unresolved correctness and security issues.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 84/92 changed files
- Comments generated: 6
- Review effort level: Balanced
Contributor
There was a problem hiding this comment.
🔵 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/initwill 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'scompletenotification reachestraceScenario, so a successful upload never callsscenario.complete; the map entry is then deleted and the scenario remains active until timeout. Settle the scenario in the successfulnextpath 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
Mapkey does not refresh its insertion order. Consequently, a key re-emitted after its dedup window can have a fresh timestamp but still be selected asoldestKeyat 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 SDKClientBuilderwith specific parameter types is not assignable to a function claiming it accepts arbitraryunknownarguments. 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, andflushto zero-argument methods. Although TypeScript permits these implementations as overrides, consumers typed asCpsNoopTelemetrySinkcannot call the normal sink API (for example,new CpsNoopTelemetrySink().record(type, payload)fails type checking). Preserve the abstract method parameters on each override.
- Files reviewed: 83/92 changed files
- Comments generated: 0 new
- Review effort level: Balanced
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Scenario cancellation, noop identity retention, BI deduplication, and download cleanup contain unresolved behavioral defects.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (3)
Previously missed (3) — in code that hasn't changed since the last review.
projects/composition/src/app/services/app-log-api.provider.ts:102
- If
anchor.click()throws,anchor.remove()is skipped and the temporary download link remains indocument.body; the existing failure-path test only checks URL revocation. Create the anchor before thetryand remove it infinallytogether 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, thetapobserver'sunsubscribehook) 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 aseventType: '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.
- Files reviewed: 83/95 changed files
- Comments generated: 1
- Review effort level: Balanced
Contributor
There was a problem hiding this comment.
🔵 Needs a closer look
The cross-cutting telemetry architecture still has unresolved packaging, test-configuration, and timestamp-filtering issues.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
projects/composition/src/app/services/app-log-api.provider.ts:54
- Lexicographic comparison is only chronological when both timestamps use the same canonical offset.
CpsLogQueryaccepts general ISO-8601 bounds, so a valid offset such as2024-01-02T01:00:00+02:00incorrectly excludes a record at2024-01-02T00:00:00Z, even though the record is later. Parse the bounds and record timestamps before comparing them.
- Files reviewed: 82/95 changed files
- Comments generated: 2
- Review effort level: Balanced
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Cross-tab attribution and redaction gaps can produce inaccurate telemetry and retain unsanitized data.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
projects/composition/src/app/services/app-telemetry.service.ts:176
- This JSDoc describes
navigationStartedAt, but therouteTemplateJSDoc immediately following it detaches it from that method. Move this block directly abovenavigationStartedAtso generated documentation and editor hovers describe the intended function.
- Files reviewed: 88/103 changed files
- Comments generated: 2
- Review effort level: Balanced
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Scenario metadata can exceed its configured bounds, and User Timing cleanup can remove unrelated performance entries.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (2)
projects/cps-telemetry/src/lib/scenario/cps-scenario/cps-scenario.ts:645
- This spread can retain up to
2 * maxKeysentries because the step's initial metadata and its closing metadata were capped separately. That violates the documented per-metadata payload bound; apply the cap after combining both bags.
projects/cps-telemetry/src/lib/scenario/cps-scenario/cps-scenario.ts:546 - Outcome metadata is capped before being assigned, but the existing scenario metadata may already contain
maxKeysentries, so this merge can exceed the configured payload limit. Reapply the bound to the combined root metadata rather than assigning the independently capped bag.
- Files reviewed: 87/103 changed files
- Comments generated: 1
- Review effort level: Balanced
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Broadcast validation and long-duration timeout handling are incorrect, and CI does not build the consuming application.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (2)
Previously missed (1) — in code that hasn't changed since the last review.
projects/cps-telemetry/src/lib/sinks/cps-broadcast/cps-broadcast.messages.ts:235
- This type guard accepts event messages without validating optional
metadata. A payload such as{ metadata: { nested: {} } }is therefore narrowed toCpsBroadcastMessageeven though it violatesCpsTelemetryMetadata; the host forwards it unchanged and the RUM sanitizer preserves that object value. Validate metadata as a non-array object containing only finite numbers, strings, booleans, or null (preferably through a shared helper).
This issue also appears on line 236 of the same file.
projects/cps-telemetry/src/lib/sinks/cps-broadcast/cps-broadcast.messages.ts:244
- The error branch has the same unsound narrowing for optional
metadata: malformed same-channel traffic can be accepted asCpsBroadcastMessageand forwarded to the sink with non-primitive metadata values. Apply the same runtime metadata validator used by the event branch.
- Files reviewed: 87/103 changed files
- Comments generated: 2
- Review effort level: Balanced
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Unhandled SSR access, skipped-navigation state, and an unguarded diagnostic can produce runtime failures or inaccurate telemetry.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
projects/composition/src/app/services/app-telemetry.service.ts:112
NavigationSkippedis a terminal router outcome (notably for same-URL navigation), but this handler ignores it. A sidebar click can therefore leavenavigationIntentAtpending 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).
- Files reviewed: 87/103 changed files
- Comments generated: 2
- Review effort level: Balanced
Contributor
There was a problem hiding this comment.
🔵 Needs a closer look
The updated CodeExampleComponent tests don’t provide a Router despite injecting AppTelemetryService, which should cause DI failures in unit tests.
Review details
Suppressed comments (1)
projects/composition/src/app/components/code-example/code-example.component.spec.ts:18
CodeExampleComponentnow injectsAppTelemetryService, which injectsRouter. This test module doesn't provide a router, soTestBed.createComponent(CodeExampleComponent)will fail with a DI error. AddprovideRouter([])(or stubAppTelemetryService) so the component can be instantiated in unit tests.
- Files reviewed: 87/103 changed files
- Comments generated: 0 new
- Review effort level: Lite
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.