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 e864056 |
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
Scenario snapshots, credential transitions, and fail-open error handling contain unresolved correctness issues.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 88/103 changed files
- Comments generated: 5
- Review effort level: Balanced
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Settled scenario metrics, error redaction, RUM failure state, and a public RUM callback contract remain incorrect.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (3)
Previously missed (2) — in code that hasn't changed since the last review.
projects/cps-telemetry/src/lib/scenario/cps-scenario/cps-scenario.ts:596
- An aggregate left open when the scenario settles keeps using the current
cpsNow()on every latertoRecord()call. The supposedly settled record therefore continues to gain duration and is no longer a stable snapshot. Clamp an open aggregate to the scenario's settlement time onceisSettledis true.
projects/cps-telemetry/rum/src/lib/cps-rum-credentials/cps-rum-credentials.ts:164 - The public callback type omits the credentials and compression-strategy arguments that the adjacent documentation says the SDK supplies. Consumers therefore get no contextual types for half of this advanced API and must invent or cast those parameters, despite this interface claiming to mirror the SDK field-for-field. Mirror the complete callback signature in the hand-written type.
projects/cps-telemetry/rum/src/lib/cps-rum-telemetry.sink/cps-rum-telemetry.sink.ts:178
- The failure message says monitoring is disabled, but this path never sets
disabled. SinceinitPromiseis already resolved and cannot retry, every subsequent event is instead buffered indefinitely (up to the cap), and unload reports the misleading “before RUM finished initializing” warning. Transition to the same disabled state as a declined bootstrap and clear queued items.
- Files reviewed: 88/103 changed files
- Comments generated: 1
- Review effort level: Balanced
Contributor
There was a problem hiding this comment.
🔵 Needs a closer look
AWS RUM remains active after revocation or teardown, and route telemetry can expose matrix-parameter values.
Review details
Suppressed comments (4)
Previously missed (3) — in code that hasn't changed since the last review.
projects/composition/src/app/services/app-telemetry.service.ts:195
- Static Angular routes can still carry matrix parameters (for example,
/button;customerId=123). This only strips query strings and fragments, so those resolved values become theroutemetric dimension and can leak identifiers or create unbounded cardinality. Strip each segment's matrix parameters as well.
projects/cps-telemetry/rum/src/lib/cps-rum-telemetry.sink/cps-rum-telemetry.sink.ts:332 - Destroying the Angular sink only clears the credential timer; the AWS RUM instance continues running its event listeners, plugins, cache, and dispatch timer. This can keep collecting after teardown and duplicate telemetry if the app is bootstrapped again in the same page. Disable the client during destruction.
projects/cps-telemetry/rum/src/lib/cps-rum-credentials/cps-rum-credentials.ts:159 - This description is incorrect for
aws-rum-web3.2.1: credentials supplied throughsetAwsCredentialsare used to sign requests even without anidentityPoolId. Telling consumers this option has no effect can lead them to disable signing and receive 403 responses. Document that it should remain enabled for broker-supplied credentials, and is disabled only when another transport/proxy handles signing.
projects/cps-telemetry/rum/src/lib/cps-rum-telemetry.sink/cps-rum-telemetry.sink.ts:488
- Dropping this reference does not disable the already-created AWS RUM client: its automatic plugins and dispatch timer remain active, so a provider returning
nulldoes not actually revoke monitoring as promised. Call the SDK'sdisable()method before clearing the reference.
- Files reviewed: 88/103 changed files
- Comments generated: 0 new
- 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
Comment on lines
+122
to
+127
| const key = JSON.stringify([ | ||
| eventName, | ||
| detail?.scenarioId ?? '', | ||
| detail?.eventType ?? '', | ||
| detail?.feature ?? '', | ||
| this.metadataKey(metadata) |
Comment on lines
+133
to
+136
| try { | ||
| locks | ||
| .request( | ||
| `cps-telemetry-host:${channelName}`, |
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.