fix: added ipversion to metrics #4271
Conversation
📝 WalkthroughWalkthroughThe changes introduce IP version (IPv4 or IPv6) detection and reporting throughout the Webex meetings plugin. The 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. 🔧 ast-grep (0.38.1)packages/@webex/plugin-meetings/test/unit/spec/meeting/index.js🔧 ESLint
yarn install v1.22.22 (For a CapTP with native promises, see @endo/eventual-send and @endo/captp) Note ⚡️ AI Code Reviews for VS Code, Cursor, WindsurfCodeRabbit now has a plugin for VS Code, Cursor and Windsurf. This brings AI code reviews directly in the code editor. Each commit is reviewed immediately, finding bugs before the PR is raised. Seamless context handoff to your AI code agent ensures that you can easily incorporate review feedback. Note ⚡️ Faster reviews with cachingCodeRabbit now supports caching for code and dependencies, helping speed up reviews. This means quicker feedback, reduced wait times, and a smoother review experience overall. Cached data is encrypted and stored securely. This feature will be automatically enabled for all accounts on May 30th. To opt out, configure 📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 90000ms (2)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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). |
marcin-bazyl
force-pushed
the
spark-663300
branch
from
67d2fb5 to
f387e70
Compare
marcin-bazyl
added
the
validated
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
packages/@webex/plugin-meetings/test/unit/spec/media/properties.ts (1)
76-84:⚠️ Potential issue
getStatsis no longer a function – this will throw immediately, defeating the timeout testBy overwriting
mockMC.getStatswith a Promise instance you turn the property from a callable function into a plain object.
MediaProperties#getCurrentConnectionInfo()subsequently callsthis.webrtcMediaConnection.getStats(), causing aTypeError: getStats is not a functionlong before the 1-second timeout is reached, so the test never verifies what it intends to.Proposed fix:
-// Promise that never resolves -mockMC.getStats = new Promise(() => {}); +// Stub that returns a promise that never resolves +mockMC.getStats.resetBehavior(); +mockMC.getStats.returns(new Promise(() => {}));This keeps
getStatscallable while still simulating the hanging behaviour.
🧹 Nitpick comments (2)
packages/@webex/plugin-meetings/src/media/properties.ts (2)
219-226:isIPv6heuristic may mis-classify unusual but valid IPv4 literalsThe helper currently decides solely on the presence of
':'.
While perfectly fine for standard dotted-decimal vs. colon-hex notation, addresses such as IPv4-mapped IPv6 literals (::ffff:192.0.2.1) or future textual representations could be mis-categorised.Not blocking, but if you need stricter accuracy consider a simple RegExp:
private isIPv6(ip: string): boolean { return /^[0-9a-fA-F:]+$/.test(ip) && ip.includes(':'); }
233-304: Repeated full-SDP parsing may be expensive – cache or short-circuit
getConnectionIpVersionparses the entire local SDP every time it is called and when the ICE stats are missing an address. In a busy stats-polling loop this could become non-trivial overhead.Consider memoising the parsed SDP per
RTCPeerConnection(e.g., store on aWeakMap) or parsing only once on first call.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (5)
packages/@webex/plugin-meetings/package.json(1 hunks)packages/@webex/plugin-meetings/src/media/properties.ts(6 hunks)packages/@webex/plugin-meetings/src/meeting/index.ts(3 hunks)packages/@webex/plugin-meetings/test/unit/spec/media/properties.ts(3 hunks)packages/@webex/plugin-meetings/test/unit/spec/meeting/index.js(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build Packages
- GitHub Check: AWS Amplify Console Web Preview
🔇 Additional comments (8)
packages/@webex/plugin-meetings/package.json (1)
78-78: Add SDP parsing library dependency
The new@webex/ts-sdp@^1.8.1dependency is necessary for the IP version extraction via SDP parsing and aligns with the PR objective.packages/@webex/plugin-meetings/src/meeting/index.ts (3)
7775-7776: Correctly extracting IP version information for metrics.The code now extracts
ipVersionalong with other connection information, which will be used for logging network metrics about the media connection.
7786-7787: Added IP version to behavioral metrics payload.Including
ipVersionin theADD_MEDIA_SUCCESSbehavioral metric provides essential network connection information that can help with media connection diagnostics and analysis.
7799-7801: Added IP version to client event payload.The IP version information is now also included in the client event for
client.media-engine.ready, ensuring consistent reporting across different types of metrics. This change completes the implementation by making the IP version visible in Customer Analytics events.packages/@webex/plugin-meetings/test/unit/spec/meeting/index.js (4)
2050-2055: Test expectations correctly include the new ipVersion fieldThe test mock for
getCurrentConnectionInfonow properly includes theipVersionfield with 'IPv6' value, correctly reflecting the implementation changes in the MediaProperties class.
3067-3069: Client event payload now includes IP version dataThe test assertion for the 'client.media-engine.ready' event correctly verifies that the implementation now includes the IP version in client event payloads.
3126-3126: Updated metrics assertions for IP version reportingGood job updating the metrics verification to include the new
ipVersionfield, ensuring that the connection information is properly reported in the metrics payload.
3280-3280: Consistent IP version reporting in all metrics assertionsThe test consistently checks for the
ipVersionfield across different test scenarios, properly validating the implementation's IP version reporting across various connection cases.
marcin-bazyl
force-pushed
the
spark-663300
branch
from
f387e70 to
0ad0842
Compare
marcin-bazyl
force-pushed
the
spark-663300
branch
from
0ad0842 to
f4efadd
Compare
| * @returns {boolean} true if the address is IPv6, false otherwise | ||
| */ | ||
| private isIPv6(ip: string): boolean { | ||
| return ip.includes(':'); |
There was a problem hiding this comment.
this one makes me laugh !
There was a problem hiding this comment.
as long as it's called with an ip address, then it does the job :-) If it ever needs to be more robust to handle non-ip strings like urls, then it can be expanded when needed.
3 participants
COMPLETES #SPARK-663300
This pull request addresses
Missing ipVersion in our metrics
by making the following changes
Added ipVersion to Amplitude metrics and CA event. It indicates what ip version was used for the media connection.
I've started using ts-sdp for SDP parsing in this PR, because we now have multiple places where we parse SDP in SDK and we've been using regex in them, once this PR lands, I'll update the other places to use ts-sdp in another PR.
Change Type
The following scenarios where tested
unit tests, e2e with web app - verified CA result, see correlationId=d5807698-bb5c-4a7a-a78e-a10286663e94
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.