Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 23 commits into from
Apr 3, 2024

Conversation

W-A-James
Copy link
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

src/sdam/monitor.ts Outdated Show resolved Hide resolved
@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
src/sdam/monitor.ts Outdated Show resolved Hide resolved
src/sdam/monitor.ts Outdated Show resolved Hide resolved
@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
src/sdam/server_description.ts Show resolved Hide resolved
src/cmap/connection.ts Outdated Show resolved Hide resolved
src/sdam/monitor.ts Outdated Show resolved Hide resolved
src/sdam/monitor.ts Outdated Show resolved Hide resolved
src/sdam/monitor.ts Outdated Show resolved Hide resolved
src/utils.ts Outdated Show resolved Hide resolved
src/utils.ts Outdated Show resolved Hide resolved
src/utils.ts Outdated Show resolved Hide resolved
test/unit/sdam/monitor.test.ts Outdated Show resolved Hide resolved
test/unit/sdam/monitor.test.ts Outdated Show resolved Hide resolved
src/utils.ts Outdated Show resolved Hide resolved
src/utils.ts Outdated Show resolved Hide resolved
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