From b93ddef390703821f320d3776941f99baf2066f7 Mon Sep 17 00:00:00 2001 From: Marcin Date: Tue, 24 Sep 2024 10:53:49 +0100 Subject: [PATCH] fix(meetings): added reachability trigger to metrics (#3850) --- .../plugin-meetings/src/meetings/index.ts | 7 +- .../plugin-meetings/src/reachability/index.ts | 8 ++- .../src/reconnection-manager/index.ts | 2 +- .../test/unit/spec/meetings/index.js | 33 +++++++++- .../test/unit/spec/reachability/index.ts | 66 +++++++++++++++---- 5 files changed, 94 insertions(+), 22 deletions(-) diff --git a/packages/@webex/plugin-meetings/src/meetings/index.ts b/packages/@webex/plugin-meetings/src/meetings/index.ts index 52481a6aee8..ccbf479842d 100644 --- a/packages/@webex/plugin-meetings/src/meetings/index.ts +++ b/packages/@webex/plugin-meetings/src/meetings/index.ts @@ -765,7 +765,7 @@ export default class Meetings extends WebexPlugin { return Promise.all([ this.fetchUserPreferredWebexSite(), this.getGeoHint(), - this.startReachability().catch((error) => { + this.startReachability('registration').catch((error) => { LoggerProxy.logger.error(`Meetings:index#register --> GDM error, ${error.message}`); }), // @ts-ignore @@ -967,12 +967,13 @@ export default class Meetings extends WebexPlugin { /** * initializes and starts gathering reachability for Meetings + * @param {string} trigger - explains the reason for starting reachability * @returns {Promise} * @public * @memberof Meetings */ - startReachability() { - return this.getReachability().gatherReachability(); + startReachability(trigger = 'client') { + return this.getReachability().gatherReachability(trigger); } /** diff --git a/packages/@webex/plugin-meetings/src/reachability/index.ts b/packages/@webex/plugin-meetings/src/reachability/index.ts index 17b60313ab0..7b6868f04f0 100644 --- a/packages/@webex/plugin-meetings/src/reachability/index.ts +++ b/packages/@webex/plugin-meetings/src/reachability/index.ts @@ -93,6 +93,8 @@ export default class Reachability extends EventsScope { expectedResultsCount = {videoMesh: {udp: 0}, public: {udp: 0, tcp: 0, xtls: 0}}; resultsCount = {videoMesh: {udp: 0}, public: {udp: 0, tcp: 0, xtls: 0}}; + protected lastTrigger?: string; + /** * Creates an instance of Reachability. * @param {object} webex @@ -142,13 +144,16 @@ export default class Reachability extends EventsScope { /** * Gets a list of media clusters from the backend and performs reachability checks on all the clusters + * @param {string} trigger - explains the reason for starting reachability * @returns {Promise} reachability results * @public * @memberof Reachability */ - public async gatherReachability(): Promise { + public async gatherReachability(trigger: string): Promise { // Fetch clusters and measure latency try { + this.lastTrigger = trigger; + // kick off ip version detection. For now we don't await it, as we're doing it // to gather the timings and send them with our reachability metrics // @ts-ignore @@ -552,6 +557,7 @@ export default class Reachability extends EventsScope { // @ts-ignore totalTime: this.webex.internal.device.ipNetworkDetector.totalTime, }, + trigger: this.lastTrigger, }; Metrics.sendBehavioralMetric( BEHAVIORAL_METRICS.REACHABILITY_COMPLETED, diff --git a/packages/@webex/plugin-meetings/src/reconnection-manager/index.ts b/packages/@webex/plugin-meetings/src/reconnection-manager/index.ts index ea2c47f05f4..55ca4a764c6 100644 --- a/packages/@webex/plugin-meetings/src/reconnection-manager/index.ts +++ b/packages/@webex/plugin-meetings/src/reconnection-manager/index.ts @@ -342,7 +342,7 @@ export default class ReconnectionManager { } try { - await this.webex.meetings.startReachability(); + await this.webex.meetings.startReachability('reconnection'); } catch (err) { LoggerProxy.logger.info( 'ReconnectionManager:index#reconnect --> Reachability failed, continuing with reconnection attempt, err: ', diff --git a/packages/@webex/plugin-meetings/test/unit/spec/meetings/index.js b/packages/@webex/plugin-meetings/test/unit/spec/meetings/index.js index 9775a52bb97..937fa1fba73 100644 --- a/packages/@webex/plugin-meetings/test/unit/spec/meetings/index.js +++ b/packages/@webex/plugin-meetings/test/unit/spec/meetings/index.js @@ -79,6 +79,7 @@ describe('plugin-meetings', () => { let locusInfo; let services; let catalog; + let startReachabilityStub; describe('meetings index', () => { beforeEach(() => { @@ -129,9 +130,7 @@ describe('plugin-meetings', () => { logger, }); - Object.assign(webex.meetings, { - startReachability: sinon.stub().returns(Promise.resolve()), - }); + startReachabilityStub = sinon.stub(webex.meetings, 'startReachability').resolves(); Object.assign(webex.internal, { llm: {on: sinon.stub()}, @@ -197,6 +196,34 @@ describe('plugin-meetings', () => { assert.calledOnce(MeetingsUtil.checkH264Support); }); + describe('#startReachability', () => { + let gatherReachabilitySpy; + let fakeResult = {id: 'fake-result'}; + + beforeEach(() => { + startReachabilityStub.restore(); + gatherReachabilitySpy = sinon + .stub(webex.meetings.getReachability(), 'gatherReachability') + .resolves(fakeResult); + }); + + it('should gather reachability with default trigger value', async () => { + const result = await webex.meetings.startReachability(); + + assert.calledOnceWithExactly(gatherReachabilitySpy, 'client'); + assert.equal(result, fakeResult); + }); + + it('should gather reachability and pass custom trigger value', async () => { + const trigger = 'custom-trigger'; + + const result = await webex.meetings.startReachability(trigger); + + assert.calledOnceWithExactly(gatherReachabilitySpy, trigger); + assert.equal(result, fakeResult); + }); + }); + describe('#_toggleUnifiedMeetings', () => { it('should have toggleUnifiedMeetings', () => { assert.equal(typeof webex.meetings._toggleUnifiedMeetings, 'function'); diff --git a/packages/@webex/plugin-meetings/test/unit/spec/reachability/index.ts b/packages/@webex/plugin-meetings/test/unit/spec/reachability/index.ts index 2e400817b5e..29b9ae371ec 100644 --- a/packages/@webex/plugin-meetings/test/unit/spec/reachability/index.ts +++ b/packages/@webex/plugin-meetings/test/unit/spec/reachability/index.ts @@ -4,7 +4,6 @@ import sinon from 'sinon'; import EventEmitter from 'events'; import testUtils from '../../../utils/testUtils'; import Reachability, { - ReachabilityResults, ReachabilityResultsForBackend, } from '@webex/plugin-meetings/src/reachability/'; import {ClusterNode} from '../../../../src/reachability/request'; @@ -507,6 +506,11 @@ describe('gatherReachability', () => { mockClusterReachabilityInstances[id] = mockInstance; return mockInstance; }); + + webex.config.meetings.experimental = { + enableTcpReachability: false, + enableTlsReachability: false, + }; }); afterEach(() => { @@ -1044,13 +1048,14 @@ describe('gatherReachability', () => { enableTlsReachability: true, }; - // the metrics related to ipver are not tested in these tests and are all the same, so setting them up here + // the metrics related to ipver and trigger are not tested in these tests and are all the same, so setting them up here const expectedMetricsFull = { ...expectedMetrics, ipver_firstIpV4: -1, ipver_firstIpV6: -1, ipver_firstMdns: -1, ipver_totalTime: -1, + trigger: 'test', }; const receivedEvents = { @@ -1082,7 +1087,7 @@ describe('gatherReachability', () => { reachability.reachabilityRequest.getClusters = sinon.stub().returns(mockGetClustersResult); - const resultPromise = reachability.gatherReachability(); + const resultPromise = reachability.gatherReachability('test'); await testUtils.flushPromises(); @@ -1142,6 +1147,38 @@ describe('gatherReachability', () => { }) ); + it('sends the trigger parameter in the metrics', async () => { + const reachability = new TestReachability(webex); + + const mockGetClustersResult = { + clusters: { + clusterA: { + udp: ['udp-url'], + tcp: [], + xtls: [], + isVideoMesh: false, + }, + }, + joinCookie: {id: 'id'}, + }; + + reachability.reachabilityRequest.getClusters = sinon.stub().returns(mockGetClustersResult); + + const resultPromise = reachability.gatherReachability('some trigger'); + + // let it time out + await testUtils.flushPromises(); + clock.tick(15000); + await resultPromise; + + // check the metric contains the right trigger value + assert.calledWith( + Metrics.sendBehavioralMetric, + 'js_sdk_reachability_completed', + sinon.match({trigger: 'some trigger'}) + ); + }); + it(`starts ip network version detection and includes the results in the metrics`, async () => { webex.config.meetings.experimental = { enableTcpReachability: true, @@ -1180,7 +1217,7 @@ describe('gatherReachability', () => { joinCookie: {id: 'id'}, }); - const resultPromise = reachability.gatherReachability(); + const resultPromise = reachability.gatherReachability('test'); await testUtils.flushPromises(); @@ -1217,6 +1254,7 @@ describe('gatherReachability', () => { ipver_firstIpV6: webex.internal.device.ipNetworkDetector.firstIpV6, ipver_firstMdns: webex.internal.device.ipNetworkDetector.firstMdns, ipver_totalTime: webex.internal.device.ipNetworkDetector.totalTime, + trigger: 'test', }); }); @@ -1248,7 +1286,7 @@ describe('gatherReachability', () => { reachability.reachabilityRequest.getClusters = sinon.stub().returns(mockGetClustersResult); - const resultPromise = reachability.gatherReachability(); + const resultPromise = reachability.gatherReachability('test'); await testUtils.flushPromises(); @@ -1341,7 +1379,7 @@ describe('gatherReachability', () => { reachability.reachabilityRequest.getClusters = sinon.stub().returns(mockGetClustersResult); - const resultPromise = reachability.gatherReachability(); + const resultPromise = reachability.gatherReachability('test'); await testUtils.flushPromises(); @@ -1382,7 +1420,7 @@ describe('gatherReachability', () => { reachability.reachabilityRequest.getClusters = sinon.stub().throws(); - const result = await reachability.gatherReachability(); + const result = await reachability.gatherReachability('test'); assert.empty(result); @@ -1400,7 +1438,7 @@ describe('gatherReachability', () => { reachability.reachabilityRequest.getClusters = sinon.stub().returns(getClustersResult); (reachability as any).performReachabilityChecks = sinon.stub().throws(); - const result = await reachability.gatherReachability(); + const result = await reachability.gatherReachability('test'); assert.empty(result); @@ -1435,7 +1473,7 @@ describe('gatherReachability', () => { reachability.reachabilityRequest.getClusters = sinon.stub().returns(getClustersResult); - const promise = reachability.gatherReachability(); + const promise = reachability.gatherReachability('test'); await simulateTimeout(); await promise; @@ -1481,7 +1519,7 @@ describe('gatherReachability', () => { reachability.reachabilityRequest.getClusters = sinon.stub().returns(getClustersResult); - const promise = reachability.gatherReachability(); + const promise = reachability.gatherReachability('test'); await simulateTimeout(); await promise; @@ -1515,7 +1553,7 @@ describe('gatherReachability', () => { reachability.reachabilityRequest.getClusters = sinon.stub().returns(getClustersResult); - const promise = reachability.gatherReachability(); + const promise = reachability.gatherReachability('test'); await simulateTimeout(); await promise; @@ -1550,7 +1588,7 @@ describe('gatherReachability', () => { reachability.reachabilityRequest.getClusters = sinon.stub().returns(getClustersResult); - const promise = reachability.gatherReachability(); + const promise = reachability.gatherReachability('test'); await simulateTimeout(); await promise; @@ -1595,7 +1633,7 @@ describe('gatherReachability', () => { return getClustersResult; }); - const promise = reachability.gatherReachability(); + const promise = reachability.gatherReachability('test'); await simulateTimeout(); await promise; @@ -1616,7 +1654,7 @@ describe('gatherReachability', () => { throw new Error('fake error'); }); - const promise = reachability.gatherReachability(); + const promise = reachability.gatherReachability('test'); await simulateTimeout();