DOCSP-41982: cluster monitoring #144

rustagir · 2024-09-17T18:34:18Z

Pull Request Info

JIRA - https://jira.mongodb.org/browse/DOCSP-41982
Staging - https://deploy-preview-144--docs-php-library.netlify.app/monitoring/cluster-monitoring/

Self-Review Checklist

Is this free of any warnings or errors in the RST?
Did you run a spell-check?
Did you run a grammar-check?
Are all the links working?
Are the facets and meta keywords accurate?

netlify · 2024-09-17T18:34:36Z

✅ Deploy Preview for docs-php-library ready!

Name	Link
🔨 Latest commit	`4a39d20`
🔍 Latest deploy log	https://app.netlify.com/sites/docs-php-library/deploys/66f1c3f20ddedb000866a811
😎 Deploy Preview	https://deploy-preview-144--docs-php-library.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

mongoKart

Looks really good! A few very small changes

source/monitoring.txt

source/monitoring/cluster-monitoring.txt

mongoKart · 2024-09-17T20:32:06Z

source/monitoring/cluster-monitoring.txt

+     - Description
+
+   * - ``ServerChangedEvent``
+     - Created when an instance state changes, such as from secondary to


Optional: Could remove "Created when" from the start of every event (probably along with a new column header name)

I can't think of a good way to adjust the column header, as renaming it "Situation that Triggers Event" seems worse than keeping the "Created when"s in

In the docs for ServerChangedEvent we say the event class "encapsulates information about a changed server description".

Here, the table is presenting the event as the thing that happened instead of just an object containing some information about a thing that happened.

The "Condition" column in Events API from the driver spec might be helpful. Note that the spec includes "Description" in the name of these events, where PHP just calls it ServerChangedEvent. I think that was due to us copying the shorter name from our libmongoc dependency.

I might suggest:

When the server description changes, such as the server's type changing from secondary to primary.

mongoKart

LGTM

source/includes/monitoring/sdam.php

jmikola · 2024-09-19T15:17:18Z

source/includes/monitoring/sdam.php

+    {
+        $this->stream = $stream;
+    }
+    public function serverOpening(MongoDB\Driver\Monitoring\ServerOpeningEvent $event): void


Suggested change

public function serverOpening(MongoDB\Driver\Monitoring\ServerOpeningEvent $event): void

public function serverOpening(MongoDB\Driver\Monitoring\ServerOpeningEvent $event): void

Also, between any methods with bodies.

source/includes/monitoring/sdam.php

jmikola · 2024-09-19T15:21:35Z

source/includes/monitoring/sdam.php

+    public function serverHeartbeatSucceeded(MongoDB\Driver\Monitoring\ServerHeartbeatSucceededEvent $event): void {}
+    public function topologyChanged(MongoDB\Driver\Monitoring\TopologyChangedEvent $event): void {}
+    public function topologyClosed(MongoDB\Driver\Monitoring\TopologyClosedEvent $event): void {}
+    public function topologyOpening(MongoDB\Driver\Monitoring\TopologyOpeningEvent $event): void {}


I believe contravariance would allow us to remove the type hints from these methods and make the code more concise. See: https://3v4l.org/H34oT#v8.3.11

That said, it'd make the example code less useful for copying if users want to implement these methods themselves. I'm fine to leave these as-is.

/cc @alcaeus

Omitting the types would make the example more concise, but I'd like to avoid suggesting to do this in documentation, as I consider it bad practice. We could leverage use statements to shorten the types, but that may be difficult if only small snippets of this code are included in the documentation.

We face a similar issue with the other documentation examples used in the server docs, and we decided to just deal with the long types in favour of having copy-pastable code.

source/includes/monitoring/sdam.php

jmikola · 2024-09-19T15:26:33Z

source/monitoring/cluster-monitoring.txt

+
+This guide shows you how to use the {+php-library+} to monitor topology
+events in a MongoDB instance, replica set, or sharded cluster. The
+driver creates topology events, also known as Server Discovery and


Technically, topology events are those related to the entire cluster, while server events are those pertaining to a single server within the topology. SDAM monitoring includes both types of events.

I would personally just refer to them as SDAM events, as you seem to do in a subsequent section. If you need a short name, "cluster events" might also work but there's no prior art for that in any driver spec.

Disregard if this name was intentionally chosen for consistency with existing driver docs.

Good to know, in general it is ok if this page differs from the same page in other docs. Each docs standardization is meant to improve on previous ones!

source/monitoring/cluster-monitoring.txt

jmikola · 2024-09-19T15:41:20Z

source/monitoring/cluster-monitoring.txt

+     - Description
+
+   * - ``ServerChangedEvent``
+     - Created when an instance state changes, such as from secondary to


In the docs for ServerChangedEvent we say the event class "encapsulates information about a changed server description".

Here, the table is presenting the event as the thing that happened instead of just an object containing some information about a thing that happened.

The "Condition" column in Events API from the driver spec might be helpful. Note that the spec includes "Description" in the name of these events, where PHP just calls it ServerChangedEvent. I think that was due to us copying the shorter name from our libmongoc dependency.

I might suggest:

When the server description changes, such as the server's type changing from secondary to primary.

source/monitoring/cluster-monitoring.txt

jmikola

Some suggested rewording, but I want to wait until I hear back from other folks in drivers before signing off on the ServerOpeningEvent text.

source/monitoring/cluster-monitoring.txt

jmikola · 2024-09-20T14:48:41Z

source/monitoring/cluster-monitoring.txt


-   * - ``TopologyClosedEvent``
+   * - :php:`TopologyClosedEvent <mongodb-driver-monitoring-topologyclosedevent>`
     - Created when the topology is closed.


I'd reword this as "the driver disconnects from the cluster". Given the event name, I don't think we need to repeat "topology is closed" in the description, although you can still include both if you like. In that case, maybe note the disconnect behavior with "i.e." or something.

Per the SDAM Monitoring spec, this would always be the last event to be dispatched. In the PHP driver, it's not actually possible to observe this event when using default behavior, as we persist the libmongoc client object (and thus the connections) beyond the lifetime of the PHP script. So by the time a client is actually closed, there'd be no subscriber in memory to receive the event.

This event can only be observed when specifying disableClientPersistence: true in the Manager or Client driver options array. There's more context to be found in PHPC-2023 and mongodb/mongo-php-driver@c2410ab.

Having said that, I think it's sufficient to just refer to disconnecting from the cluster here. And admittedly, we (the PHP team) should improve the docs for these event classes. I created PHPC-2449 to track that.

thanks for providing this background!

source/monitoring/cluster-monitoring.txt

jmikola

LGTM with my suggestions applied.

jmikola · 2024-09-23T18:26:16Z

source/monitoring/cluster-monitoring.txt

+     - Created when the driver disconnects from the cluster.
+
+   * - :php:`ServerHeartbeatStartedEvent <mongodb-driver-monitoring-serverheartbeatstartedevent>`
+     - Created when the server monitor sends a ``hello`` call to the server.


Suggested change

- Created when the server monitor sends a ``hello`` call to the server.

- Created when the server monitor sends a ``hello`` command to the server.

"Command" is less ambiguous.

jmikola · 2024-09-23T18:26:43Z

source/monitoring/cluster-monitoring.txt

+     - Created when a server connection is closed.
+
+   * - :php:`TopologyChangedEvent <mongodb-driver-monitoring-topologychangedevent>`
+     - Created when the topology description changes, such when there is


I think you accidentally a word in this sentence.

Suggested change

- Created when the topology description changes, such when there is

- Created when the topology description changes, such as when there is

mongoKart requested changes Sep 17, 2024

View reviewed changes

rustagir added 3 commits September 18, 2024 09:27

DOCSP-41982: cluster monitoring

5dcbd15

small fixes

564dfe9

MW PR fixes 1

fce8972

rustagir force-pushed the DOCSP-41982-cluster-monitoring branch from a36de3e to fce8972 Compare September 18, 2024 13:33

rustagir requested a review from mongoKart September 18, 2024 15:05

mongoKart approved these changes Sep 18, 2024

View reviewed changes

rustagir requested review from a team and jmikola and removed request for a team September 18, 2024 17:33

jmikola reviewed Sep 19, 2024

View reviewed changes

rustagir added 4 commits September 19, 2024 13:42

JM tech review 1

9dfc72a

single quotes

8cf4fc1

links

fc1506c

vale

25d4f88

rustagir requested a review from jmikola September 19, 2024 17:51

jmikola reviewed Sep 20, 2024

View reviewed changes

JM tech review 2

e3351c5

rustagir requested a review from jmikola September 23, 2024 14:25

jmikola approved these changes Sep 23, 2024

View reviewed changes

JM final fixes

4a39d20

rustagir merged commit e2cc75b into mongodb:php-standardization Sep 23, 2024
4 of 5 checks passed

	public function serverOpening(MongoDB\Driver\Monitoring\ServerOpeningEvent $event): void

	public function serverOpening(MongoDB\Driver\Monitoring\ServerOpeningEvent $event): void

	- Created when the server monitor sends a ``hello`` call to the server.
	- Created when the server monitor sends a ``hello`` command to the server.

	- Created when the topology description changes, such when there is
	- Created when the topology description changes, such as when there is

DOCSP-41982: cluster monitoring #144

DOCSP-41982: cluster monitoring #144

Uh oh!

Conversation

rustagir commented Sep 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Info

Self-Review Checklist

Uh oh!

netlify bot commented Sep 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Deploy Preview for docs-php-library ready!

Uh oh!

mongoKart left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

mongoKart left a comment

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!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jmikola left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jmikola left a comment

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!

Uh oh!

rustagir commented Sep 17, 2024 •

edited

Loading

netlify bot commented Sep 17, 2024 •

edited

Loading