PYTHON-5945 Fix four command-span correctness bugs found in review

- db.query.text truncation reused a blind full-string slice, almost
  always producing invalid JSON; now truncates oversized field values
  first (matching the existing log-message truncation approach), with
  the blind slice only as a last-resort safety net.
- explain wraps the real command ({"explain": {"find": ...}}), the
  same shape as getMore's indirection, but wasn't handled, so explain
  spans silently lost db.collection.name.
- An explicit tracing.query_text_max_length=0 (meant to disable
  db.query.text) was indistinguishable from "not configured" and got
  silently overridden by the environment variable. The option is now
  Optional[int]: unset defers to the environment variable, any
  explicit value (including 0) always wins.
- server.port was set to None for Unix domain socket connections
  (address[1] is None for .sock addresses), an invalid attribute type
  that the OTel SDK would warn about and drop; the attribute is now
  omitted instead. Also corrected _ConnectionTelemetryInfo.address's
  type to match reality (_Address, not a non-optional int port).

Author: blink1073

pymongo/_otel.py

@@ -26,8 +26,9 @@
 from typing import TYPE_CHECKING, Any, Optional, TypedDict
 
 from bson import json_util
+from bson.json_util import _truncate_documents
 from pymongo._version import __version__
-from pymongo.logger import _HELLO_COMMANDS, _SENSITIVE_COMMANDS
+from pymongo.logger import _HELLO_COMMANDS, _JSON_OPTIONS, _SENSITIVE_COMMANDS
 
 try:
     from opentelemetry import trace
@@ -45,10 +46,15 @@
 
 
 class TracingOptions(TypedDict):
-    """The shape of the ``MongoClient`` ``tracing`` option."""
+    """The shape of the ``MongoClient`` ``tracing`` option.
+
+    ``query_text_max_length`` is None when the client didn't configure it, so
+    the environment variable can be consulted; any explicit value (including
+    0, to force ``db.query.text`` off) overrides the environment variable.
+    """
 
     enabled: bool
-    query_text_max_length: int
+    query_text_max_length: Optional[int]
 
 
 _OTEL_ENABLED_ENV = "OTEL_PYTHON_INSTRUMENTATION_MONGODB_ENABLED"
@@ -64,6 +70,11 @@ class TracingOptions(TypedDict):
 # See _gen_get_more_command in pymongo/message.py.
 _GET_MORE = "getMore"
 
+# explain wraps the real command (e.g. find/aggregate) rather than naming a
+# collection directly: {"explain": {"find": "coll", ...}}. See _Query.as_command
+# in pymongo/message.py.
+_EXPLAIN = "explain"
+
 # Commands against this database (e.g. user/role management, renameCollection)
 # never have a real collection name, even when their command value is a string.
 _ADMIN_DB = "admin"
@@ -94,21 +105,35 @@ def _get_tracer() -> Tracer:
 
 
 def _get_query_text_max_length(tracing_options: Optional[TracingOptions]) -> int:
-    """Return the configured db.query.text truncation length, or 0 to omit the attribute."""
-    client_value = tracing_options.get("query_text_max_length", 0) if tracing_options else 0
-    if client_value > 0:
-        return client_value
+    """Return the configured db.query.text truncation length, or 0 to omit the attribute.
+
+    An explicit client value (including 0) always wins; the environment
+    variable is only consulted when the client didn't configure it at all.
+    """
+    client_value = tracing_options.get("query_text_max_length") if tracing_options else None
+    if client_value is not None:
+        return max(0, client_value)
     try:
         return max(0, int(os.getenv(_OTEL_QUERY_TEXT_MAX_LENGTH_ENV, "0")))
     except ValueError:
         return 0
 
 
 def _build_query_text(cmd: Mapping[str, Any], max_length: int) -> str:
-    """Serialize ``cmd`` to extended JSON, redacted and truncated to ``max_length``."""
+    """Serialize ``cmd`` to extended JSON, redacted and truncated to ``max_length``.
+
+    Mirrors the truncation approach used for log messages: truncate field
+    values first, which usually keeps the result well-formed JSON (unlike a
+    blind cut of the fully-serialized string), then fall back to a hard
+    string cut with a "..." marker as a safety net for whatever the field
+    truncation's size estimate still leaves over ``max_length``.
+    """
     filtered = {k: v for k, v in cmd.items() if k not in _QUERY_TEXT_EXCLUDED_FIELDS}
-    text = json_util.dumps(filtered)
-    return text[:max_length]
+    truncated_cmd = _truncate_documents(filtered, max_length)[0]
+    text = json_util.dumps(truncated_cmd, json_options=_JSON_OPTIONS)
+    if len(text) > max_length:
+        text = text[:max_length] + "..."
+    return text
 
 
 def _extract_collection_name(
@@ -122,6 +147,12 @@ def _extract_collection_name(
     """
     if dbname == _ADMIN_DB:
         return None
+    if command_name == _EXPLAIN:
+        inner = cmd.get(_EXPLAIN)
+        if not isinstance(inner, Mapping) or not inner:
+            return None
+        inner_name = next(iter(inner))
+        return _extract_collection_name(inner_name, dbname, inner)
     key = "collection" if command_name == _GET_MORE else command_name
     value = cmd.get(key)
     return value if isinstance(value, str) else None
@@ -178,10 +209,12 @@ def start_span(
         "db.command.name": command_name,
         "db.query.summary": _build_query_summary(command_name, dbname, collection),
         "server.address": address[0],
-        "server.port": address[1],
         "network.transport": "unix" if address[0].endswith(".sock") else "tcp",
         "db.mongodb.driver_connection_id": conn.id,
     }
+    # Unix domain socket addresses have no port.
+    if address[1] is not None:
+        attributes["server.port"] = address[1]
     if collection:
         attributes["db.collection.name"] = collection
     if conn.server_connection_id is not None:

pymongo/asynchronous/mongo_client.py

@@ -625,9 +625,11 @@ def __init__(
               ``OTEL_PYTHON_INSTRUMENTATION_MONGODB_ENABLED`` environment variable; either
               being enabled is sufficient.
             - ``query_text_max_length``: (int) The maximum length of the ``db.query.text``
-              span attribute. Defaults to ``0``, which omits the attribute. Also controlled
-              by the ``OTEL_PYTHON_INSTRUMENTATION_MONGODB_QUERY_TEXT_MAX_LENGTH``
-              environment variable.
+              span attribute. Unset by default, which defers to the
+              ``OTEL_PYTHON_INSTRUMENTATION_MONGODB_QUERY_TEXT_MAX_LENGTH`` environment
+              variable (itself defaulting to ``0``, which omits the attribute). Setting
+              this explicitly, including to ``0``, always overrides the environment
+              variable.
 
         .. seealso:: The MongoDB documentation on `connections <https://dochub.mongodb.org/core/connections>`_.
 

pymongo/client_options.py

@@ -250,7 +250,7 @@ def __init__(
         )
         self.__tracing = cast(
             "_otel.TracingOptions",
-            options.get("tracing") or {"enabled": False, "query_text_max_length": 0},
+            options.get("tracing") or {"enabled": False, "query_text_max_length": None},
         )
 
     @property

pymongo/common.py

@@ -618,8 +618,9 @@ def validate_tracing_or_none(option: str, value: Any) -> Optional[_otel.TracingO
         raise ConfigurationError(f"Unknown tracing option(s): {sorted(unknown)}")
     enabled = value.get("enabled", False)
     validate_boolean("tracing.enabled", enabled)
-    query_text_max_length = value.get("query_text_max_length", 0)
-    validate_non_negative_integer("tracing.query_text_max_length", query_text_max_length)
+    query_text_max_length = value.get("query_text_max_length")
+    if query_text_max_length is not None:
+        validate_non_negative_integer("tracing.query_text_max_length", query_text_max_length)
     return {"enabled": enabled, "query_text_max_length": query_text_max_length}
 
 

pymongo/pool_shared.py

@@ -55,7 +55,7 @@ class _ConnectionTelemetryInfo(Protocol):
 
     id: int
     server_connection_id: Optional[int]
-    address: tuple[str, int]
+    address: _Address
     service_id: Optional[ObjectId]
 
 

pymongo/synchronous/mongo_client.py

@@ -626,9 +626,11 @@ def __init__(
               ``OTEL_PYTHON_INSTRUMENTATION_MONGODB_ENABLED`` environment variable; either
               being enabled is sufficient.
             - ``query_text_max_length``: (int) The maximum length of the ``db.query.text``
-              span attribute. Defaults to ``0``, which omits the attribute. Also controlled
-              by the ``OTEL_PYTHON_INSTRUMENTATION_MONGODB_QUERY_TEXT_MAX_LENGTH``
-              environment variable.
+              span attribute. Unset by default, which defers to the
+              ``OTEL_PYTHON_INSTRUMENTATION_MONGODB_QUERY_TEXT_MAX_LENGTH`` environment
+              variable (itself defaulting to ``0``, which omits the attribute). Setting
+              this explicitly, including to ``0``, always overrides the environment
+              variable.
 
         .. seealso:: The MongoDB documentation on `connections <https://dochub.mongodb.org/core/connections>`_.
 

test/asynchronous/test_otel.py

@@ -18,6 +18,7 @@
 
 import os
 import sys
+from typing import Optional
 from unittest.mock import patch
 
 sys.path[0:0] = [""]
@@ -26,6 +27,7 @@
 
 import pymongo._otel as _otel
 from pymongo.errors import OperationFailure
+from pymongo.typings import _Address
 from test.asynchronous import AsyncIntegrationTest, unittest
 
 if _otel._HAS_OPENTELEMETRY:
@@ -116,6 +118,43 @@ async def test_span_created_for_get_more(self):
             self.assertEqual(span.attributes["db.collection.name"], "test_otel_getmore")
             self.assertEqual(span.attributes["db.command.name"], "getMore")
 
+    async def test_explain_retains_collection_name(self):
+        # explain wraps the real command ({"explain": {"find": "coll", ...}}), the
+        # same shape as getMore's indirection, so it needs the same handling.
+        client = await self.async_rs_or_single_client(tracing={"enabled": True})
+        self.exporter.clear()
+        await client[self.db.name].command("explain", {"find": "test_otel", "filter": {}})
+
+        spans = self.spans("explain")
+        self.assertEqual(len(spans), 1)
+        attrs = spans[0].attributes
+        self.assertEqual(attrs["db.collection.name"], "test_otel")
+        self.assertEqual(attrs["db.query.summary"], f"explain {self.db.name}.test_otel")
+
+    async def test_server_port_omitted_for_unix_socket(self):
+        class _FakeUnixConn:
+            id = 1
+            server_connection_id: Optional[int] = None
+            address: _Address = ("/tmp/fake-otel-test.sock", None)
+            service_id = None
+
+        self.exporter.clear()
+        span = _otel.start_span(
+            {"enabled": True, "query_text_max_length": None},
+            _FakeUnixConn(),
+            {"ping": 1},
+            "admin",
+            "ping",
+            False,
+        )
+        _otel.end_span_success(span, {"ok": 1})
+
+        spans = self.spans("ping")
+        self.assertEqual(len(spans), 1)
+        attrs = spans[0].attributes
+        self.assertNotIn("server.port", attrs)
+        self.assertEqual(attrs["network.transport"], "unix")
+
     async def test_sensitive_command_produces_no_span(self):
         client = await self.async_rs_or_single_client(tracing={"enabled": True})
         self.exporter.clear()
@@ -208,6 +247,40 @@ async def test_query_text_included_when_configured(self):
         self.assertIn("db.query.text", spans[0].attributes)
         self.assertNotIn("lsid", spans[0].attributes["db.query.text"])
 
+    async def test_explicit_query_text_max_length_zero_overrides_env_var(self):
+        # An explicit client-side 0 must win over the environment variable, unlike
+        # unset (which defers to it) - otherwise an app can't reliably opt out.
+        env = {"OTEL_PYTHON_INSTRUMENTATION_MONGODB_QUERY_TEXT_MAX_LENGTH": "1024"}
+        with patch.dict(os.environ, env):
+            client = await self.async_rs_or_single_client(
+                tracing={"enabled": True, "query_text_max_length": 0}
+            )
+            self.exporter.clear()
+            await client.admin.command("ping")
+
+        spans = self.spans("ping")
+        self.assertEqual(len(spans), 1)
+        self.assertNotIn("db.query.text", spans[0].attributes)
+
+    async def test_query_text_truncation_shrinks_oversized_field_values(self):
+        client = await self.async_rs_or_single_client(
+            tracing={"enabled": True, "query_text_max_length": 200}
+        )
+        coll = client[self.db.name].test_otel
+        await coll.drop()
+        self.exporter.clear()
+        await coll.insert_one({"x": "a" * 500})
+
+        spans = self.spans("insert")
+        self.assertEqual(len(spans), 1)
+        query_text = spans[0].attributes["db.query.text"]
+        # The oversized field value must be truncated at the field level (not
+        # just a blind cut of the fully-serialized string), keeping the result
+        # close to the configured bound; a "..." marker signals the (possible)
+        # safety-net cut, mirroring the driver's existing log-truncation approach.
+        self.assertLessEqual(len(query_text), 200 + len("..."))
+        self.assertNotIn("a" * 500, query_text)
+
 
 if __name__ == "__main__":
     unittest.main()

test/test_otel.py

@@ -18,6 +18,7 @@
 
 import os
 import sys
+from typing import Optional
 from unittest.mock import patch
 
 sys.path[0:0] = [""]
@@ -26,6 +27,7 @@
 
 import pymongo._otel as _otel
 from pymongo.errors import OperationFailure
+from pymongo.typings import _Address
 from test import IntegrationTest, unittest
 
 if _otel._HAS_OPENTELEMETRY:
@@ -116,6 +118,43 @@ def test_span_created_for_get_more(self):
             self.assertEqual(span.attributes["db.collection.name"], "test_otel_getmore")
             self.assertEqual(span.attributes["db.command.name"], "getMore")
 
+    def test_explain_retains_collection_name(self):
+        # explain wraps the real command ({"explain": {"find": "coll", ...}}), the
+        # same shape as getMore's indirection, so it needs the same handling.
+        client = self.rs_or_single_client(tracing={"enabled": True})
+        self.exporter.clear()
+        client[self.db.name].command("explain", {"find": "test_otel", "filter": {}})
+
+        spans = self.spans("explain")
+        self.assertEqual(len(spans), 1)
+        attrs = spans[0].attributes
+        self.assertEqual(attrs["db.collection.name"], "test_otel")
+        self.assertEqual(attrs["db.query.summary"], f"explain {self.db.name}.test_otel")
+
+    def test_server_port_omitted_for_unix_socket(self):
+        class _FakeUnixConn:
+            id = 1
+            server_connection_id: Optional[int] = None
+            address: _Address = ("/tmp/fake-otel-test.sock", None)
+            service_id = None
+
+        self.exporter.clear()
+        span = _otel.start_span(
+            {"enabled": True, "query_text_max_length": None},
+            _FakeUnixConn(),
+            {"ping": 1},
+            "admin",
+            "ping",
+            False,
+        )
+        _otel.end_span_success(span, {"ok": 1})
+
+        spans = self.spans("ping")
+        self.assertEqual(len(spans), 1)
+        attrs = spans[0].attributes
+        self.assertNotIn("server.port", attrs)
+        self.assertEqual(attrs["network.transport"], "unix")
+
     def test_sensitive_command_produces_no_span(self):
         client = self.rs_or_single_client(tracing={"enabled": True})
         self.exporter.clear()
@@ -206,6 +245,36 @@ def test_query_text_included_when_configured(self):
         self.assertIn("db.query.text", spans[0].attributes)
         self.assertNotIn("lsid", spans[0].attributes["db.query.text"])
 
+    def test_explicit_query_text_max_length_zero_overrides_env_var(self):
+        # An explicit client-side 0 must win over the environment variable, unlike
+        # unset (which defers to it) - otherwise an app can't reliably opt out.
+        env = {"OTEL_PYTHON_INSTRUMENTATION_MONGODB_QUERY_TEXT_MAX_LENGTH": "1024"}
+        with patch.dict(os.environ, env):
+            client = self.rs_or_single_client(tracing={"enabled": True, "query_text_max_length": 0})
+            self.exporter.clear()
+            client.admin.command("ping")
+
+        spans = self.spans("ping")
+        self.assertEqual(len(spans), 1)
+        self.assertNotIn("db.query.text", spans[0].attributes)
+
+    def test_query_text_truncation_shrinks_oversized_field_values(self):
+        client = self.rs_or_single_client(tracing={"enabled": True, "query_text_max_length": 200})
+        coll = client[self.db.name].test_otel
+        coll.drop()
+        self.exporter.clear()
+        coll.insert_one({"x": "a" * 500})
+
+        spans = self.spans("insert")
+        self.assertEqual(len(spans), 1)
+        query_text = spans[0].attributes["db.query.text"]
+        # The oversized field value must be truncated at the field level (not
+        # just a blind cut of the fully-serialized string), keeping the result
+        # close to the configured bound; a "..." marker signals the (possible)
+        # safety-net cut, mirroring the driver's existing log-truncation approach.
+        self.assertLessEqual(len(query_text), 200 + len("..."))
+        self.assertNotIn("a" * 500, query_text)
+
 
 if __name__ == "__main__":
     unittest.main()