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

DRIVERS-2677: clarify when ServerHeartbeatStartedEvent should be emitted #1483

Merged
Show file tree
Hide file tree
Changes from 23 commits
Commits
Show all changes
29 commits
Select commit Hold shift + click to select a range
c5deb51
DRIVERS-2677: clarify heartbeat started event ordering and field requ…
W-A-James Dec 14, 2023
00304ba
DRIVERS-2677 Clarify ordering of ServerHeartbeatStartedEvent and moni…
W-A-James Jan 5, 2024
a57b4b3
Correct typo
W-A-James Jan 5, 2024
e6a59c7
Merge branch 'mongodb:master' into DRIVERS-2677/clarify-heartbeat-sta…
W-A-James Jan 5, 2024
5e186b5
Update wording on undefined value
W-A-James Jan 5, 2024
d6bc209
Merge branch 'DRIVERS-2677/clarify-heartbeat-started-event-ordering' …
W-A-James Jan 5, 2024
879e676
Update wording on undefined value
W-A-James Jan 5, 2024
fd946d2
add rationale
W-A-James Jan 5, 2024
6372ba3
revise ordering
W-A-James Jan 5, 2024
5816c83
Update spacing
W-A-James Jan 5, 2024
3bc7921
Add prose test in progress
W-A-James Jan 5, 2024
c448f03
formatting
W-A-James Jan 5, 2024
b767798
formatting
W-A-James Jan 5, 2024
cedb0a2
formatting
W-A-James Jan 5, 2024
f236489
formatting
W-A-James Jan 5, 2024
e1d798d
ordering
W-A-James Jan 5, 2024
8d1b6be
add new prose test
W-A-James Jan 9, 2024
1279fa8
remove old prose test
W-A-James Jan 9, 2024
434acab
remove poolSize settings
W-A-James Jan 9, 2024
9ec5ddf
Fix wording and remove unnecessary config
W-A-James Jan 9, 2024
d8ecff3
refine wording
W-A-James Jan 9, 2024
ee6c986
strengthen wording
W-A-James Jan 9, 2024
1072e22
remove backticks
W-A-James Jan 9, 2024
6b93f0c
Reorder events
W-A-James Jan 10, 2024
6b0e0ee
capitalize
W-A-James Jan 10, 2024
c6cc625
Fix spelling and update prose test
W-A-James Jan 11, 2024
9d33a89
Remove section on serverConnectionId
W-A-James Jan 11, 2024
ac2e8bc
Remove references to serverConnectionId
W-A-James Jan 12, 2024
8969f1f
update prose tests
W-A-James Jan 16, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -138,7 +138,7 @@ Events that MUST be published (with their conditions) are as follows.
* - ``TopologyClosedEvent``
- When a topology is shut down - this MUST be the last SDAM event fired.
* - ``ServerHeartbeatStartedEvent``
- Published when the server monitor sends its ``hello`` or legacy hello call to the server.
- Published when the server monitor sends its ``hello`` or legacy hello call to the server. When the monitor is creating a new connection, this event MUST be published just after the socket is created and just before the ``hello`` or legacy hello.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this right? I don't think this is what we do in Node. we emit heartbeat started events before establishing a socket (before a call to connect)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shane and I are discussing this further, but our initial decision was to do what we currently do in the Node driver, but this does make it so that the the duration on the first ServerHeartbeatSucceededEvent would include the time taken to complete the TCP handshake which adds at least another network round trip, artificially inflating the RTT since that gets calculated from the aforementioned duration field.

How I currently have this written would avoid this, but would require changes in the Node driver among others.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Update: Will change this to expect the ServerHeartbeatStartedEvent before connection creation. There was confusion on my end as I thought we had to measure the start time as when the ServerHeartbeatStartedEvent was emitted. On Node's end this will require still require us to change how we implement our monitoring since we currently measure the heartbeat duration starting as mentioned above.

* - ``ServerHeartbeatSucceededEvent``
- Published on successful completion of the server monitor's ``hello`` or legacy hello call.
* - ``ServerHeartbeatFailedEvent``
Expand Down Expand Up @@ -251,6 +251,8 @@ Events that MUST be published (with their conditions) are as follows.
/**
* Fired when the server monitor's ``hello`` or legacy hello command is started - immediately before
* the ``hello`` or legacy hello command is serialized into raw BSON and written to the socket.
* When the monitor is creating a new monitoring connection, this event is fired just after the
* socket is opened.
*/
interface ServerHeartbeatStartedEvent {

Expand Down Expand Up @@ -462,7 +464,7 @@ The following key-value pairs are common to all or several log messages and MUST
* - serverConnectionId
- Heartbeat-related log messages
- Int
- The server's ID for the monitoring connection, if known. This value will be unknown and can be omitted in certain cases, e.g. the first
- The server's ID for the monitoring connection, if known. This value will be unknown and MUST be omitted in certain cases, e.g. the first
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should also be pushed out to the other ticket.

"heartbeat started" message for a monitoring connection. Only present on server versions 4.2+.

"Starting Topology Monitoring" Log Message
Expand Down Expand Up @@ -696,6 +698,7 @@ See the `README <https://github.com/mongodb/specifications/server-discovery-and-
Changelog
=========

:2024-01-04: Updated to clarify when ServerHeartbeatStartedEvent should be emitted and when the serverConnectionId should be undefined.
:2023-03-31: Renamed to include "logging" in the title. Reorganized contents and made consistent with CLAM spec, and added requirements
for SDAM log messages.
:2022-10-05: Remove spec front matter and reformat changelog.
Expand Down
26 changes: 26 additions & 0 deletions source/server-discovery-and-monitoring/tests/README.rst
Original file line number Diff line number Diff line change
Expand Up @@ -262,6 +262,32 @@ Run the following test(s) on MongoDB 4.4+.
mode: "off",
});

Heartbeat Tests
~~~~~~~~~~~~~~~

1. Test that ``ServerHeartbeatStartedEvent`` is emitted after the monitoring socket was created and before the ``hello`` is sent to the server
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

emitted after the monitoring socket was created
This is the opposite of what the current spec says.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch


#. Create a mock TCP server (example shown below) that pushes a ``client connected`` event to a shared array when a client connects and a ``client hello received`` event when the server receives the client hello and then closes the connection::

let events = [];
server = createServer(clientSocket => {
events.push('client connected');

clientSocket.on('data', () => {
events.push('client hello received');
clientSocket.end();
});
});
server.listen(9999);

#. Create a client with ``serverSelectionTimeoutMS: 500`` and listen to ``ServerHeartbeatStartedEvent`` and ``ServerHeartbeatFailedEvent``, pushing the event name to the same shared array as the mock TCP server.

#. Attempt to connect client to previously created TCP server, catching the error when the client fails to connect

#. Assert that the event array has the following contents in the same order::
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since SDAM can try to connect multiple times, I would say:
"Assert that the first two events in the array are::"


['client connected', 'serverHeartbeatStartedEvent', 'client hello received', 'serverHeartbeatFailedEvent']

.. Section for links.

.. _Server Description Equality: /source/server-discovery-and-monitoring/server-discovery-and-monitoring.rst#server-description-equality
Loading