fix(NODE-5785): do not deserialize heartbeat events with promoteBuffers set to true
#4063
Conversation
baileympearson
changed the title
promoteBuffers to false when monitoringpromoteBuffers set to true
W-A-James
added
the
Primary Review
| requires: { | ||
| topology: ['replicaset', 'sharded'] | ||
| } |
There was a problem hiding this comment.
Any reason not to run these on standalone as well?
There was a problem hiding this comment.
The $clusterTime field isn't present on heartbeats from standalone servers
W-A-James
added
Team Review
|
|
||
| await client.connect(); | ||
| // give the driver a chance to connect to all servers and collect some heartbeats | ||
| await setTimeout(100); |
There was a problem hiding this comment.
LGTM, but just a knowledge question, why is 100ms a sufficient time?
There was a problem hiding this comment.
When we connect, we send initial hello (heartbeat) messages to each server and collect the responses. Since the test is running directly alongside the server, we're expecting the network latency to be very low ~10 ms
There was a problem hiding this comment.
Also because you might notice this sleep is after the awaited connect. Connect will (by default) resolve when the driver finds a primary. That means that connect can resolve before connecting to secondaries, arbiters or other servers.
|
What is the public-facing fix here? |
@nbbeeken server heartbeat events I guess. Are you thinking this is a "refactor" instead? |
|
Oh I see, ServerHeartbeatSucceededEvent's |
My thought is no - because we type the |
|
I mentioned the fact that it changes with server version/topology because I think that it makes sense that is the only way |
| @@ -135,7 +135,7 @@ export class Monitor extends TypedEventEmitter<MonitorEvents> { | |||
| useBigInt64: false, | |||
| promoteLongs: true, | |||
| promoteValues: true, | |||
| promoteBuffers: true | |||
| promoteBuffers: false | |||
There was a problem hiding this comment.
Leaving a comment for viz, discussing public API concerns on slack.
Description
What is changing?
This PR sets
promoteBuffersto false for the monitor class. This change is almost entirely invisible to users because monitors are not publicly exposed.Server heartbeat events and log message are impacted by this change, but we don't currently include the types of each field in the server's heartbeat as a part of our semver-supported API.
Is there new documentation needed for these changes?
No.
What is the motivation for this change?
Setting
promoteBuffers: trueincreases the size of an EJSON.stringified heartbeat event, increasing the likelihood that log messages are truncated.Double check the following
npm run check:lintscripttype(NODE-xxxx)[!]: descriptionfeat(NODE-1234)!: rewriting everything in coffeescript