feat(internal-plugin-metrics): allow for delayed submission of client events #4121
Conversation
📝 WalkthroughWalkthroughThe changes add support for delaying the submission of client events within the metrics system. In the Possibly related PRs
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 ESLint
yarn install v1.22.22 (For a CapTP with native promises, see @endo/eventual-send and @endo/captp) ✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (8)
packages/@webex/internal-plugin-metrics/src/metrics.types.ts (1)
323-328: Add a descriptive doc comment for this interface.Adding a JSDoc or a short documentation snippet for
DelayedClientEventmight help maintain clarity and context around its usage.packages/@webex/internal-plugin-metrics/test/unit/spec/new-metrics.ts (2)
263-276: Thorough test for toggling delayed submission.This test confirms that setting
delaySubmitClientEventsbehaves as expected. Consider including an additional test for quickly toggling the setting multiple times in succession.
278-286: Properly verifying the submitDelayedClientEvents invocation.Ensures that the method is called once, matching expectations. You could expand the test to assert the promise resolution or returned values if needed.
packages/@webex/internal-plugin-metrics/src/new-metrics.ts (2)
48-52: Consider clarifying uses for delaySubmitClientEvents.The doc comment is concise. Optionally, expand on scenarios (e.g., preload) where deferring event submissions offers tangible benefits.
402-417: Handle or log potential errors during bulk submission.
submitDelayedClientEvents()returns a promise that could fail if any delayed event submissions fail. Consider catching and logging partial errors for clarity.packages/@webex/internal-plugin-metrics/src/call-diagnostic/call-diagnostic-metrics.ts (3)
863-863: Doc comment additions are helpful.Providing a brief example of how
delaySubmitEventmight be used could further improve clarity for future maintainers.
877-891: Buffering events instead of sending immediately.By pushing to
delayedClientEventsbefore returning, you ensure minimal overhead for delayed submissions. For critical events, consider implementing fallback or logging if the queue becomes large.
909-929: Bulk submission logic seems solid.
submitDelayedClientEvents()correctly iterates over the stored events. If partial failures matter, consider capturing individual errors fromPromise.all().
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
packages/@webex/internal-plugin-metrics/src/call-diagnostic/call-diagnostic-metrics.ts(6 hunks)packages/@webex/internal-plugin-metrics/src/metrics.types.ts(1 hunks)packages/@webex/internal-plugin-metrics/src/new-metrics.ts(3 hunks)packages/@webex/internal-plugin-metrics/test/unit/spec/call-diagnostic/call-diagnostic-metrics.ts(21 hunks)packages/@webex/internal-plugin-metrics/test/unit/spec/new-metrics.ts(3 hunks)
🔇 Additional comments (9)
packages/@webex/internal-plugin-metrics/test/unit/spec/new-metrics.ts (2)
62-62: Test stub initialization looks good.Stubbing
submitDelayedClientEventsensures the invocation can be easily tracked without making actual calls.
124-124: Explicitly setting the delaySubmitEvent property is good practice.Declaring
delaySubmitEvent: falseclarifies the default immediate-submission scenario for readability and maintainability.packages/@webex/internal-plugin-metrics/src/new-metrics.ts (1)
298-303: Properly routing the delaySubmitEvent flag.Passing the
delaySubmitEventproperty ensures consistent handling incallDiagnosticMetrics. This is a clean integration point.packages/@webex/internal-plugin-metrics/src/call-diagnostic/call-diagnostic-metrics.ts (1)
99-99: Initializing an array for delayed events looks good.Storing delayed events in-memory is straightforward. Double-check concurrency scenarios if multiple calls might happen in parallel.
packages/@webex/internal-plugin-metrics/test/unit/spec/call-diagnostic/call-diagnostic-metrics.ts (5)
16-17: LGTM - New imports for testingThese imports are appropriate for enhanced test functionality.
142-142: Good practice to reset global stateProperly resetting
global.window.webClientPreloadafter each test prevents test state from leaking between test cases.
848-848: LGTM - Added webClientPreload property to event payloadsThe new
webClientPreloadproperty is consistently added to all relevant event payloads, which aligns with the PR objective of supporting the preload feature.Also applies to: 874-874, 911-911, 985-985, 1012-1012, 1050-1050, 1168-1169, 1190-1190, 1264-1265, 1292-1292, 1445-1446, 1467-1468, 1539-1539, 1619-1619, 1691-1691, 1765-1765, 1848-1848
1403-1480: Good test coverage for webClientPreload functionalityThis test case properly verifies that the
webClientPreloadproperty is included in event payloads when the feature is enabled through the global window object.
3053-3126: Well-structured tests for delayed event submissionThese tests thoroughly validate the new
submitDelayedClientEventsfunctionality:
- Verifies no action is taken when there are no delayed events
- Confirms each delayed event is properly submitted with its original timestamp
- Ensures the delayed events array is cleared after submission
The implementation follows good practices by preserving the original triggered time of each event.
|
This pull request is automatically being deployed by Amplify Hosting (learn more). |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
packages/@webex/internal-plugin-metrics/src/new-metrics.ts (2)
48-52: Consider adding an explicit type annotation.Although this property is clearly assigned a boolean, adding
: booleanexplicitly can improve clarity and help avoid unintentional type changes in the future.
402-417: Consider robust error handling on delayed submissions.When
shouldDelaytransitions from true to false,submitDelayedClientEvents()is invoked. If submissions fail, it might be beneficial to catch and handle errors (e.g., retry or log).packages/@webex/internal-plugin-metrics/src/call-diagnostic/call-diagnostic-metrics.ts (1)
911-931: Consider handling async outcomes when clearing delayed events.After calling
submitClientEventfor each delayed event, failures, if any, may silently skip logging or future retries. It might be worthwhile to handle promise rejections to avoid losing critical diagnostics.packages/@webex/internal-plugin-metrics/test/unit/spec/call-diagnostic/call-diagnostic-metrics.ts (2)
16-17: Remove unused import if not required.It appears
globis not actually referenced. You can safely remove it to keep the test file tidy.-import { glob } from 'glob'; import { expect } from 'chai';
3050-3123: Add negative test cases.The new tests confirm that delayed events are submitted correctly. You might consider a scenario where submissions fail to ensure graceful handling of rejections or retries.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/@webex/internal-plugin-metrics/src/call-diagnostic/call-diagnostic-metrics.ts(6 hunks)packages/@webex/internal-plugin-metrics/src/new-metrics.ts(3 hunks)packages/@webex/internal-plugin-metrics/test/unit/spec/call-diagnostic/call-diagnostic-metrics.ts(20 hunks)packages/@webex/internal-plugin-metrics/test/unit/spec/new-metrics.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/@webex/internal-plugin-metrics/test/unit/spec/new-metrics.ts
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build Packages
- GitHub Check: AWS Amplify Console Web Preview
🔇 Additional comments (5)
packages/@webex/internal-plugin-metrics/src/new-metrics.ts (1)
298-303: Looks good and straightforward.Passing
delaySubmitEvent: this.delaySubmitClientEventscleanly integrates the delay submission logic without altering existing function signatures.packages/@webex/internal-plugin-metrics/src/call-diagnostic/call-diagnostic-metrics.ts (3)
43-44: Import statement confirmed.Importing
DelayedClientEventfrommetrics.typesis aligned with the new delayed submission feature.
99-99: Good use of a typed array.Storing delayed events in
delayedClientEventsis a straightforward approach.
865-893: Documentation and parameter usage look solid.The
delaySubmitEventparameter and corresponding doc block are consistent. Preserving a timestamp before deferring the event submission is a neat solution for accurate event chronologies.packages/@webex/internal-plugin-metrics/test/unit/spec/call-diagnostic/call-diagnostic-metrics.ts (1)
1402-1477: Test coverage forwebClientPreloadlooks thorough.The new test effectively verifies payload fields and ensures the property is included in event submissions.
… events
COMPLETES #SPARK-623039
https://jira-eng-gpk2.cisco.com/jira/browse/SPARK-623039
As part of the preload feature on web client we need to delay sending all client events until the user decides to join via web. So, we need a mechanism to delay these events in the sdk.
Change Type
The following scenarios were tested
< ENUMERATE TESTS PERFORMED, WHETHER MANUAL OR AUTOMATED >
I certified that
I have read and followed contributing guidelines
I discussed changes with code owners prior to submitting this pull request
I have not skipped any automated checks
All existing and new tests passed
I have updated the documentation accordingly
Make sure to have followed the contributing guidelines before submitting.