PYTHON-5745 Consolidate command telemetry#2891
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
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.pywith_CommandTelemetryto unify STARTED/SUCCEEDED/FAILED command logging + APM publishing and duration tracking. - Updated sync/async
command_runner.pyto delegate command telemetry to_CommandTelemetryand simplified call signatures (removing explicitaddress/startplumbing). - Introduced
_ConnectionTelemetryInfoprotocol 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. |
| self._duration = datetime.datetime.now() - self._start | ||
| if not self._should_log and not self._publish: | ||
| return | ||
| duration = self._duration |
There was a problem hiding this comment.
Do we need this assignment? self._duration should only get assigned once per _CommandTelemetry lifecycle.
| self._duration = datetime.datetime.now() - self._start | ||
| if not self._should_log and not self._publish: | ||
| return | ||
| duration = self._duration |
| request_id: int, | ||
| op_id: Optional[int], | ||
| ) -> None: | ||
| self._topology_id = topology_id |
There was a problem hiding this comment.
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?
| self._start: datetime.datetime | ||
| self._duration: datetime.timedelta | ||
|
|
||
| def started(self, orig: MutableMapping[str, Any], ensure_db: bool) -> None: |
There was a problem hiding this comment.
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.
| from pymongo.typings import _Address | ||
|
|
||
|
|
||
| class _ConnectionTelemetryInfo(Protocol): |
There was a problem hiding this comment.
Is there a reason to use this over the existing _AgnosticConnection?
PYTHON-5745
Changes in this PR
Introduces
pymongo/_telemetry.pywith a_CommandTelemetryclass 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
Checklist for Reviewer