DRIVERS-2875: add support for TopologyDescriptionChangedEvent to expectEvents #1556

W-A-James · 2024-03-25T02:59:17Z

Updated unified test format spec to detail how assertions on topologyDescriptionChangedEvents should be done
Updated schema to support this spec change
Added valid pass test for addition to schema
Bumped schema version in makefile for unified test runner

Please complete the following before merging:

Update changelog.
Make sure there are generated JSON files from the YAML test files.
Test changes in at least one language driver.
Test these changes against all server versions and topologies (including standalone, replica set, sharded
clusters, and serverless).

W-A-James · 2024-03-26T01:35:45Z

See changes made to the test runner implementation in this PR

...ied-test-format/tests/valid-pass/expectedEventsForClient-topologyDescriptionChangedEvent.yml

jmikola · 2024-03-27T14:54:30Z

...ied-test-format/tests/valid-pass/expectedEventsForClient-topologyDescriptionChangedEvent.yml

+          - topologyDescriptionChangedEvent: {} # unknown -> replset no primary
+          - topologyDescriptionChangedEvent: {} # server connected
+          - topologyDescriptionChangedEvent: {} # server connected
+          - topologyDescriptionChangedEvent: {} # server connected


Is this an implicit assumption that the replica set has exactly three members? If so, that should probably be clearly called out in a comment.

I'm not familiar with any of the CMAP and SDAM tests, so if this assumption is blindly made elsewhere then feel free to take no action here.

With the way we set up our replica-sets for testing via drivers-evergreen-tools, yes this is the assumption we generally make. See also the replica set sdam logging tests.

jmikola · 2024-03-27T15:08:39Z

...ied-test-format/tests/valid-pass/expectedEventsForClient-topologyDescriptionChangedEvent.yml

+        ignoreExtraEvents: false
+        events:
+          - topologyOpeningEvent: {}
+          - topologyDescriptionChangedEvent: {} # unknown -> replset no primary


Why doesn't this assert that the description goes from "Unknown" to "ReplicaSetWithPrimary"? If that's possible to assert, then you can potentially stop the entire test after the waitForEvent operation. Add ignoreExtraEvents: true and avoid closing the client entity.

In that case, you could avoid any assumptions about replica set size and subsequent event assertions. That might also make the test more resilient to variations in SDAM monitoring (not every driver uses awaitable hello).

I was initially curious why the TopologyDescription starts as "Unknown", but verified that locally with the PHP driver.

I think it'd also be helpful to call out why the TopologyDescription starts as "Unknown".

Initial TopologyType in the SDAM spec suggests that it would start as "ReplicaSetNoPrimary" (if we assume the URI includes replicaSet).

When I ran this locally with PHP, I did observe an initial "Unknown" state for the previous TopologyDescription; however, I don't see where that's mandated in the spec. Initial ServerDescription in the SDAM Logging/Monitoring spec notes why ServerDescription is initialized to "Unknown" (to avoid a null value in the previous description). I assume that same applies to TopologyDescription. Is that specified anywhere? If not, I'd propose reporting a ticket to get that clarified -- or working it into this ticket and having an SDAM spec owner review.

Good catch here on the Initial Topology type. I think the wording of the spec here is pretty clear as it relates to the Initial Topology type, I just happened to miss it when working on this, so I don't think we should assume it always starts as UNKNOWN.
The part that's not entirely clear is whether or not the replicaSet option should be required in the URI when running unified tests against replica sets. It seems that it should be when doing these kinds of spec tests given that, to my knowledge, pretty much all drivers make use of drivers-evergreen-tools scripts for their cluster setups in CI, but that's not specified anywhere. Do you think that's a change I should make here?

That being said, I'm in favour of changing that spec test here to expect this replica set to start with ReplicaSetNoPrimary.

I'm not convinced that Initial TopologyType applies to SDAM monitoring events. If it does, then there is evidently an issue with libmongoc.

What did you observe in your driver?

...but that's not specified anywhere. Do you think that's a change I should make here?

I don't see much benefit in doing so, as it'd likely be ignored. There's presently nothing in the unified test format spec that talks about a well-formed URI, and that's not really in the scope of the test format anyway since it'd mainly impact other spec tests (excluding the "valid" test you're cooking up here).

I think it's more reasonable to just document the assumption in the test file here, such that it'd be easier for someone to diagnose a potential failure down the line.

Our driver has the previousDescription hardcoded to UNKNOWN on the first TopologyDescriptionChangedEvent to be emitted.

Why wouldn't the initial TopologyType apply to SDAM monitoring events though? Aren't the monitoring events supposed to give a view of the internal view of the Topology? If so, then it would make sense for them to set the previousDescription field of the first TopologyDescriptionChangedEvent to the internal view of the topology that we get from the connection string.

@jmikola Talked to Shane about it and seems it's just something we left off of the sdam monitoring spec. Will add that clarification in this PR.

Why wouldn't the initial TopologyType apply to SDAM monitoring events though?

Closing the loop here. My point was that Initial TopologyType specifies what the initial topology should be but makes no mention of the "Unknown" state that is used for the previousDescription field of the first emitted TopologyDescriptionChangedEvent. The rules for previousDescription seems to exist entirely within the SDAM Monitoring spec.

I wasn't debating whether that's correct or not; merely pointing out that the main SDAM spec doesn't address this.

…lient-topologyDescriptionChangedEvent.yml Co-authored-by: Jeremy Mikola <[email protected]>

…f github.com:W-A-James/specifications into support-topology-description-changed-in-expectEvents

ShaneHarvey · 2024-04-05T18:56:52Z

...ied-test-format/tests/valid-pass/expectedEventsForClient-topologyDescriptionChangedEvent.yml

+        eventType: sdam
+        ignoreExtraEvents: true 
+        events:
+          - topologyOpeningEvent: {}


The test sets observeEvents: [topologyDescriptionChangedEvent] so this topologyOpeningEvent should be omitted.

ShaneHarvey · 2024-04-05T19:46:29Z

...ied-test-format/tests/valid-pass/expectedEventsForClient-topologyDescriptionChangedEvent.yml

+              previousDescription:
+                type: "Unknown"
+              newDescription:
+                type: "ReplicaSetWithPrimary"


When I run this test in Python, this first event is ReplicaSetNoPrimary, not ReplicaSetWithPrimary:

<TopologyDescriptionChangedEvent topology_id: 66104eba924530e675e287ae changed from: <TopologyDescription id: 66104eba924530e675e287ae, topology_type: Unknown, servers: []>, to: <TopologyDescription id: 66104eba924530e675e287ae, topology_type: ReplicaSetNoPrimary, servers: [<ServerDescription ('localhost', 27019) server_type: Unknown, rtt: None>]>>

We do this when creating the client because in this case we know ahead of time we are connecting to a replica set (because our test suite adds the replicaSet option.

How about we add "directConnection": true to the client and use type: Single?

ShaneHarvey

LGTM! Python tests are passing here: https://spruce.mongodb.com/version/66107586f9da6e00077e3ff1/tasks?sorts=STATUS%3AASC%3BBASE_STATUS%3ADESC

...e/server-discovery-and-monitoring/server-discovery-and-monitoring-logging-and-monitoring.rst

jmikola · 2024-04-09T14:49:59Z

...ied-test-format/tests/valid-pass/expectedEventsForClient-topologyDescriptionChangedEvent.yml

+            - client:
+                id: &client client
+                uriOptions:
+                  directConnection: true


Assuming the cluster is launched with mongo-orchestration, wouldn't we expect the connection string to include multiple seeds and a replicaSet option?

I expect the multiple seeds would conflict with directConnection, as mentioned in directConnection URI option with multiple seeds or SRV URI.

@ShaneHarvey how did this pass in Python?

The python test suite helper to create a MongoClient always connects with a single seed and only adds the replicaSet option if directConnection is not given (or False):
https://github.com/mongodb/mongo-python-driver/blob/e37394d4029d61f72a02dc465fbfc9afe35a5701/test/utils.py#L570-L579

always connects with a single seed and only adds the replicaSet option if directConnection is not given (or False)

This sounds like something that needs to be clarified in the Unified Test Format spec, when we talk about feeding the test runner with a connection string. I imagine these may be the first unified tests with directConnection, so this hasn't come up before.

…nitoring-logging-and-monitoring.rst Co-authored-by: Jeremy Mikola <[email protected]>

jmikola · 2024-04-16T13:16:07Z

source/unified-test-format/unified-test-format.md

@@ -415,6 +415,11 @@ The structure of this object is as follows:
    notable exception: if `readPreferenceTags` is specified in this object, the key will map to an array of strings,
    each representing a tag set, since it is not feasible to define multiple `readPreferenceTags` keys in the object.

+    Note also that when specifying `directConnection` as true, the connection string used to
+    instantiate a client MUST only have a single seed and MUST NOT specify the `replicaSet` option.
+    See the [`directConnection` specification](../uri-options/uri-options.rst#directconnection-uri-option-with-multiple-seeds-or-srv-uri)


Suggested change

See the [`directConnection` specification](../uri-options/uri-options.rst#directconnection-uri-option-with-multiple-seeds-or-srv-uri)

See the [URI Options spec](../uri-options/uri-options.rst#directconnection-uri-option-with-multiple-seeds-or-srv-uri)

jmikola · 2024-04-16T13:21:09Z

Feel free to merge with @ShaneHarvey's review. I'd just like to see the spec link text changed.

W-A-James added 5 commits March 24, 2024 22:59

add support for TopologyDescriptionChangedEvent to expectEvents

efda7d2

add tests and update schema

4d825d7

fix schema

3863396

lint fixes

16744ba

bump schema version in tests

0a1d448

W-A-James marked this pull request as ready for review March 26, 2024 01:35

W-A-James requested a review from a team as a code owner March 26, 2024 01:35

W-A-James requested review from jmikola and removed request for a team March 26, 2024 01:35

W-A-James changed the title ~~add support for TopologyDescriptionChangedEvent to expectEvents~~ DRIVERS-2875: add support for TopologyDescriptionChangedEvent to expectEvents Mar 26, 2024

jmikola reviewed Mar 27, 2024

View reviewed changes

W-A-James and others added 4 commits March 27, 2024 13:29

Update source/unified-test-format/tests/valid-pass/expectedEventsForC…

da109a0

…lient-topologyDescriptionChangedEvent.yml Co-authored-by: Jeremy Mikola <[email protected]>

Update logging and monitoring spec

fedf41a

update tests

04f9240

Merge branch 'support-topology-description-changed-in-expectEvents' o…

6e85211

…f github.com:W-A-James/specifications into support-topology-description-changed-in-expectEvents

W-A-James requested a review from a team as a code owner March 29, 2024 19:30

W-A-James requested review from tom-selander and ShaneHarvey and removed request for a team and tom-selander March 29, 2024 19:30

fix test

b15fbe7

W-A-James requested a review from jmikola April 3, 2024 15:07

ShaneHarvey requested changes Apr 5, 2024

View reviewed changes

fix test

86f8141

ShaneHarvey mentioned this pull request Apr 5, 2024

PYTHON-4299 Add support for TopologyDescriptionChangedEvent to expectEvents mongodb/mongo-python-driver#1572

Merged

W-A-James requested a review from ShaneHarvey April 5, 2024 20:39

ShaneHarvey approved these changes Apr 5, 2024

View reviewed changes

jmikola reviewed Apr 9, 2024

View reviewed changes

Update source/server-discovery-and-monitoring/server-discovery-and-mo…

a5e4d5f

…nitoring-logging-and-monitoring.rst Co-authored-by: Jeremy Mikola <[email protected]>

update unified test format spec

1f11b5a

W-A-James requested a review from jmikola April 15, 2024 19:01

lint fix

8cb808f

ShaneHarvey approved these changes Apr 15, 2024

View reviewed changes

jmikola reviewed Apr 16, 2024

View reviewed changes

fix link

ec488b1

W-A-James merged commit 688b369 into mongodb:master Apr 17, 2024
4 checks passed

W-A-James deleted the support-topology-description-changed-in-expectEvents branch April 17, 2024 14:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DRIVERS-2875: add support for TopologyDescriptionChangedEvent to expectEvents #1556

DRIVERS-2875: add support for TopologyDescriptionChangedEvent to expectEvents #1556

W-A-James commented Mar 25, 2024 •

edited

Loading

W-A-James commented Mar 26, 2024

jmikola Mar 27, 2024

W-A-James Mar 27, 2024

jmikola Mar 27, 2024 •

edited

Loading

W-A-James Mar 27, 2024

W-A-James Mar 27, 2024

jmikola Mar 27, 2024

W-A-James Mar 28, 2024

W-A-James Mar 28, 2024

W-A-James Mar 29, 2024

jmikola Apr 16, 2024

ShaneHarvey Apr 5, 2024

ShaneHarvey Apr 5, 2024

ShaneHarvey Apr 5, 2024

ShaneHarvey left a comment •

edited

Loading

jmikola Apr 9, 2024

ShaneHarvey Apr 9, 2024

jmikola Apr 9, 2024

jmikola Apr 16, 2024

jmikola commented Apr 16, 2024

	See the [`directConnection` specification](../uri-options/uri-options.rst#directconnection-uri-option-with-multiple-seeds-or-srv-uri)
	See the [URI Options spec](../uri-options/uri-options.rst#directconnection-uri-option-with-multiple-seeds-or-srv-uri)

DRIVERS-2875: add support for TopologyDescriptionChangedEvent to expectEvents #1556

DRIVERS-2875: add support for TopologyDescriptionChangedEvent to expectEvents #1556

Conversation

W-A-James commented Mar 25, 2024 • edited Loading

W-A-James commented Mar 26, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmikola Mar 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ShaneHarvey left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmikola commented Apr 16, 2024

W-A-James commented Mar 25, 2024 •

edited

Loading

jmikola Mar 27, 2024 •

edited

Loading

ShaneHarvey left a comment •

edited

Loading