SDAM monitoring updates #379

patrickfreed · 2020-01-02T19:39:41Z

This PR introduces a number of updates to the SDAM monitoring API:

Misc. updates:

ConnectionId refactored to Address. This more accurately reflects what is actually stored within the id, and the field name / object returned are flexibly specified in the SDAM monitoring spec:
- The connection id is the unique identifier of the driver’s Connection object that wraps the socket. For languages that do not have this object, this MUST a string of “hostname:port” or an object that that contains the hostname and port as attributes. The name of this field is flexible to match the object that is returned from the driver. Examples are, but not limited to, ‘address’, ‘serverAddress’, ‘connectionId’,
Fields that would always be nil on TopologyDescription and ServerDescription have just been omitted. We can add them back once we have a pure Swift implementation of SDAM if necessary.
Removed opTime from ServerDescription since it only needs to be recorded by mongos.
Introduce the compatible property on TopoogyDescription as described in the SDAM spec.
Convert integer types on ServerDescription and TopologyDescription to just be Ints. Rationale:
- these would overflow 32 bits (which is only a concern if Swift even ran on 32-bit systems, which it doesn't)
- the SDAM spec is not very prescriptive of how the data is actually stored, it just gives a suggested outline

SWIFT-601 changes:

Introduced an umbrella MongoSDAMEvent protocol that is empty at the moment.
- Inherits from MongoEvent
- not directly conformed to.
Introduced MongoTopologyUpdateEvent protocol:
- inherits from MongoSDAMEvent
- conformed to by TopologyDescriptionChangedEvent, TopologyOpeningEvent, and TopologyClosedEvent
Introduced MongoServerUpdateEvent protocol:
- inherits from MongoSDAMEvent
- conformed to by ServerDescriptionChangedEvent, ServerOpeningEvent, and ServerClosingEvent
Introduced MongoServerHeartbeatEvent protocol
- inherits from MongoSDAMEvent
- conformed to by ServerHeartbeatStartedEvent, ServerHeartbeatSucceededEvent, and ServerHeartbeatFailedEvent

SWIFT-288 was also completed as part of this work.

patrickfreed · 2020-01-02T19:40:27Z

.swiftformat

@@ -1,4 +1,5 @@
 --exclude Sources/MongoSwift/MongoSwiftVersion.swift,Sources/MongoSwiftSync/*
+--exclude .build/*


not sure why this wasn't a problem for me before, maybe a linux only thing?

🤔weird, I don't think I've run into this either

Tests/DisabledTests/ClientSessionTests.swift

kmahar · 2020-01-02T19:44:37Z

.swiftformat

@@ -1,4 +1,5 @@
 --exclude Sources/MongoSwift/MongoSwiftVersion.swift,Sources/MongoSwiftSync/*
+--exclude .build/*


🤔weird, I don't think I've run into this either

Sources/MongoSwift/SDAM.swift

kmahar · 2020-01-02T19:54:58Z

Sources/MongoSwift/SDAM.swift

+        self.lastUpdateTime = Date(msSinceEpoch: mongoc_server_description_last_update_time(description))
        let serverType = String(cString: mongoc_server_description_type(description))
        // swiftlint:disable:next force_unwrapping
        self.type = ServerType(rawValue: serverType)! // libmongoc will always give us a valid raw value.


i wonder if it would be worth adding a .other ServerType case 🤔

A new server type would be a mega breaking change for all drivers I think. The only case I can think of that might introduce one is multi-master, but that might be handled entirely by mongos or something. @mbroadst probably has a better perspective on this.

Sources/MongoSwift/SDAM.swift

mbroadst · 2020-01-03T19:03:11Z

Sources/MongoSwift/SDAM.swift

-public struct ConnectionId: Equatable {
-    /// A string representing the host for this connection.
+/// A struct representing a server address, consisting of a host and port.
+public struct Address: Equatable {


nit: let's either call it ServerAddress or correct the comment to explain this is a generic address. Also, now that we have NIO as a dependency, is there a good reason we aren't just using their SocketAddress? It seems to serve a similar purpose.

done. I added a try! when converting from a mongoc_host_list_t to a SocketAddress, since I figured libmongoc would only contain well-formatted addresses in its server descriptions. Is that a safe assumption to make?

SocketAddress only works on resolved IP addresses, so things like "localhost" or a domain name won't be able to be stored in it, which I think doesn't work for us. As such, I've reverted the commits that enabled its usage in the driver.

I've updated the comment to say a "network address" instead of "server address"

mbroadst · 2020-01-03T19:07:02Z

Sources/MongoSwift/SDAM.swift

-            lhs.lastWriteDate == rhs.lastWriteDate &&
-            lhs.opTime == rhs.opTime &&
+        // As per the SDAM spec, only some fields are necessary to compare for equality.
+        return lhs.address == rhs.address &&


This will at some point need to account for a stored error on the ServerDescription as well.

mbroadst · 2020-01-03T19:10:37Z

Sources/MongoSwift/SDAM.swift

+    /// Whether every server's wire protocol version range is compatible with MongoSwift's.
+    public var compatible: Bool {
+        return self.servers.allSatisfy {
+            $0.maxWireVersion >= MIN_SUPPORTED_WIRE_VERSION


This logic is incorrect. From the spec:

A ServerDescription which is not Unknown is incompatible if: minWireVersion > clientMaxWireVersion, or maxWireVersion < clientMinWireVersion

Here we are not ignoring Unknown servers, nor are we checking the minWireVersion requirements. Additionally, the reason the error itself is stored is because it will likely change based on what part of this is incompatible, its unclear you'd have that information where you eventually report the error to the user.

Do you think this property is worth having around right now? I'm now starting to realize that it was probably intended to be used as part of server selection error reporting rather than SDAM monitoring.

For now I've just removed it.

Sources/MongoSwift/SDAM.swift

This reverts commit ce090e0.

This reverts commit 9670410.

patrickfreed commented Jan 2, 2020

View reviewed changes

patrickfreed marked this pull request as ready for review January 2, 2020 19:42

patrickfreed requested review from kmahar and mbroadst January 2, 2020 19:42

kmahar reviewed Jan 2, 2020

View reviewed changes

kmahar approved these changes Jan 2, 2020

View reviewed changes

mbroadst reviewed Jan 3, 2020

View reviewed changes

patrickfreed force-pushed the sdam-updates branch 3 times, most recently from 9670410 to 5945844 Compare January 9, 2020 22:21

patrickfreed added 14 commits January 30, 2020 10:49

updates to sdam

3aa8825

introduce protocols for SDAM monitoring events

84f60c2

sync exports

54d4954

add lastWriteDate back

c56861b

add todo comment

c33cc95

make all fields on ServerDescription immutable

e6a348f

make fields of TopologyDescription immutable

7bdfc98

rewrite with one-liner

67133de

use NIO's SocketAddress

6ed955b

update after rebase

eda652e

Revert "use NIO's SocketAddress"

998e40e

This reverts commit ce090e0.

Revert "update after rebase"

3830f73

This reverts commit 9670410.

fix docstring

8b65cd1

remove compatible computed property

f88613e

patrickfreed force-pushed the sdam-updates branch from 12f8477 to f88613e Compare January 30, 2020 16:02

patrickfreed merged commit 3988800 into master Jan 30, 2020

patrickfreed deleted the sdam-updates branch February 13, 2020 18:48

		@@ -1,4 +1,5 @@
		--exclude Sources/MongoSwift/MongoSwiftVersion.swift,Sources/MongoSwiftSync/*
		--exclude .build/*

SDAM monitoring updates #379

SDAM monitoring updates #379

Uh oh!

Conversation

patrickfreed commented Jan 2, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants