fix(plugin-meetings): metrics on reachability subnet #4234
Conversation
📝 WalkthroughWalkthroughThe changes introduce tracking and reporting of media server subnet reachability in the Webex Meetings plugin. A new utility method extracts the media server IP from SDP, and this IP is stored as a private property in the Meeting class. During media connection setup, the system checks if the subnet of the media server IP was previously reached using a new method in the Reachability class, which analyzes the subnets gathered during ICE candidate processing. This subnet reachability status is included in behavioral metrics. Related tests are updated and new tests are added to verify the subnet reachability logic and its integration with metrics and ICE candidate handling. 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) Tip ⚡️ Faster reviews with caching
Enjoy the performance boost—your workflow just got faster. 📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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
|
k-wasniowski
force-pushed
the
feat-metrics-on-reachability-subnet
branch
from
ed8f609 to
c1e9e85
Compare
|
This pull request is automatically being deployed by Amplify Hosting (learn more). |
k-wasniowski
force-pushed
the
feat-metrics-on-reachability-subnet
branch
from
c1e9e85 to
90235fc
Compare
k-wasniowski
force-pushed
the
feat-metrics-on-reachability-subnet
branch
from
90235fc to
296c10a
Compare
packages/@webex/plugin-meetings/src/reachability/clusterReachability.ts
Outdated
Show resolved
Hide resolved
k-wasniowski
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (2)
packages/@webex/plugin-meetings/src/reachability/clusterReachability.ts (2)
365-375: Consider browser compatibility with URL extractionThere's a browser compatibility concern with accessing the
urlproperty on ICE candidates.According to the previous comments, the
urlproperty is only available in Chrome (possibly Edge/Opera too). Consider adding a fallback mechanism for other browsers or confirming that this meets your browser support requirements.#!/bin/bash # Check browser compatibility for RTCIceCandidate.url property rg -A 5 -B 5 "browser.*compatibility|browser.*support" --type js
386-389: Consider not stopping after first result for a protocolEarly termination when protocol results are available might miss additional useful data.
Based on previous comments, there was a concern about stopping when getting the first result for a protocol. While you indicated TCP and TLS are more time-consuming than STUN, consider whether gathering additional candidates could provide more comprehensive subnet data.
#!/bin/bash # Check if there are other places in the codebase where ice candidates continue to be collected rg -A 5 "onicecandidate.*\{" --type js
🧹 Nitpick comments (3)
packages/@webex/plugin-meetings/src/meeting/index.ts (1)
725-725: Property type could be more precise withstring | nullSince this property is initialized as
nulland later set to a string, consider changing its type tostring | nullfor better type safety.- private mediaServerIp: string; + private mediaServerIp: string | null;packages/@webex/plugin-meetings/test/unit/spec/reachability/clusterReachability.ts (1)
460-464: Improve assertion readability with sortingFor more reliable test assertions on Set data, consider sorting the array to ensure consistent order across test runs.
assert.deepEqual(Array.from(clusterReachability.reachedSubnets), [ - '1.2.3.4', - '4.3.2.1', - 'someTurnRelayIp' + '1.2.3.4', + '4.3.2.1', + 'someTurnRelayIp' + ].sort());Alternatively, you could use a Set-specific assertion pattern to avoid order dependency:
const expected = new Set(['1.2.3.4', '4.3.2.1', 'someTurnRelayIp']); assert.deepEqual(clusterReachability.reachedSubnets, expected);packages/@webex/plugin-meetings/src/reachability/clusterReachability.ts (1)
369-369: Consider using proper type checking instead of type assertionThe
as anytype assertion bypasses TypeScript's type checking. Consider using proper type checking instead.- const match = (e.candidate as any).url.match(regex); + // Check if url property exists before accessing + const candidateUrl = 'url' in e.candidate ? (e.candidate as any).url : undefined; + const match = candidateUrl ? candidateUrl.match(regex) : null;
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
packages/@webex/plugin-meetings/src/meeting/index.ts(8 hunks)packages/@webex/plugin-meetings/src/meetings/util.ts(1 hunks)packages/@webex/plugin-meetings/src/reachability/clusterReachability.ts(4 hunks)packages/@webex/plugin-meetings/src/reachability/index.ts(1 hunks)packages/@webex/plugin-meetings/test/unit/spec/meeting/index.js(16 hunks)packages/@webex/plugin-meetings/test/unit/spec/reachability/clusterReachability.ts(1 hunks)packages/@webex/plugin-meetings/test/unit/spec/reachability/index.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
packages/@webex/plugin-meetings/src/meeting/index.ts (1)
packages/@webex/plugin-meetings/src/reachability/index.ts (1)
isSubnetReachable(148-184)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build Packages
- GitHub Check: AWS Amplify Console Web Preview
🔇 Additional comments (27)
packages/@webex/plugin-meetings/test/unit/spec/reachability/index.ts (1)
2689-2719: Well-structured tests for the newisSubnetReachablemethod.The tests effectively verify the subnet matching logic by mocking the necessary data and asserting expected behavior for both positive and negative cases.
packages/@webex/plugin-meetings/src/meeting/index.ts (5)
2353-2371: LGTM: Media server IP extraction from SDP for metricsThis code properly extracts the media server IP address from the SDP for multistream meetings, which will be used for subnet reachability metrics reporting.
7774-7777: Integration with reachability subsystem for metricsGood integration with the reachability subsystem to check if the media server's subnet was previously reachable. This information will be valuable for diagnosing connection issues.
7788-7788: LGTM: Including subnet reachability in success metricsIncluding the subnet reachability status in success metrics will help correlate successful connections with network reachability.
7818-7821: LGTM: Including subnet reachability in failure metricsIncluding the subnet reachability status in failure metrics will help identify if connection failures are related to network subnet reachability issues.
1610-1610: LGTM: Initializing mediaServerIp in constructorThe mediaServerIp property is correctly initialized as null in the constructor.
packages/@webex/plugin-meetings/test/unit/spec/meeting/index.js (16)
253-256: LGTM: Good test coverage for new reachability featureThe addition of the
isSubnetReachablestub in the reachability mock is consistent with the PR objective of adding metrics for server subnet reachability. This ensures the new functionality is properly stubbed in tests.
2123-2126: Consistent mocking of new reachability methodThe stub implementation for
isSubnetReachablematches other test setup patterns in this file and ensures tests have coverage for the new subnet reachability functionality.
2179-2182: Successfully integrating subnet reachability in metrics assertionsThe addition of
isSubnetReachable: trueto the metrics object correctly validates that the subnet reachability information is being included in metrics submissions.
2226-2229: Good coverage of connection state metricsThe inclusion of
isSubnetReachablestatus in the connection state metrics provides valuable diagnostic information for network troubleshooting, which aligns with the PR objectives.
2241-2244: Consistent mocking of subnet reachabilityProper stubbing of the
isSubnetReachablemethod ensures test consistency for scenarios involving media connection failures.
2292-2295: Validating metrics payload structureInclusion of
isSubnetReachablestatus in assertions ensures the property is correctly included in metrics submissions during failure scenarios.
2350-2353: Comprehensive ICE connection state testingThe addition of subnet reachability status enriches the connection state metrics, providing better visibility into media connection issues.
2408-2411: Thorough test coverage for connection scenariosConsistent inclusion of subnet reachability in metrics validation ensures comprehensive test coverage across different connection states.
2746-2749: Good setup for ICE failure scenariosProper stubbing of
isSubnetReachablefor testing ICE failure scenarios ensures that subnet reachability information is available during error handling flows.
2926-2929: Comprehensive metrics validationThe test properly validates that subnet reachability data is included in metrics submissions during network connection scenarios.
2957-2960: Consistent test patterns across error scenariosThe addition of
isSubnetReachablestub is consistent with other reachability mocks in the test file, ensuring comprehensive test coverage.
3125-3128: Validating metrics during TURN discoveryIncluding subnet reachability status in TURN discovery metrics provides valuable network diagnostics for troubleshooting connectivity issues.
3254-3257: Thorough mock setup for ICE candidate errorsThe
isSubnetReachablestub is properly added to the reachability mock object, ensuring consistent test behavior for ICE candidate error scenarios.
3282-3285: Comprehensive validation of ICE error metricsThe test properly validates that the subnet reachability status is included in metrics submissions alongside ICE candidate error information.
3345-3348: Thorough connection state metrics testingInclusion of subnet reachability status in the metrics assertions ensures proper validation of the metrics structure during ICE connection state transitions.
3407-3410: Complete ICE error metrics validationThe test properly validates that subnet reachability information is included alongside ICE error metrics, providing comprehensive test coverage for the new feature.
packages/@webex/plugin-meetings/src/reachability/clusterReachability.ts (5)
52-52: LGTM: Addition of reachedSubnets propertyThis new Set will efficiently track unique server subnet IP addresses reached during ICE candidate gathering.
268-276: LGTM: Added serverIp parameter to saveResult methodThe method signature is appropriately extended to accept and process server IP addresses.
304-307: LGTM: Logic to store reached server IPsThis implementation properly adds server IPs to the reachedSubnets set when provided.
376-376: LGTM: Saving server IP for srflx candidatesProperly passing the extracted server IP to the saveResult method.
383-383: LGTM: Saving server IP for relay candidatesCorrectly passing the candidate's address as the server IP for relay candidates.
packages/@webex/plugin-meetings/test/unit/spec/reachability/clusterReachability.ts
Outdated
Show resolved
Hide resolved
packages/@webex/plugin-meetings/test/unit/spec/reachability/clusterReachability.ts
Outdated
Show resolved
Hide resolved
packages/@webex/plugin-meetings/test/unit/spec/reachability/clusterReachability.ts
Show resolved
Hide resolved
k-wasniowski
requested review from
SomeBody16,
antsukanova,
brycetham,
edvujic and
szotrabh
chrisadubois
left a comment
chrisadubois
left a comment
There was a problem hiding this comment.
I do not fully understand everythiung in this PR and did not validate it with a "liar" subnet but I get what is happening here, and the code appears safe. We could prioritize getting this in quickly on monday for a prod deploy cc @Coread @marcin-bazyl
packages/@webex/plugin-meetings/src/reachability/clusterReachability.ts
Outdated
Show resolved
Hide resolved
| if (match) { | ||
| // eslint-disable-next-line prefer-destructuring | ||
| serverIp = match[1]; | ||
| } | ||
| } | ||
|
|
||
| this.saveResult('udp', latencyInMilliseconds, e.candidate.address, serverIp); |
There was a problem hiding this comment.
Suggestion: use ternary operator
| if (match) { | |
| // eslint-disable-next-line prefer-destructuring | |
| serverIp = match[1]; | |
| } | |
| } | |
| this.saveResult('udp', latencyInMilliseconds, e.candidate.address, serverIp); | |
| const match = (e.candidate as any).url.match(regex); | |
| match && ([, serverIp] = match); | |
| this.saveResult('udp', latencyInMilliseconds, e.candidate.address, serverIp); |
There was a problem hiding this comment.
I'm not sure if this line is good readable.. match && ([, serverIp] = match);
| for (let i = 0; i < reachedSubnetsArray.length; i += 1) { | ||
| const subnet = reachedSubnetsArray[i]; | ||
| const reachedSubnetFirstOctet = subnet.split('.')[0]; | ||
|
|
||
| if (subnetFirstOctet === reachedSubnetFirstOctet) { | ||
| acc.add(cluster.name); | ||
| } | ||
|
|
||
| logMessage += `${subnet}`; | ||
| if (i < reachedSubnetsArray.length - 1) { | ||
| logMessage += ','; | ||
| } | ||
| } | ||
| logMessage += `]`; |
There was a problem hiding this comment.
(1)
Suggestion:
you can use arrow function here
const logMessage = reachedSubnetsArray
.map((subnet) => {
const reachedSubnetFirstOctet = subnet.split('.')[0];
if (subnetFirstOctet === reachedSubnetFirstOctet) {
acc.add(cluster.name);
}
return subnet;
})
.join(',');
| } | ||
| logMessage += `]`; | ||
|
|
||
| LoggerProxy.logger.info(logMessage); |
There was a problem hiding this comment.
(2) and then use it here:
| LoggerProxy.logger.info(logMessage); | |
| LoggerProxy.logger.info( | |
| `Reachability:index#isSubnetReachable --> Cluster ${cluster.name} reached [${logMessage}]` | |
| ); |
packages/@webex/plugin-meetings/test/unit/spec/reachability/clusterReachability.ts
Outdated
Show resolved
Hide resolved
packages/@webex/plugin-meetings/test/unit/spec/reachability/clusterReachability.ts
Show resolved
Hide resolved
packages/@webex/plugin-meetings/test/unit/spec/reachability/clusterReachability.ts
Outdated
Show resolved
Hide resolved
Co-authored-by: Edmond Vujići <[email protected]>
5 participants
COMPLETES #SPARK-660018
This pull request addresses
Add metrics on servers subnet reachability.
by making the following changes
Change Type
The following scenarios were tested
Tested with JS-SDK sample app
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.