feat(plugin-meetings): add meetingUserCount mqe #4060
Conversation
|
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) 📝 WalkthroughWalkthroughThe pull request introduces changes to the Changes
Sequence DiagramsequenceDiagram
participant Meeting
participant StatsAnalyzer
participant Members
Meeting->>Members: Filter members in 'JOINED' state
Members-->>Meeting: Return count of JOINED members
Meeting->>StatsAnalyzer: Add meetingUserCount to metrics
StatsAnalyzer->>StatsAnalyzer: Process MEDIA_QUALITY event
Possibly related PRs
✨ 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
|
|
This pull request is automatically being deployed by Amplify Hosting (learn more). |
edvujic
requested review from
antsukanova,
szotrabh,
brycetham and
k-wasniowski
packages/@webex/plugin-meetings/test/unit/spec/meeting/index.js
Outdated
Show resolved
Hide resolved
packages/@webex/plugin-meetings/test/unit/spec/meeting/index.js
Outdated
Show resolved
Hide resolved
packages/@webex/plugin-meetings/test/unit/spec/meeting/index.js
Outdated
Show resolved
Hide resolved
packages/@webex/plugin-meetings/test/unit/spec/meeting/index.js
Outdated
Show resolved
Hide resolved
|
Please update the docs after the PR will be closed: https://confluence-eng-gpk2.cisco.com/conf/display/webexmedia/MQE+Metrics+Tables#:~:text=Window.innerWidth.-,meetingUserCount,-No |
![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 (1)
packages/@webex/plugin-meetings/test/unit/spec/meeting/index.js (1)
3482-3525: LGTM! Consider adding a few more test cases.The test implementation for counting meeting users in MQE events looks good. It verifies both the initial count and updates when member states change. The test structure is clear and follows good practices.
A few suggestions to make it even better:
- Add test cases for edge cases like empty members collection
- Add test cases for members with other states besides
isInMeeting: true/false- Consider testing the error handling path when getMembers() fails
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/@webex/plugin-meetings/test/unit/spec/meeting/index.js(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Initialize Project
- GitHub Check: AWS Amplify Console Web Preview
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/@webex/plugin-meetings/src/meeting/index.ts (1)
6450-6457: Improve the member counting logic for better performance and readability.The current implementation filters members to count participants in the meeting. Consider these improvements:
- Use a more concise array method chain
- Add type safety by explicitly typing the members object
- Cache the result to avoid recalculating on every stats event
- const {members} = this.getMembers().membersCollection; - options.data.intervalMetadata.meetingUserCount = Object.values(members).filter( - (member: Member) => member.isInMeeting - ).length; + const {membersCollection} = this.getMembers(); + options.data.intervalMetadata.meetingUserCount = Object.values(membersCollection.members as Record<string, Member>) + .filter((member) => member.isInMeeting) + .length;Consider caching the count and only updating it when member states change to improve performance.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 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(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/@webex/plugin-meetings/test/unit/spec/meeting/index.js
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build Packages
- GitHub Check: AWS Amplify Console Web Preview
🔇 Additional comments (1)
packages/@webex/plugin-meetings/src/meeting/index.ts (1)
163-163: LGTM!The import statement for the Member class is correctly added and aligns with its usage in the code.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/@webex/plugin-meetings/test/unit/spec/meeting/index.js (1)
3482-3525: LGTM! Well structured test for meeting participant count.The test case provides good coverage for the meeting user count functionality:
- Verifies initial count of joined participants
- Tests count updates when participant states change
- Validates the count is correctly included in media quality metrics
- Uses clear test data setup and assertions
Consider adding these minor improvements:
- let fakeMembersCollection = { + const fakeMembersCollection = {Also consider adding test cases for:
- Edge cases like 0 participants
- Large number of participants
- Participants in other states besides JOINED/LEFT
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 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(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/@webex/plugin-meetings/src/meeting/index.ts
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build Packages
- GitHub Check: AWS Amplify Console Web Preview
COMPLETES SPARK-602531
This pull request addresses
... the MQE stat for
meetingUserCount.by making the following changes
Change Type
The following scenarios were tested
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.