From e5019482ca087f20b0886a2ebcadf439e2b637cd Mon Sep 17 00:00:00 2001 From: Warren James Date: Mon, 13 May 2024 23:17:43 -0400 Subject: [PATCH 01/12] fix bug --- src/sdam/monitor.ts | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/src/sdam/monitor.ts b/src/sdam/monitor.ts index 769c41d16d..0bce6bfc68 100644 --- a/src/sdam/monitor.ts +++ b/src/sdam/monitor.ts @@ -219,8 +219,8 @@ export class Monitor extends TypedEventEmitter { return this.rttSampler.min(); } - get latestRtt(): number { - return this.rttSampler.last ?? 0; // FIXME: Check if this is acceptable + get latestRtt(): number | null { + return this.rttSampler.last; // FIXME: Check if this is acceptable } addRttSample(rtt: number) { @@ -304,7 +304,8 @@ function checkServer(monitor: Monitor, callback: Callback) { } // NOTE: here we use the latestRtt as this measurement corresponds with the value - // obtained for this successful heartbeat + // obtained for this successful heartbeat, if there is no latestRtt, then we calculate the + // duration const duration = isAwaitable && monitor.rttPinger ? monitor.rttPinger.latestRtt ?? calculateDurationInMs(start) @@ -498,7 +499,7 @@ export class RTTPinger { this[kCancellationToken] = monitor[kCancellationToken]; this.closed = false; this.monitor = monitor; - this.latestRtt = monitor.latestRtt; + this.latestRtt = monitor.latestRtt ?? undefined; const heartbeatFrequencyMS = monitor.options.heartbeatFrequencyMS; this[kMonitorId] = setTimeout(() => this.measureRoundTripTime(), heartbeatFrequencyMS); @@ -565,7 +566,7 @@ export class RTTPinger { connection.serverApi?.version || connection.helloOk ? 'hello' : LEGACY_HELLO_COMMAND; // eslint-disable-next-line github/no-then connection.command(ns('admin.$cmd'), { [commandName]: 1 }, undefined).then( - () => this.measureAndReschedule(), + () => this.measureAndReschedule(start), () => { this.connection?.destroy(); this.connection = undefined; From 27f4de2aa12664e3cc13d6703502a3d3333f8372 Mon Sep 17 00:00:00 2001 From: Warren James Date: Mon, 13 May 2024 23:23:00 -0400 Subject: [PATCH 02/12] tighten types --- src/sdam/monitor.ts | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/src/sdam/monitor.ts b/src/sdam/monitor.ts index 0bce6bfc68..1b70359730 100644 --- a/src/sdam/monitor.ts +++ b/src/sdam/monitor.ts @@ -521,10 +521,7 @@ export class RTTPinger { this.connection = undefined; } - private measureAndReschedule(start?: number, conn?: Connection) { - if (start == null) { - start = now(); - } + private measureAndReschedule(start: number, conn?: Connection) { if (this.closed) { conn?.destroy(); return; From f7b5d0c39bdb986a786a8d68f84c89c7ec64babd Mon Sep 17 00:00:00 2001 From: Warren James Date: Mon, 13 May 2024 23:37:00 -0400 Subject: [PATCH 03/12] Add test to verify that rtt is never reported as zero for a successfull heartbeat --- .../server_discover_and_monitoring.test.ts | 46 +++++++++++++++++++ 1 file changed, 46 insertions(+) diff --git a/test/integration/server-discovery-and-monitoring/server_discover_and_monitoring.test.ts b/test/integration/server-discovery-and-monitoring/server_discover_and_monitoring.test.ts index d0b3fc9944..3785a1b1d9 100644 --- a/test/integration/server-discovery-and-monitoring/server_discover_and_monitoring.test.ts +++ b/test/integration/server-discovery-and-monitoring/server_discover_and_monitoring.test.ts @@ -1,3 +1,8 @@ +import { EventEmitter, once } from 'node:events'; + +import { expect } from 'chai'; + +import { type MongoClient, type ServerHeartbeatSucceededEvent } from '../../mongodb'; import { loadSpecTests } from '../../spec'; import { runUnifiedSuite } from '../../tools/unified-spec-runner/runner'; @@ -8,3 +13,44 @@ describe('SDAM Unified Tests (Node Driver)', function () { ); runUnifiedSuite(clonedAndAlteredSpecTests); }); + +describe('Monitoring rtt tests', function () { + let client: MongoClient; + let durations: number[]; + const thresh = 40; + const ee = new EventEmitter(); + const listener = (ev: ServerHeartbeatSucceededEvent) => { + durations.push(ev.duration); + if (durations.length >= thresh) { + client.off('serverHeartbeatSucceeded', listener); + ee.emit('done'); + } + }; + + beforeEach(function () { + durations = []; + }); + + afterEach(async function () { + await client?.close(); + }); + + for (const serverMonitoringMode of ['poll', 'stream']) { + context(`when serverMonitoringMode is set to '${serverMonitoringMode}'`, function () { + beforeEach(async function () { + client = this.configuration.newClient({ + heartbeatFrequencyMS: 100, + serverMonitoringMode + }); + client.on('serverHeartbeatSucceeded', listener); + + await client.connect(); + }); + + it('duration of a successful heartbeat is never reported as 0ms', async function () { + await once(ee, 'done'); + expect(durations.every(x => x !== 0)).to.be.true; + }); + }); + } +}); From 1cc50d85b245e906a0732f9f7b968f4bef928057 Mon Sep 17 00:00:00 2001 From: Warren James Date: Tue, 14 May 2024 11:25:05 -0400 Subject: [PATCH 04/12] add tests --- .../server_discover_and_monitoring.test.ts | 54 +++++++++++++++---- 1 file changed, 45 insertions(+), 9 deletions(-) diff --git a/test/integration/server-discovery-and-monitoring/server_discover_and_monitoring.test.ts b/test/integration/server-discovery-and-monitoring/server_discover_and_monitoring.test.ts index 3785a1b1d9..23a53e1163 100644 --- a/test/integration/server-discovery-and-monitoring/server_discover_and_monitoring.test.ts +++ b/test/integration/server-discovery-and-monitoring/server_discover_and_monitoring.test.ts @@ -16,19 +16,37 @@ describe('SDAM Unified Tests (Node Driver)', function () { describe('Monitoring rtt tests', function () { let client: MongoClient; - let durations: number[]; - const thresh = 40; + let windows: Record; + const thresh = 100; // Wait for 100 total heartbeats + const SAMPLING_WINDOW_SIZE = 10; + let count: number; const ee = new EventEmitter(); const listener = (ev: ServerHeartbeatSucceededEvent) => { - durations.push(ev.duration); - if (durations.length >= thresh) { + try { + // @ts-expect-error accessing private fields + const rttSampler = client.topology.s.servers.get(ev.connectionId).monitor.rttSampler; + // @ts-expect-error accessing private fields + const rttSamples = rttSampler.rttSamples; + windows[ev.connectionId].push(Array.from(rttSamples)); + count++; + } catch { + // silently ignore when servers aren't yet populated + } + + if (count === SAMPLING_WINDOW_SIZE) { + ee.emit('samplingWindowFilled'); + return; + } + + if (count >= thresh) { client.off('serverHeartbeatSucceeded', listener); ee.emit('done'); } }; beforeEach(function () { - durations = []; + count = 0; + windows = {}; }); afterEach(async function () { @@ -39,17 +57,35 @@ describe('Monitoring rtt tests', function () { context(`when serverMonitoringMode is set to '${serverMonitoringMode}'`, function () { beforeEach(async function () { client = this.configuration.newClient({ - heartbeatFrequencyMS: 100, + heartbeatFrequencyMS: 500, + connectTimeoutMS: 1000, serverMonitoringMode }); client.on('serverHeartbeatSucceeded', listener); await client.connect(); + + for (const k of client.topology.s.servers.keys()) { + windows[k] = []; + } + + await once(ee, 'samplingWindowFilled'); }); - it('duration of a successful heartbeat is never reported as 0ms', async function () { - await once(ee, 'done'); - expect(durations.every(x => x !== 0)).to.be.true; + it('rttSampler does not accumulate 0 rtt', { + metadata: { + requires: { topology: '!load-balanced' } + }, + test: async function () { + await once(ee, 'done'); + for (const s in windows) { + // Test that at every point we collect a heartbeat, the rttSampler is not filled with + // zeroes + for (const window of windows[s]) { + expect(window.reduce((acc, x) => x + acc)).to.be.greaterThanOrEqual(0); + } + } + } }); }); } From b10242f076fe276a8526c23a532c78e1b666daf7 Mon Sep 17 00:00:00 2001 From: Warren James Date: Tue, 14 May 2024 11:34:58 -0400 Subject: [PATCH 05/12] fix up tests --- .../server_discover_and_monitoring.test.ts | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/test/integration/server-discovery-and-monitoring/server_discover_and_monitoring.test.ts b/test/integration/server-discovery-and-monitoring/server_discover_and_monitoring.test.ts index 23a53e1163..c61f55d0d3 100644 --- a/test/integration/server-discovery-and-monitoring/server_discover_and_monitoring.test.ts +++ b/test/integration/server-discovery-and-monitoring/server_discover_and_monitoring.test.ts @@ -17,10 +17,11 @@ describe('SDAM Unified Tests (Node Driver)', function () { describe('Monitoring rtt tests', function () { let client: MongoClient; let windows: Record; - const thresh = 100; // Wait for 100 total heartbeats + const THRESH = 100; // Wait for 100 total heartbeats. This is high enough to work for standalone, sharded and our typical 3-node replica set topology tests const SAMPLING_WINDOW_SIZE = 10; let count: number; const ee = new EventEmitter(); + const listener = (ev: ServerHeartbeatSucceededEvent) => { try { // @ts-expect-error accessing private fields @@ -38,7 +39,7 @@ describe('Monitoring rtt tests', function () { return; } - if (count >= thresh) { + if (count >= THRESH) { client.off('serverHeartbeatSucceeded', listener); ee.emit('done'); } @@ -82,7 +83,7 @@ describe('Monitoring rtt tests', function () { // Test that at every point we collect a heartbeat, the rttSampler is not filled with // zeroes for (const window of windows[s]) { - expect(window.reduce((acc, x) => x + acc)).to.be.greaterThanOrEqual(0); + expect(window.reduce((acc, x) => x + acc)).to.be.greaterThan(0); } } } From 21648996795239ad5986a585d9cb2e3610e386a3 Mon Sep 17 00:00:00 2001 From: Warren James Date: Tue, 14 May 2024 13:11:26 -0400 Subject: [PATCH 06/12] collect more heartbeats --- .../server_discover_and_monitoring.test.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/integration/server-discovery-and-monitoring/server_discover_and_monitoring.test.ts b/test/integration/server-discovery-and-monitoring/server_discover_and_monitoring.test.ts index c61f55d0d3..51f3f986f8 100644 --- a/test/integration/server-discovery-and-monitoring/server_discover_and_monitoring.test.ts +++ b/test/integration/server-discovery-and-monitoring/server_discover_and_monitoring.test.ts @@ -17,7 +17,7 @@ describe('SDAM Unified Tests (Node Driver)', function () { describe('Monitoring rtt tests', function () { let client: MongoClient; let windows: Record; - const THRESH = 100; // Wait for 100 total heartbeats. This is high enough to work for standalone, sharded and our typical 3-node replica set topology tests + const THRESH = 200; // Wait for 100 total heartbeats. This is high enough to work for standalone, sharded and our typical 3-node replica set topology tests const SAMPLING_WINDOW_SIZE = 10; let count: number; const ee = new EventEmitter(); @@ -58,7 +58,7 @@ describe('Monitoring rtt tests', function () { context(`when serverMonitoringMode is set to '${serverMonitoringMode}'`, function () { beforeEach(async function () { client = this.configuration.newClient({ - heartbeatFrequencyMS: 500, + heartbeatFrequencyMS: 100, connectTimeoutMS: 1000, serverMonitoringMode }); From 0c923e74f75381878e92e436f8d7eb78e49d5741 Mon Sep 17 00:00:00 2001 From: Warren James Date: Tue, 14 May 2024 13:25:08 -0400 Subject: [PATCH 07/12] add listener after connecting --- .../server_discover_and_monitoring.test.ts | 23 +++++++++---------- 1 file changed, 11 insertions(+), 12 deletions(-) diff --git a/test/integration/server-discovery-and-monitoring/server_discover_and_monitoring.test.ts b/test/integration/server-discovery-and-monitoring/server_discover_and_monitoring.test.ts index 51f3f986f8..acd70aac88 100644 --- a/test/integration/server-discovery-and-monitoring/server_discover_and_monitoring.test.ts +++ b/test/integration/server-discovery-and-monitoring/server_discover_and_monitoring.test.ts @@ -17,22 +17,19 @@ describe('SDAM Unified Tests (Node Driver)', function () { describe('Monitoring rtt tests', function () { let client: MongoClient; let windows: Record; - const THRESH = 200; // Wait for 100 total heartbeats. This is high enough to work for standalone, sharded and our typical 3-node replica set topology tests + const THRESH = 200; // Wait for 200 total heartbeats. This is high enough to work for standalone, sharded and our typical 3-node replica set topology tests const SAMPLING_WINDOW_SIZE = 10; let count: number; const ee = new EventEmitter(); const listener = (ev: ServerHeartbeatSucceededEvent) => { - try { - // @ts-expect-error accessing private fields - const rttSampler = client.topology.s.servers.get(ev.connectionId).monitor.rttSampler; - // @ts-expect-error accessing private fields - const rttSamples = rttSampler.rttSamples; - windows[ev.connectionId].push(Array.from(rttSamples)); - count++; - } catch { - // silently ignore when servers aren't yet populated - } + if (!client.topology.s.servers.has(ev.connectionId)) return; + // @ts-expect-error accessing private fields + const rttSampler = client.topology.s.servers.get(ev.connectionId).monitor.rttSampler; + // @ts-expect-error accessing private fields + const rttSamples = rttSampler.rttSamples; + windows[ev.connectionId].push(Array.from(rttSamples)); + count++; if (count === SAMPLING_WINDOW_SIZE) { ee.emit('samplingWindowFilled'); @@ -62,9 +59,11 @@ describe('Monitoring rtt tests', function () { connectTimeoutMS: 1000, serverMonitoringMode }); - client.on('serverHeartbeatSucceeded', listener); await client.connect(); + //await client.db('test').admin().ping(); + + client.on('serverHeartbeatSucceeded', listener); for (const k of client.topology.s.servers.keys()) { windows[k] = []; From 4126873a1e8cb3ebf77f839f968ba159981cbb06 Mon Sep 17 00:00:00 2001 From: Warren James Date: Tue, 14 May 2024 16:58:43 -0400 Subject: [PATCH 08/12] finalize tests --- .../server_discover_and_monitoring.test.ts | 115 ++++++++++++------ 1 file changed, 75 insertions(+), 40 deletions(-) diff --git a/test/integration/server-discovery-and-monitoring/server_discover_and_monitoring.test.ts b/test/integration/server-discovery-and-monitoring/server_discover_and_monitoring.test.ts index acd70aac88..2799af3bef 100644 --- a/test/integration/server-discovery-and-monitoring/server_discover_and_monitoring.test.ts +++ b/test/integration/server-discovery-and-monitoring/server_discover_and_monitoring.test.ts @@ -1,8 +1,10 @@ import { EventEmitter, once } from 'node:events'; +import { setTimeout } from 'node:timers/promises'; import { expect } from 'chai'; +import * as sinon from 'sinon'; -import { type MongoClient, type ServerHeartbeatSucceededEvent } from '../../mongodb'; +import { Connection, type MongoClient, type ServerHeartbeatSucceededEvent } from '../../mongodb'; import { loadSpecTests } from '../../spec'; import { runUnifiedSuite } from '../../tools/unified-spec-runner/runner'; @@ -16,27 +18,29 @@ describe('SDAM Unified Tests (Node Driver)', function () { describe('Monitoring rtt tests', function () { let client: MongoClient; - let windows: Record; - const THRESH = 200; // Wait for 200 total heartbeats. This is high enough to work for standalone, sharded and our typical 3-node replica set topology tests - const SAMPLING_WINDOW_SIZE = 10; + let heartbeatDurations: Record; + const HEARTBEATS_TO_COLLECT = 200; // Wait for 200 total heartbeats. This is high enough to work for standalone, sharded and our typical 3-node replica set topology tests + const IGNORE_SIZE = 20; + const DELAY_MS = 10; let count: number; const ee = new EventEmitter(); const listener = (ev: ServerHeartbeatSucceededEvent) => { if (!client.topology.s.servers.has(ev.connectionId)) return; - // @ts-expect-error accessing private fields - const rttSampler = client.topology.s.servers.get(ev.connectionId).monitor.rttSampler; - // @ts-expect-error accessing private fields - const rttSamples = rttSampler.rttSamples; - windows[ev.connectionId].push(Array.from(rttSamples)); count++; + if (count < IGNORE_SIZE) { + return; + } - if (count === SAMPLING_WINDOW_SIZE) { - ee.emit('samplingWindowFilled'); + heartbeatDurations[ev.connectionId].push(ev.duration); + + // We ignore the first few heartbeats since the problem reported in NODE-6172 showed that the + // first few heartbeats were recorded properly + if (count === IGNORE_SIZE) { return; } - if (count >= THRESH) { + if (count >= HEARTBEATS_TO_COLLECT + IGNORE_SIZE) { client.off('serverHeartbeatSucceeded', listener); ee.emit('done'); } @@ -44,48 +48,79 @@ describe('Monitoring rtt tests', function () { beforeEach(function () { count = 0; - windows = {}; + heartbeatDurations = {}; }); afterEach(async function () { - await client?.close(); + if (client) { + await client.close(); + } + sinon.restore(); }); for (const serverMonitoringMode of ['poll', 'stream']) { context(`when serverMonitoringMode is set to '${serverMonitoringMode}'`, function () { - beforeEach(async function () { - client = this.configuration.newClient({ - heartbeatFrequencyMS: 100, - connectTimeoutMS: 1000, - serverMonitoringMode - }); - - await client.connect(); - //await client.db('test').admin().ping(); + context('after collecting a number of heartbeats', function () { + beforeEach(async function () { + client = this.configuration.newClient({ + heartbeatFrequencyMS: 100, + serverMonitoringMode + }); - client.on('serverHeartbeatSucceeded', listener); + // make send command delay for DELAY_MS ms to ensure that the actual time between sending + // a heartbeat and receiving a response don't drop below 1ms. This is done since our + // testing is colocated with its mongo deployment so network latency is very low + const stub = sinon + // @ts-expect-error accessing private method + .stub(Connection.prototype, 'sendCommand') + .callsFake(async function* (...args) { + await setTimeout(DELAY_MS); + yield* stub.wrappedMethod.call(this, ...args); + }); + await client.connect(); - for (const k of client.topology.s.servers.keys()) { - windows[k] = []; - } + client.on('serverHeartbeatSucceeded', listener); - await once(ee, 'samplingWindowFilled'); - }); + for (const k of client.topology.s.servers.keys()) { + heartbeatDurations[k] = []; + } - it('rttSampler does not accumulate 0 rtt', { - metadata: { - requires: { topology: '!load-balanced' } - }, - test: async function () { await once(ee, 'done'); - for (const s in windows) { - // Test that at every point we collect a heartbeat, the rttSampler is not filled with - // zeroes - for (const window of windows[s]) { - expect(window.reduce((acc, x) => x + acc)).to.be.greaterThan(0); + }); + + it( + 'heartbeat duration is not incorrectly reported as zero on ServerHeartbeatSucceededEvents', + { + metadata: { + requires: { topology: '!load-balanced' } + }, + test: async function () { + for (const server in heartbeatDurations) { + const averageDuration = + heartbeatDurations[server].reduce((acc, x) => acc + x) / + heartbeatDurations[server].length; + expect(averageDuration).to.be.gt(DELAY_MS); + } } } - } + ); + + it('ServerDescription.roundTripTime is not incorrectly reported as zero', { + metadata: { + requires: { topology: '!load-balanced' } + }, + test: async function () { + await once(ee, 'done'); + for (const server in heartbeatDurations) { + const averageDuration = + heartbeatDurations[server].reduce((acc, x) => acc + x) / + heartbeatDurations[server].length; + expect( + client.topology.description.servers.get(server).roundTripTime + ).to.be.approximately(averageDuration, 1); + } + } + }); }); }); } From 71d414ea287f3f766cc24aa9cb37eab730c0b1b8 Mon Sep 17 00:00:00 2001 From: Warren James Date: Wed, 15 May 2024 12:11:24 -0400 Subject: [PATCH 09/12] remove comment --- src/sdam/monitor.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/sdam/monitor.ts b/src/sdam/monitor.ts index 1b70359730..baf75b6332 100644 --- a/src/sdam/monitor.ts +++ b/src/sdam/monitor.ts @@ -220,7 +220,7 @@ export class Monitor extends TypedEventEmitter { } get latestRtt(): number | null { - return this.rttSampler.last; // FIXME: Check if this is acceptable + return this.rttSampler.last; } addRttSample(rtt: number) { From 7dbe7a59015d39fe4493be655aac90523186e293 Mon Sep 17 00:00:00 2001 From: Warren James Date: Wed, 15 May 2024 12:11:40 -0400 Subject: [PATCH 10/12] fix hanging test --- .../server_discover_and_monitoring.test.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/test/integration/server-discovery-and-monitoring/server_discover_and_monitoring.test.ts b/test/integration/server-discovery-and-monitoring/server_discover_and_monitoring.test.ts index 2799af3bef..ff5dfb2351 100644 --- a/test/integration/server-discovery-and-monitoring/server_discover_and_monitoring.test.ts +++ b/test/integration/server-discovery-and-monitoring/server_discover_and_monitoring.test.ts @@ -110,7 +110,6 @@ describe('Monitoring rtt tests', function () { requires: { topology: '!load-balanced' } }, test: async function () { - await once(ee, 'done'); for (const server in heartbeatDurations) { const averageDuration = heartbeatDurations[server].reduce((acc, x) => acc + x) / From 83f65460fc519c60562a1c47703072bdbc375eba Mon Sep 17 00:00:00 2001 From: Warren James Date: Wed, 15 May 2024 13:27:51 -0400 Subject: [PATCH 11/12] clean up tests --- .../server_discover_and_monitoring.test.ts | 83 +++++++++---------- 1 file changed, 40 insertions(+), 43 deletions(-) diff --git a/test/integration/server-discovery-and-monitoring/server_discover_and_monitoring.test.ts b/test/integration/server-discovery-and-monitoring/server_discover_and_monitoring.test.ts index ff5dfb2351..6150b5e693 100644 --- a/test/integration/server-discovery-and-monitoring/server_discover_and_monitoring.test.ts +++ b/test/integration/server-discovery-and-monitoring/server_discover_and_monitoring.test.ts @@ -1,10 +1,14 @@ -import { EventEmitter, once } from 'node:events'; import { setTimeout } from 'node:timers/promises'; import { expect } from 'chai'; import * as sinon from 'sinon'; -import { Connection, type MongoClient, type ServerHeartbeatSucceededEvent } from '../../mongodb'; +import { + Connection, + type MongoClient, + promiseWithResolvers, + type ServerHeartbeatSucceededEvent +} from '../../mongodb'; import { loadSpecTests } from '../../spec'; import { runUnifiedSuite } from '../../tools/unified-spec-runner/runner'; @@ -19,36 +23,12 @@ describe('SDAM Unified Tests (Node Driver)', function () { describe('Monitoring rtt tests', function () { let client: MongoClient; let heartbeatDurations: Record; - const HEARTBEATS_TO_COLLECT = 200; // Wait for 200 total heartbeats. This is high enough to work for standalone, sharded and our typical 3-node replica set topology tests - const IGNORE_SIZE = 20; + const HEARTBEATS_TO_COLLECT_PER_NODE = 65; + const IGNORE_SIZE = 5; const DELAY_MS = 10; - let count: number; - const ee = new EventEmitter(); - - const listener = (ev: ServerHeartbeatSucceededEvent) => { - if (!client.topology.s.servers.has(ev.connectionId)) return; - count++; - if (count < IGNORE_SIZE) { - return; - } - - heartbeatDurations[ev.connectionId].push(ev.duration); - - // We ignore the first few heartbeats since the problem reported in NODE-6172 showed that the - // first few heartbeats were recorded properly - if (count === IGNORE_SIZE) { - return; - } - - if (count >= HEARTBEATS_TO_COLLECT + IGNORE_SIZE) { - client.off('serverHeartbeatSucceeded', listener); - ee.emit('done'); - } - }; beforeEach(function () { - count = 0; - heartbeatDurations = {}; + heartbeatDurations = Object.create(null); }); afterEach(async function () { @@ -67,7 +47,7 @@ describe('Monitoring rtt tests', function () { serverMonitoringMode }); - // make send command delay for DELAY_MS ms to ensure that the actual time between sending + // make sendCommand delay for DELAY_MS ms to ensure that the actual time between sending // a heartbeat and receiving a response don't drop below 1ms. This is done since our // testing is colocated with its mongo deployment so network latency is very low const stub = sinon @@ -79,13 +59,28 @@ describe('Monitoring rtt tests', function () { }); await client.connect(); - client.on('serverHeartbeatSucceeded', listener); - - for (const k of client.topology.s.servers.keys()) { - heartbeatDurations[k] = []; - } - - await once(ee, 'done'); + const { promise, resolve } = promiseWithResolvers(); + client.on('serverHeartbeatSucceeded', (ev: ServerHeartbeatSucceededEvent) => { + heartbeatDurations[ev.connectionId] ??= []; + if ( + heartbeatDurations[ev.connectionId].length < + HEARTBEATS_TO_COLLECT_PER_NODE + IGNORE_SIZE + ) + heartbeatDurations[ev.connectionId].push(ev.duration); + + // We ignore the first few heartbeats since the problem reported in NODE-6172 showed that the + // first few heartbeats were recorded properly + if ( + Object.keys(heartbeatDurations).length === client.topology.s.servers.size && + Object.values(heartbeatDurations).every( + d => d.length === HEARTBEATS_TO_COLLECT_PER_NODE + IGNORE_SIZE + ) + ) { + client.removeAllListeners('serverHeartbeatSucceeded'); + resolve(); + } + }); + await promise; }); it( @@ -95,10 +90,11 @@ describe('Monitoring rtt tests', function () { requires: { topology: '!load-balanced' } }, test: async function () { - for (const server in heartbeatDurations) { + for (const durations of Object.values(heartbeatDurations)) { + const relevantDurations = durations.slice(IGNORE_SIZE); + expect(relevantDurations).to.have.length.gt(0); const averageDuration = - heartbeatDurations[server].reduce((acc, x) => acc + x) / - heartbeatDurations[server].length; + relevantDurations.reduce((acc, x) => acc + x) / relevantDurations.length; expect(averageDuration).to.be.gt(DELAY_MS); } } @@ -110,10 +106,11 @@ describe('Monitoring rtt tests', function () { requires: { topology: '!load-balanced' } }, test: async function () { - for (const server in heartbeatDurations) { + for (const [server, durations] of Object.entries(heartbeatDurations)) { + const relevantDurations = durations.slice(IGNORE_SIZE); + expect(relevantDurations).to.have.length.gt(0); const averageDuration = - heartbeatDurations[server].reduce((acc, x) => acc + x) / - heartbeatDurations[server].length; + relevantDurations.reduce((acc, x) => acc + x) / relevantDurations.length; expect( client.topology.description.servers.get(server).roundTripTime ).to.be.approximately(averageDuration, 1); From 9c83722db5ac29ff5f7b5311e9bb21c3376b3607 Mon Sep 17 00:00:00 2001 From: Warren James Date: Wed, 15 May 2024 13:33:36 -0400 Subject: [PATCH 12/12] loosen approximate assertion --- .../server_discover_and_monitoring.test.ts | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/test/integration/server-discovery-and-monitoring/server_discover_and_monitoring.test.ts b/test/integration/server-discovery-and-monitoring/server_discover_and_monitoring.test.ts index 6150b5e693..81006928e9 100644 --- a/test/integration/server-discovery-and-monitoring/server_discover_and_monitoring.test.ts +++ b/test/integration/server-discovery-and-monitoring/server_discover_and_monitoring.test.ts @@ -111,9 +111,9 @@ describe('Monitoring rtt tests', function () { expect(relevantDurations).to.have.length.gt(0); const averageDuration = relevantDurations.reduce((acc, x) => acc + x) / relevantDurations.length; - expect( - client.topology.description.servers.get(server).roundTripTime - ).to.be.approximately(averageDuration, 1); + const rtt = client.topology.description.servers.get(server).roundTripTime; + expect(rtt).to.not.equal(0); + expect(rtt).to.be.approximately(averageDuration, 3); } } });