fix: lobby entry/exit metrics #4018
Conversation
WalkthroughThe changes in this pull request enhance the Changes
Possibly related PRs
Suggested reviewers
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
warning [email protected]: This version is no longer supported. Please see https://eslint.org/version-support for other options. (For a CapTP with native promises, see @endo/eventual-send and @endo/captp) 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
CodeRabbit Configuration File (
|
|
This pull request is automatically being deployed by Amplify Hosting (learn more). |
![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
🧹 Outside diff range and nitpick comments (1)
packages/@webex/plugin-meetings/src/metrics/constants.ts (1)
74-75: LGTM! Consider documenting these metrics.The new constants follow the established naming patterns and directly address the PR objective of tracking lobby events.
Consider adding these new metrics to the documentation to help other developers understand when these events are triggered.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
packages/@webex/plugin-meetings/src/meeting/index.ts(2 hunks)packages/@webex/plugin-meetings/src/metrics/constants.ts(1 hunks)packages/@webex/plugin-meetings/test/unit/spec/meeting/index.js(2 hunks)
🔇 Additional comments (11)
packages/@webex/plugin-meetings/test/unit/spec/meeting/index.js (8)
Line range hint 1-8: LGTM!
The import statements look good and are correctly importing the required dependencies.
Line range hint 10-11: LGTM!
The describe block is correctly set up to group the tests for the plugin-meetings module.
Line range hint 13-35: LGTM!
The setup code in the before block looks good. It correctly mocks the required objects and methods.
Line range hint 37-65: LGTM!
The setup code in the beforeEach block looks good. It correctly initializes the required variables and mocks the necessary methods.
Line range hint 67-8631: LGTM!
The tests for the meeting index module look comprehensive and well-structured. They cover various scenarios and edge cases. The assertions are properly used to verify the expected behavior.
Line range hint 8668-8860: LGTM!
The remaining tests for the meeting index module look good. They cover various scenarios and edge cases. The assertions are properly used to verify the expected behavior.
8632-8639: Verify the behavioral metric is sent correctly.
Please verify that the BEHAVIORAL_METRICS.GUEST_ENTERED_LOBBY metric is sent with the correct payload when the SELF_UNADMITTED_GUEST event is emitted.
Run the following script to verify the metric:
✅ Verification successful
Let me gather more context about the event handling to verify the complete flow.
Let me check the implementation of the event handler in the source code to verify the complete flow.
Let me fix the regex and try again.
The behavioral metric is correctly implemented and verified
The implementation correctly sends the GUEST_ENTERED_LOBBY metric when the SELF_UNADMITTED_GUEST event is emitted. The verification shows:
- The metric is sent with the correct payload containing the correlation ID
- The event handling is properly set up in
setUpLocusInfoSelfListener() - The metric constant is correctly defined as
'js_sdk_guest_entered_lobby' - The test correctly asserts both the metric and its payload
The implementation matches the test's expectations and follows the correct pattern for behavioral metric tracking in the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify BEHAVIORAL_METRICS.GUEST_ENTERED_LOBBY metric is sent correctly.
# Test: Search for the metric being sent. Expect: metric sent with correct payload.
rg -A 5 $'Metrics.sendBehavioralMetric\(.*BEHAVIORAL_METRICS.GUEST_ENTERED_LOBBY'
Length of output: 654
Script:
#!/bin/bash
# Search for SELF_UNADMITTED_GUEST event handling
ast-grep --pattern 'on($_, LOCUSINFO.EVENTS.SELF_UNADMITTED_GUEST, $_)'
# Also check the test file to understand the test setup
rg -B 5 -A 10 "SELF_UNADMITTED_GUEST" "packages/@webex/plugin-meetings/test/unit/spec/meeting/index.js"
Length of output: 954
Script:
#!/bin/bash
# Search for the implementation of setUpLocusInfoSelfListener
rg -B 10 -A 20 "setUpLocusInfoSelfListener.*{" "packages/@webex/plugin-meetings/src/meeting/index.ts"
# Also check the BEHAVIORAL_METRICS constant definition
rg "GUEST_ENTERED_LOBBY.*=" --type ts
Length of output: 263
Script:
#!/bin/bash
# Search for the implementation of setUpLocusInfoSelfListener
rg -B 10 -A 20 "setUpLocusInfoSelfListener" "packages/@webex/plugin-meetings/src/meeting/index.ts"
# Search for SELF_UNADMITTED_GUEST event handling in the source
rg -B 5 -A 10 "SELF_UNADMITTED_GUEST" "packages/@webex/plugin-meetings/src/meeting/index.ts"
# Check the behavioral metrics constant
rg -B 2 -A 2 "GUEST_ENTERED_LOBBY" "packages/@webex/plugin-meetings/src/metrics/constants.ts"
Length of output: 10251
8660-8667: Verify the behavioral metric is sent correctly.
Please verify that the BEHAVIORAL_METRICS.GUEST_EXITED_LOBBY metric is sent with the correct payload when the SELF_ADMITTED_GUEST event is emitted.
Run the following script to verify the metric:
packages/@webex/plugin-meetings/src/meeting/index.ts (2)
3252-3254: LGTM! Guest lobby entry metric is properly implemented.
The behavioral metric is correctly sent when a guest enters the lobby, including the required correlation ID.
3278-3280: LGTM! Guest lobby exit metric is properly implemented.
The behavioral metric is correctly sent when a guest exits the lobby, including the required correlation ID.
packages/@webex/plugin-meetings/src/metrics/constants.ts (1)
74-75: Verify consistent metric usage across the codebase.
Let's ensure these new metrics are used consistently in the codebase.
✅ Verification successful
Let me check the actual implementation and test coverage to ensure these metrics are properly used.
The metrics are implemented and tested correctly
The new lobby metrics are properly integrated:
- Both metrics are used in
meeting/index.tsalongside corresponding client events - Implementation includes proper correlation IDs
- Unit tests verify both metrics with correct parameters
- Test coverage includes both entry and exit scenarios
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the usage of new lobby metrics across the codebase
# Search for metric usage
echo "=== Searching for lobby metric usage ==="
rg -l "GUEST_ENTERED_LOBBY|GUEST_EXITED_LOBBY|js_sdk_guest_entered_lobby|js_sdk_guest_exited_lobby"
# Search for related test coverage
echo -e "\n=== Checking test coverage ==="
rg -l "GUEST_ENTERED_LOBBY|GUEST_EXITED_LOBBY" "packages/@webex/plugin-meetings/test"
Length of output: 593
Script:
#!/bin/bash
# Check the implementation in meeting/index.ts
echo "=== Implementation in meeting/index.ts ==="
rg -A 5 -B 5 "GUEST_ENTERED_LOBBY|GUEST_EXITED_LOBBY" "packages/@webex/plugin-meetings/src/meeting/index.ts"
# Check the test coverage
echo -e "\n=== Test coverage details ==="
rg -A 5 -B 5 "GUEST_ENTERED_LOBBY|GUEST_EXITED_LOBBY" "packages/@webex/plugin-meetings/test/unit/spec/meeting/index.js"
Length of output: 1937
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
packages/@webex/plugin-meetings/test/unit/spec/meeting/index.js (2)
Line range hint
3-3: Consider removing 'jsdom-global/register' import.Since this is a unit test file, it likely doesn't need to register jsdom globally. Importing jsdom directly and using it locally in specific test cases that need it may be cleaner.
Line range hint
9-9: Consider using sinon's fake timers instead of importing an external state machine library.For simple state transitions in unit tests, sinon's fake timers are usually sufficient. Importing an external state machine library adds unnecessary complexity.
packages/@webex/plugin-meetings/src/metrics/constants.ts (1)
74-75: LGTM! The new metrics align with the PR objectives.The added constants follow the established naming conventions and directly address the need for lobby tracking metrics.
Consider maintaining alphabetical order within the
BEHAVIORAL_METRICSobject by moving these constants betweenGUEST_ENTERED_LOBBYbetweenGET_USER_MEDIA_FAILUREandINVALID_ICE_CANDIDATE.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
packages/@webex/plugin-meetings/src/meeting/index.ts(2 hunks)packages/@webex/plugin-meetings/src/metrics/constants.ts(1 hunks)packages/@webex/plugin-meetings/test/unit/spec/meeting/index.js(2 hunks)
🔇 Additional comments (65)
packages/@webex/plugin-meetings/test/unit/spec/meeting/index.js (63)
Line range hint 1-2: LGTM!
The copyright header looks good.
Line range hint 4-5: LGTM!
The lodash imports look good and are being used in the code.
Line range hint 6-6: LGTM!
Importing sinon for test mocking/stubbing is good practice.
Line range hint 7-7: LGTM!
Importing the internal media core module is fine for unit testing purposes.
Line range hint 8-8: LGTM!
Importing the remote media manager module is fine for unit testing purposes.
Line range hint 10-10: LGTM!
Importing uuid for generating unique ids in tests is a good practice.
Line range hint 11-11: LGTM!
Importing chai's assert and expect for making test assertions is a good practice.
Line range hint 12-12: LGTM!
Importing the core Webex and WebexPlugin modules is necessary for instantiating the meetings plugin in tests.
Line range hint 13-13: LGTM!
Importing the support plugin is fine, it is a dependency of the meetings plugin.
Line range hint 14-14: LGTM!
Importing the mock Webex instance for testing is a good practice.
Line range hint 15-15: LGTM!
Importing the static config is fine, it is used to configure the plugin in tests.
Line range hint 16-16: LGTM!
Importing the reconnection not started error is fine, it is used in a test case.
Line range hint 17-17: LGTM!
Importing the Defer class is fine, it is used for creating deferred promises in tests.
Line range hint 18-34: LGTM!
Importing various constants from the meetings plugin is fine, they are used throughout the test cases.
Line range hint 35-41: LGTM!
Importing various enums and constants from the internal media core module is fine, they are used in test cases.
Line range hint 42-42: LGTM!
Importing the local stream event names is fine, they are used in a test case.
Line range hint 43-43: LGTM!
Importing the events scope class is fine, it is used to simulate events in tests.
Line range hint 44-44: LGTM!
Importing the meetings plugin and its constants is necessary for the unit tests.
Line range hint 45-45: LGTM!
Importing the Meeting class is necessary as it is the class being tested.
Line range hint 46-47: LGTM!
Importing the Members class and the members module is fine, they are used in test cases.
Line range hint 48-48: LGTM!
Importing the Roap class is fine, it is used in test cases.
Line range hint 49-50: LGTM!
Importing the MeetingRequest class and module is fine, they are used in test cases.
Line range hint 51-51: LGTM!
Importing the LocusInfo class is fine, it is used throughout the test cases.
Line range hint 52-52: LGTM!
Importing the MediaProperties class is fine, it is used in test cases.
Line range hint 53-53: LGTM!
Importing the MeetingUtil module is fine, it contains utility functions used in tests.
Line range hint 54-54: LGTM!
Importing the MeetingsUtil module is fine, it contains utility functions used in tests.
Line range hint 55-55: LGTM!
Importing the Media module is fine, it is used in test cases.
Line range hint 56-56: LGTM!
Importing the ReconnectionManager class is fine, it is used in test cases.
Line range hint 57-57: LGTM!
Importing the MediaUtil module is fine, it contains utility functions used in tests.
Line range hint 58-58: LGTM!
Importing the RecordingUtil module is fine, it contains utility functions used in tests.
Line range hint 59-59: LGTM!
Importing the ControlsOptionsUtil module is fine, it contains utility functions used in tests.
Line range hint 60-60: LGTM!
Importing the LoggerProxy module is fine, it is used to set up logging in tests.
Line range hint 61-61: LGTM!
Importing the LoggerConfig module is fine, it is used to configure logging in tests.
Line range hint 62-62: LGTM!
Importing the TriggerProxy module is fine, it is used to trigger events in tests.
Line range hint 63-64: LGTM!
Importing the Metrics module and its constants is fine, they are used in test cases.
Line range hint 65-65: LGTM!
Importing the MediaRequestManager class is fine, it is used in test cases.
Line range hint 66-67: LGTM!
Importing the ReceiveSlotManager and SendSlotManager modules is fine, they are used in test cases.
Line range hint 68-68: LGTM!
Importing the CallDiagnosticUtils module is fine, it contains utility functions used in tests.
Line range hint 69-69: LGTM!
Importing the LocusMediaRequest module is fine, it is used in test cases.
Line range hint 71-71: LGTM!
Importing the CallDiagnosticLatencies class is fine, it is used in test cases.
Line range hint 72-72: LGTM!
Importing the LLM plugin is fine, it is a dependency of the meetings plugin.
Line range hint 73-73: LGTM!
Importing the Mercury plugin is fine, it is a dependency of the meetings plugin.
Line range hint 74-74: LGTM!
Importing the Breakouts class is fine, it is used in test cases.
Line range hint 75-75: LGTM!
Importing the SimultaneousInterpretation class is fine, it is used in test cases.
Line range hint 76-76: LGTM!
Importing the Webinar class is fine, it is used in test cases.
Line range hint 77-77: LGTM!
Importing the reaction relay types constant is fine, it is used in a test case.
Line range hint 78-78: LGTM!
Importing the locus fixture is fine, it provides test data.
Line range hint 79-83: LGTM!
Importing various error classes is fine, they are used in test cases.
Line range hint 84-84: LGTM!
Importing the WebExMeetingsErrors class is fine, it is used in a test case.
Line range hint 85-85: LGTM!
Importing the ParameterError class is fine, it is used in a test case.
Line range hint 86-86: LGTM!
Importing the PasswordError class is fine, it is used in test cases.
Line range hint 87-87: LGTM!
Importing the CaptchaError class is fine, it is used in test cases.
Line range hint 88-88: LGTM!
Importing the PermissionError class is fine, it is used in test cases.
Line range hint 89-89: LGTM!
Importing the WebinarRegistrationError class is fine, it is used in a test case.
Line range hint 90-90: LGTM!
Importing the IntentToJoinError class is fine, it is used in a test case.
Line range hint 91-91: LGTM!
Importing the testUtils module is fine, it contains utility functions for tests.
Line range hint 92-96: LGTM!
Importing various meeting info error classes is fine, they are used in test cases.
Line range hint 97-102: LGTM!
Importing various call diagnostic constants is fine, they are used in test cases.
Line range hint 103-103: LGTM!
Importing the CallDiagnosticMetrics class is fine, it is used in test cases.
Line range hint 104-104: LGTM!
Importing the error descriptions constant is fine, it is used in a test case.
Line range hint 105-105: LGTM!
Importing the MeetingCollection class is fine, it is used in a test case.
Line range hint 107-107: LGTM!
Importing the voicea event triggers constant is fine, it is used in test cases.
Line range hint 109-8631: LGTM!
The describe block for the plugin-meetings module and its nested describe blocks for the Meeting class look good. The test cases cover a wide range of functionality and edge cases.
packages/@webex/plugin-meetings/src/meeting/index.ts (1)
3252-3254: LGTM! Behavioral metrics properly implemented for lobby events.
The metrics are correctly implemented in the appropriate event handlers with proper correlation tracking.
Also applies to: 3278-3280
packages/@webex/plugin-meetings/src/metrics/constants.ts (1)
74-75: Verify metric usage and uniqueness.
Let's ensure these metrics are properly integrated and unique.
✅ Verification successful
Let's check how these metrics are used in meeting/index.ts to ensure proper implementation.
Metrics are properly integrated and unique
The metrics are correctly implemented in the codebase:
- They are uniquely defined in
constants.tswith no duplicates - Properly used in
meeting/index.tsalongside corresponding client events - Implementation follows the established pattern of sending behavioral metrics with correlation IDs
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for any existing similar metrics and verify the usage of new metrics
echo "Checking for similar existing metrics..."
rg -i "lobby|guest" packages/@webex/plugin-meetings/src/metrics/
echo "Verifying usage of new metrics..."
rg -l "GUEST_ENTERED_LOBBY|GUEST_EXITED_LOBBY" packages/@webex/plugin-meetings/src/
Length of output: 648
Script:
#!/bin/bash
# Check the implementation of these metrics in meeting/index.ts
echo "Checking metric implementation..."
rg -B 5 -A 5 "GUEST_ENTERED_LOBBY|GUEST_EXITED_LOBBY" packages/@webex/plugin-meetings/src/meeting/index.ts
Length of output: 995
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
packages/@webex/plugin-meetings/test/unit/spec/meeting/index.js (3)
Line range hint
1-100: Consider adding test coverage for error scenarios in the lobby metrics.While the happy path is tested, it would be good to add test cases for:
- Network failures when sending metrics
- Invalid correlation IDs
- Multiple rapid enter/exit events
Line range hint
1-8714: Consider restructuring test organization for better maintainability.The test file is quite large and could benefit from being split into smaller, more focused test files:
- Move media-related tests to a separate file
- Move metrics tests to a separate file
- Move lobby/guest flow tests to a separate file
This would improve maintainability and make it easier to find relevant tests.
Line range hint
1-8714: Consider adding JSDoc comments for test helper functions.The test file contains several helper functions that could benefit from JSDoc documentation to explain their purpose and parameters. For example:
/** * Helper to simulate a ROAP offer being generated * @param {Object} options - Test configuration options * @returns {Promise<void>} */ const simulateRoapOffer = async () => { // ... }This would make the test code more maintainable and easier to understand.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
packages/@webex/plugin-meetings/src/meeting/index.ts(2 hunks)packages/@webex/plugin-meetings/test/unit/spec/meeting/index.js(2 hunks)
🔇 Additional comments (3)
packages/@webex/plugin-meetings/src/meeting/index.ts (2)
3257-3259: LGTM! Guest entered lobby metric implementation looks good.
The behavioral metric for tracking guest entry into the lobby is correctly implemented with correlation ID for tracing.
3283-3285: LGTM! Guest exited lobby metric implementation looks good.
The behavioral metric for tracking guest exit from the lobby is correctly implemented with correlation ID for tracing.
packages/@webex/plugin-meetings/test/unit/spec/meeting/index.js (1)
8680-8686: LGTM! Good addition of guest lobby metrics.
The new behavioral metrics for tracking when guests enter/exit the lobby are well implemented. The metrics include the correlation ID which will help with debugging and analytics.
Also applies to: 8708-8714
This pull request addresses
We have no indication of entering/exiting lobby in SDK's behavioral metrics (we also don't have it in the web app)
by making the following changes
Added metrics for entr/exit of lobby
Change Type
The following scenarios were tested
unit tests
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.
Summary by CodeRabbit
New Features
Bug Fixes
Chores