PYTHON-5745 Consolidate command telemetry by blink1073 · Pull Request #2891 · mongodb/mongo-python-driver

blink1073 · 2026-06-24T16:22:31Z

Changes in this PR

Introduces pymongo/_telemetry.py with a _CommandTelemetry class that unifies structured logging and APM event publishing for command lifecycle events into a single internal API. This refactor is a stepping stone to adding OpenTelemetry support in a _telemetry.py, so all event-related handling is in one centralized location.

Test Plan

No new tests: this is a behavior-preserving refactor. The existing APM / command-monitoring and command-logging spec suites assert the exact event and log documents that any regression would change.

Checklist

Checklist for Author

Did you update the changelog (if necessary)? — Not necessary; internal refactor with no user-facing behavior change.
Is there test coverage? — Covered by existing spec suites (behavior preserved).
Is any followup work tracked in a JIRA ticket? If so, add link(s). — PYTHON-5846 will extend this pattern to cover the remaining logging and APM event types.

Checklist for Reviewer

Does the title of the PR reference a JIRA Ticket?
Do you fully understand the implementation? (Would you be comfortable explaining how this code works to someone else?)
Is all relevant documentation (README or docstring) updated?

codecov-commenter · 2026-06-24T17:05:20Z

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

Copilot

Pull request overview

This PR refactors command lifecycle telemetry by centralizing structured command logging and APM command monitoring event publishing into a single internal helper (_CommandTelemetry) in pymongo/_telemetry.py, and updates both sync/async command execution paths to use it.

Changes:

Added pymongo/_telemetry.py with _CommandTelemetry to unify STARTED/SUCCEEDED/FAILED command logging + APM publishing and duration tracking.
Updated sync/async command_runner.py to delegate command telemetry to _CommandTelemetry and simplified call signatures (removing explicit address/start plumbing).
Introduced _ConnectionTelemetryInfo protocol and updated sync/async pool connection classes to provide the telemetry-required connection fields.

Reviewed changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
pymongo/_telemetry.py	New centralized internal helper for command telemetry (logging + APM publishing).
pymongo/pool_shared.py	Adds `_ConnectionTelemetryInfo` protocol to type required connection metadata for telemetry.
pymongo/message.py	Removes bulk write `start_time` tracking now handled by telemetry helper.
pymongo/synchronous/command_runner.py	Replaces inline telemetry logic with `_CommandTelemetry` and updates `_run_command` signature.
pymongo/asynchronous/command_runner.py	Async equivalent refactor to `_CommandTelemetry` and updated `_run_command` signature.
pymongo/synchronous/pool.py	Sync `Connection` updated to satisfy telemetry connection interface; updates run_command call plumbing.
pymongo/asynchronous/pool.py	Async `AsyncConnection` updated to satisfy telemetry connection interface; updates run_command call plumbing.
pymongo/synchronous/server.py	Removes now-unneeded start/address telemetry plumbing for cursor ops.
pymongo/asynchronous/server.py	Async equivalent removal of start/address telemetry plumbing for cursor ops.

NoahStapp · 2026-06-25T14:10:21Z

+        self._duration = datetime.datetime.now() - self._start
+        if not self._should_log and not self._publish:
+            return
+        duration = self._duration


Do we need this assignment? self._duration should only get assigned once per _CommandTelemetry lifecycle.

NoahStapp · 2026-06-25T14:10:51Z

+        self._duration = datetime.datetime.now() - self._start
+        if not self._should_log and not self._publish:
+            return
+        duration = self._duration


NoahStapp · 2026-06-25T14:17:40Z

+        request_id: int,
+        op_id: Optional[int],
+    ) -> None:
+        self._topology_id = topology_id


We construct the whole object even if self.should_log = False, does it make sense to have the constructor short-circuit out with that as the first check?

I don't think a short-circuit makes sense in the constructor, but I added a short-circuit in the methods

NoahStapp · 2026-06-25T14:19:20Z

+        self._start: datetime.datetime
+        self._duration: datetime.timedelta
+
+    def started(self, orig: MutableMapping[str, Any], ensure_db: bool) -> None:


Could we refactor started, succeeded, failed (or a subset) to use a common internal helper like _emit? All three are very similar and effectively just pass through kwargs to debug_log.

Makes sense, done

NoahStapp · 2026-06-25T14:20:52Z

    from pymongo.typings import _Address


+class _ConnectionTelemetryInfo(Protocol):


Is there a reason to use this over the existing _AgnosticConnection?

Re #discussion_r3475117295: Two reasons: (1) _AgnosticConnection is Union["AsyncConnection", "Connection"] — a type alias, not a class, so AsyncConnection/Connection can't inherit from it. (2) _AgnosticConnection is defined in typings.py and forward-references both pool classes, which would introduce a circular import when used from pool_shared.py. The Protocol also narrows the surface to exactly the four fields _CommandTelemetry needs, making the contract explicit.

I didn't mean to send the raw response. My main goal was to have a properties-only subset that would be used here and probably the pool telemetry as well.

Is the idea to have a single, minimal listing of connection attributes that we need for telemetry? I don't think either of the AI responses are actually true in this context since _ConnectionTelemetryInfo is used purely as a type annotation.

Yeah, ignore the AI response

I'm fine with trying _ConnectionTelemetryInfo out, we can remove it in a future refactor or consolidation if it turns out to be less useful than expected here.

…flag

NoahStapp · 2026-06-25T19:06:50Z

    :param max_doc_size: The largest document size in the batch, passed to ``conn.send_message``.
    :param unacknowledged: When ``True``, send only and fake an ``{"ok": 1}`` reply.
    """
+    topology_id = client._topology_settings._topology_id if client is not None else None


Should we add a private _topology_id attribute on MongoClient at this point?

Makes sense, I had that in my previous PR at one point

NoahStapp · 2026-06-25T20:47:25Z

+
+    def started(self, orig: MutableMapping[str, Any], ensure_db: bool) -> None:
+        """Emit the STARTED log entry and APM event, and start the duration clock."""
+        self._start = datetime.datetime.now()


Nit: this (and the similar lines in the other methods) should be after the if not self.active check.

telemetry.duration is accessed unconditionally at the end of _run_command (always needed for the Response object), so _start must be set before any early-return path. If we moved self._start = datetime.datetime.now() to after the _active check in started(), telemetry.duration would raise AttributeError on every non-APM/non-logging command. The same applies to the _duration assignments in succeeded()/failed().

What the heck, I have "Ask permissions" turned on, and this is twice now it just replied. 😠

But yeah, even without logging we need duration upstream

Can't trust the robots man

NoahStapp · 2026-06-25T20:50:44Z

    from pymongo.typings import _Address


+class _ConnectionTelemetryInfo(Protocol):


Is the idea to have a single, minimal listing of connection attributes that we need for telemetry? I don't think either of the AI responses are actually true in this context since _ConnectionTelemetryInfo is used purely as a type annotation.

NoahStapp

LGTM!

PYTHON-5745 Consolidate command telemetry into _CommandTelemetry

63a9768

blink1073 added 3 commits June 24, 2026 13:26

PYTHON-5745 Fix copyright year in _telemetry.py

d62b69f

PYTHON-5745 Guard ObjectId import with TYPE_CHECKING in pool_shared.py

a08f326

PYTHON-5745 Fix speculative_hello computed regardless of APM state

adfa863

blink1073 marked this pull request as ready for review June 24, 2026 20:47

blink1073 requested a review from a team as a code owner June 24, 2026 20:47

blink1073 requested a review from NoahStapp June 24, 2026 20:47

NoahStapp requested a review from Copilot June 25, 2026 14:03

Copilot started reviewing on behalf of NoahStapp June 25, 2026 14:04 View session

Copilot AI reviewed Jun 25, 2026

View reviewed changes

Comment thread pymongo/pool_shared.py

NoahStapp requested changes Jun 25, 2026

View reviewed changes

PYTHON-5745 Remove redundant duration local in _CommandTelemetry

8c8fd34

aclark4life mentioned this pull request Jun 25, 2026

PYTHON-5856 Move cursor execution logic into cursor class #2894

Draft

6 tasks

PYTHON-5745 Refactor _CommandTelemetry: _emit_log helper and _active …

541e039

…flag

blink1073 requested a review from NoahStapp June 25, 2026 17:59

NoahStapp requested changes Jun 25, 2026

View reviewed changes

PYTHON-5745 Add _topology_id property to MongoClient

d2a842d

blink1073 requested a review from NoahStapp June 25, 2026 20:40

NoahStapp requested changes Jun 25, 2026

View reviewed changes

blink1073 requested a review from NoahStapp June 25, 2026 21:27

NoahStapp approved these changes Jun 26, 2026

View reviewed changes

		from pymongo.typings import _Address


		class _ConnectionTelemetryInfo(Protocol):

Uh oh!

Conversation

blink1073 commented Jun 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes in this PR

Test Plan

Checklist

Checklist for Author

Checklist for Reviewer

Uh oh!

codecov-commenter commented Jun 24, 2026

Codecov Report

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

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!

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!

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!

Choose a reason for hiding this comment

Uh oh!

blink1073 Jun 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

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!

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!

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!

NoahStapp left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

blink1073 commented Jun 24, 2026 •

edited

Loading

blink1073 Jun 25, 2026 •

edited

Loading