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 79.75% 81.87% +2.11%
- Complexity 249 575 +326
============================================
Files 32 54 +22
Lines 988 2422 +1434
============================================
+ Hits 788 1983 +1195
- Misses 200 439 +239
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 23 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
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