ext-mongodb 2.0 support #324
Conversation
Being able to retrieve host, port + info from a CommandStartedEvent is deprecated in 1.20.0 and will be removed in 2.0. we have been advised to use SDAM subscription instead. - update tests to allow defining mongo server from env - require ext-mongodb 1.13 (which provides SDAM subscriber for server info) - use SDAM subscriber to get server info attributes. Subscribe to the serverChanged event and store the attributes in a class variable, organised by host/port.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #324 +/- ##
============================================
+ Coverage 81.91% 85.87% +3.95%
- Complexity 948 1081 +133
============================================
Files 84 73 -11
Lines 3964 4480 +516
============================================
+ Hits 3247 3847 +600
+ Misses 717 633 -84
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 13 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
| { | ||
| $host = $event->getHost(); | ||
| $port = $event->getPort(); | ||
| $info = $event->getNewDescription()->getHelloResponse(); |
There was a problem hiding this comment.
There is a corner case mentioned in MongoDB extension documentation at https://www.php.net/manual/en/mongodb-driver-serverdescription.gethelloresponse.php
When the driver is connected to a load balancer, this method will return an empty array since load balancers are not monitored. This is in contrast to MongoDB\Driver\Server::getInfo(), which would return the backing server's » hello command response from the initial connection handshake.
The code comment regarding this still seems present in 2.x branch of mongo extension, so this is probably still the case even though the above documentation is for 1.x. I don't see an obvious workaround for 2.x, but possibly if this $info here is empty array and we detect that mongo extension version is 1.x, there could be a fallback to the previous behavior.
There was a problem hiding this comment.
Do you know what they mean by "load balancer"? I set up a replicaSet and it correctly fetches info from all servers via the serverChanged event.
There was a problem hiding this comment.
I checked what the extension tests with for tests that run "with load balancer", and it seems they use haproxy as seen here. On the driver side it seems adding loadBalanced=true to connection URL parameters should make the driver behave as if it is connecting through a load balancer, so maybe no actual load balancer is required to trigger this behavior, just that parameter? The specification is here.
There was a problem hiding this comment.
ugh, loadBalanced=true seems to require a real, working, load-balanced setup, and I cannot get a load-balanced mongo cluster running.
I've got a 3-node replicaset, and haproxy in front. I can connect through haproxy and round-robin to the different nodes, however it's not "real" load balancing as adding loadBalanced=true gives me a Driver attempted to initialize in load balancing mode, but the server does not support this mode.
Since our mongo package is not stable (latest=0.0.6), the easiest path forward is to go ahead with this change, and wait for somebody with a load-balanced setup to show up (and/or create a todo/help-wanted issue).
Another point to consider is that all of these extra fields are not part of the official semconv, and according to https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/telemetry-stability.md#stable-instrumentations they should not be used in a stable instrumentation. It's not a problem now, but we may end up needing to remove them in the future anyway, if we're following the spec dogmatically:
Stable instrumentations authored by OpenTelemetry SHOULD NOT produce telemetry that is not described by OpenTelemetry semantic conventions
There was a problem hiding this comment.
That seems reasonable. 👍
There was a problem hiding this comment.
Regarding load balancers, some drivers do use HAPorxy for local testing but the actual feature is more involved and entails the backing servers also being aware of the load balancer (and returning a serviceId field in their hello responses). The Load Balancer specification goes into more detail about the feature.
3 participants
Being able to retrieve host, port + info from a CommandStartedEvent is deprecated in 1.20.0 and will be removed in 2.0. we have been advised to use SDAM subscription instead.
Fixes: open-telemetry/opentelemetry-php#1467