Skip to content

feat(NODE-5825): add minRoundTripTime to ServerDescription and change roundTripTime to a moving average#4059

Merged
nbbeeken merged 23 commits into
mainfrom
NODE-5825/add-minRTT-to-Monitor
Apr 3, 2024
Merged

feat(NODE-5825): add minRoundTripTime to ServerDescription and change roundTripTime to a moving average#4059
nbbeeken merged 23 commits into
mainfrom
NODE-5825/add-minRTT-to-Monitor

Conversation

@W-A-James
Copy link
Copy Markdown
Contributor

@W-A-James W-A-James commented Mar 27, 2024

Description

What is changing?

Monitor changes
  • Add rttSamplesMS internal field
  • Add roundTripTime getter
  • Add latestRTT getter
  • add addRttSample method
  • Add clearRttSamples method and call in resetMonitorState
  • Add RTTSampler class to implement spec-defined minimum and average RTT calculations
RTTPinger changes
  • Add monitor field to reference parent Monitor
  • Add latestRTT field, initialized to the the monitor's latestRTT value
  • Drive by cleanup: refactor measureAndReschedule to use fewer closures and clarify control flow
ServerDescription changes
  • Add minRoundTripTime field
  • Add roundTripTime field

##### Connection changes
- Add monitor field to connection to facilitate plumbing of minRoundTrip time to connection layer

Is there new documentation needed for these changes?
  • No

What is the motivation for this change?

Release Highlight

ServerDescription Round Trip Time (RTT) measurement changes

ServerDescription.roundTripTime is now a moving average

Previously, ServerDescription.roundTripTime was calculated as a weighted average of the most recently observed heartbeat duration and the previous duration. This update changes this behaviour to average ServerDescription.roundTripTime over the last 10 observed heartbeats. This should reduce the likelihood that the selected server changes as a result of momentary spikes in server latency.

Added minRoundTripTime to ServerDescription

A new minRoundTripTime property is now available on the ServerDescription class which gives the minimum rtt over the last 10 heartbeats. Note that this value will be reported as 0 when fewer than 2 samples have been obvserved.

Double check the following

  • Ran npm run check:lint script
  • Self-review completed using the steps outlined here
  • PR title follows the correct format: type(NODE-xxxx)[!]: description
    • Example: feat(NODE-1234)!: rewriting everything in coffeescript
  • Changes are covered by tests
  • New TODOs have a related JIRA ticket

Comment thread src/sdam/monitor.ts Outdated
@W-A-James W-A-James force-pushed the NODE-5825/add-minRTT-to-Monitor branch from 87e84e7 to 63d7b52 Compare March 29, 2024 19:47
Comment thread src/sdam/server_selection.ts
Comment thread src/sdam/monitor.ts Outdated
Comment thread src/sdam/server.ts
Comment thread src/sdam/monitor.ts Outdated
@W-A-James W-A-James marked this pull request as ready for review April 1, 2024 19:57
@nbbeeken nbbeeken self-assigned this Apr 1, 2024
@nbbeeken nbbeeken added the Primary Review In Review with primary reviewer, not yet ready for team's eyes label Apr 1, 2024
@nbbeeken nbbeeken self-requested a review April 1, 2024 19:58
Comment thread src/sdam/server_description.ts
Comment thread src/cmap/connection.ts Outdated
Comment thread src/sdam/monitor.ts Outdated
Comment thread src/sdam/monitor.ts Outdated
Comment thread src/sdam/monitor.ts Outdated
Comment thread src/utils.ts Outdated
Comment thread src/utils.ts Outdated
Comment thread src/utils.ts Outdated
Comment thread test/unit/sdam/monitor.test.ts Outdated
Comment thread test/unit/sdam/monitor.test.ts Outdated
Comment thread src/utils.ts Outdated
Comment thread src/utils.ts Outdated
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Primary Review In Review with primary reviewer, not yet ready for team's eyes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants