SWIFT-1015 Only create monitoring events if they are being consumed #531
Conversation
| let event = type.init(mongocEvent: mongocEvent) | ||
|
|
||
| // TODO: SWIFT-524: remove workaround for CDRIVER-3256 | ||
| if let tdChanged = event as? TopologyDescriptionChangedEvent, |
There was a problem hiding this comment.
this CDRIVER ticket was resolved as part of 1.17.
| } | ||
|
|
||
| expect(receivedEvents.count).to(equal(4)) | ||
| expect(receivedEvents.count).to(equal(5)) |
There was a problem hiding this comment.
so, the CDRIVER ticket mentioned above only actually resolved the duplicate server changed event issue. The topology changed one still existed. however, according to DRIVERS-944, publishing the event is consistent with the SDAM monitoring tests.
I'm kind of 50/50 on whether to make the change, because as Kevin points out in the comment, there is no visible change to the user from the topology description. But to not publish it would be inconsistent with the spec tests and what other drivers seem to do if they pass the tests.
There was a problem hiding this comment.
Yeah I kind of agree that we probably shouldn't emit events in this case, but also think for simplicity and conformity we should just do what the tests require. If that ticket ever gets resolved in the other direction we could just stop emitting the event--I don't think it would constitute a breaking behavioral change.
Codecov Report
@@ Coverage Diff @@
## master #531 +/- ##
=========================================
Coverage ? 77.09%
=========================================
Files ? 132
Lines ? 14095
Branches ? 0
=========================================
Hits ? 10866
Misses ? 3229
Partials ? 0
Continue to review full report at Codecov.
|
| } | ||
|
|
||
| expect(receivedEvents.count).to(equal(4)) | ||
| expect(receivedEvents.count).to(equal(5)) |
There was a problem hiding this comment.
Yeah I kind of agree that we probably shouldn't emit events in this case, but also think for simplicity and conformity we should just do what the tests require. If that ticket ever gets resolved in the other direction we could just stop emitting the event--I don't think it would constitute a breaking behavioral change.
This is a longstanding inefficiency in the driver, but only came up as a problem when using the new BSON library.
Even if the user has no handlers registered, we create a corresponding Swift event, which in the case of e.g. a command started event for a large command is quite expensive (especially now that - at least for the moment - by default we validate incoming documents.)
This PR amends that by introducing a new concept of a monitoring "component" for each event type. Depending on the component of the event to be published, we inspect the client to see if any corresponding handlers exist.
I am feeling like this file is getting a little wild, using a lot of different types and protocols, but at the same time I couldn't figure out a clean way to accomplish this without the addition of the new type. I technically could have checked if the type conformed to
CommandEventProtocolor not and acted accordingly but that felt kind of hacky. If you have any ideas let me know.Note that as part of this I also resolved SWIFT-524 (see inline comment about test changes).
This problem might have somewhat "gone away" if we stopped validating the command documents (which we should probably also do), but this was a very quick win that I think is worth implementing regardless.
Comparison of runtimes, in seconds:
Note that the first and second to last ones literally got 10,000 times faster...!! My spawn host was working hard last night 🙂
I need to do some further investigation into what the remaining bottlenecks for these benchmarks are, which should be much easier now that their runtimes are significantly lower. I suspect validation continues to be a main culprit, which we still encounter via e.g. parsing reply documents and reading documents from cursors. I will probably end up writing a couple of new small benchmarks that specifically test BSON validation in order to speed up iterating on that code.