PYTHON-5977 APM/logging hot-path optimizations - #2968
Conversation
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
|
Failures are unrelated. |
There was a problem hiding this comment.
Pull request overview
This PR targets performance regressions on low-latency, high-op-count workloads by removing avoidable APM/logging/telemetry work from hot paths when telemetry is disabled, while preserving existing behavior when telemetry is enabled.
Changes:
- Gate operation-id generation and telemetry object construction behind “telemetry enabled” checks (APM listeners and/or DEBUG logging), especially on retry and bulk-write paths.
- Reduce hot-path overhead in command execution, CMAP checkout/checkin, and server selection by skipping telemetry/logging work when disabled.
- Add regression tests to ensure no RawBSON reply inflation and no operation-id generation when telemetry is disabled.
Reviewed changes
Copilot reviewed 19 out of 19 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| pymongo/_telemetry.py | Adds helper gating and streamlines command/CMAP/SDAM telemetry to avoid work when disabled. |
| pymongo/monitoring.py | Clarifies speculative-authentication redaction guard for potential RawBSONDocument inputs. |
| pymongo/message.py | Allows passing optional operation_id through bulk-write context types. |
| pymongo/asynchronous/command_runner.py | Skips command telemetry construction when APM/logging disabled; returns duration as seconds internally. |
| pymongo/synchronous/command_runner.py | Sync equivalent of the async command-runner hot-path gating and duration handling. |
| pymongo/asynchronous/mongo_client.py | Centralizes op-id generation gating for retries via _should_generate_op_id. |
| pymongo/synchronous/mongo_client.py | Sync equivalent of op-id generation gating for retries. |
| pymongo/asynchronous/bulk.py | Avoids op-id generation for bulk writes when telemetry disabled. |
| pymongo/synchronous/bulk.py | Sync equivalent bulk-write op-id gating. |
| pymongo/asynchronous/client_bulk.py | Avoids op-id generation for client bulk writes when telemetry disabled. |
| pymongo/synchronous/client_bulk.py | Sync equivalent client-bulk op-id gating and type adjustments. |
| pymongo/asynchronous/pool.py | Adds fast paths to skip CMAP telemetry/logging calls when disabled. |
| pymongo/synchronous/pool.py | Sync equivalent pool fast paths for CMAP telemetry/logging. |
| pymongo/asynchronous/topology.py | Avoids server-selection telemetry object creation and success logging when server-selection logging is disabled. |
| pymongo/synchronous/topology.py | Sync equivalent server-selection logging gating. |
| test/asynchronous/test_operation_id_retry.py | Adds coverage ensuring no op-id generation when telemetry disabled; covers bulk/client-bulk. |
| test/test_operation_id_retry.py | Sync generated equivalent test coverage for no-op-id behavior when telemetry disabled. |
| test/asynchronous/test_monitoring.py | Adds test ensuring CommandSucceededEvent redaction checks don’t inflate RawBSON replies for non-hello commands. |
| test/test_monitoring.py | Sync generated equivalent of the RawBSON inflation regression test. |
Suppressed comments (1)
pymongo/_telemetry.py:108
- Grammar: “fast path … inline this gate” should use the verb form “inlines” (and the following sentence should refer to a singular subject).
# NOTE: the _run_command fast path in command_runner.py inline this gate for performance
# They must be kept in sync with any gating changes
| # Fast path: skip telemetry construction when logging and APM are disabled | ||
| # Inline enabled check here for performance | ||
| telemetry: Optional[_CommandTelemetry] = None | ||
| if (topology_id is not None and _COMMAND_LOGGER.isEnabledFor(logging.DEBUG)) or ( |
There was a problem hiding this comment.
I think this part should be a helper in _telemetry.py
There was a problem hiding this comment.
This is intentionally inlined here for performance since this is called on every single operation, moving to a helper is a small performance hit that compounds across each time the inlined check is abstracted.
| self.active_contexts.discard(conn.cancel_context) | ||
| self._telemetry.checked_in(conn.id) | ||
| telemetry = self._telemetry | ||
| if telemetry._should_publish or ( |
There was a problem hiding this comment.
This logic should also be in _telemetry.py, perhaps as a property on the class
There was a problem hiding this comment.
Same here as above, inlined intentionally for performance.
| checkout_started_time = pool._telemetry.checkout_started() | ||
| telemetry = pool._telemetry | ||
| # Fast path: skip telemetry calls when CMAP events/logging are disabled | ||
| if not telemetry._should_publish and not ( |
There was a problem hiding this comment.
Also could be factored into _telemetry.py
There was a problem hiding this comment.
Same here as well.
| ss.started() | ||
| # Server selection does not have APM events, gate only on logging | ||
| ss: Optional[_ServerSelectionTelemetry] = None | ||
| if _SERVER_SELECTION_LOGGER.isEnabledFor(logging.DEBUG): |
There was a problem hiding this comment.
Also can be factored into _telemetry.py
| server.description.address[0], | ||
| server.description.address[1], | ||
| ) | ||
| if _SERVER_SELECTION_LOGGER.isEnabledFor(logging.DEBUG): |
There was a problem hiding this comment.
Same here, we shouldn't need to have this type of logic in multiple places.
| } | ||
| op_id = _randint() | ||
| client = self.collection.database.client | ||
| op_id = _randint() if _should_generate_op_id(client._event_listeners) else None |
There was a problem hiding this comment.
Maybe this function should either return a _randint or None itself
PYTHON-5977
Changes in this PR
The changes introduced by PYTHON-5903, PYTHON-5846, PYTHON-5745, and PYTHON-5676 caused small but measurable regressions for low-latency, high operation count workloads such as the DSI locust_ping benchmark. Much of this is due to constructing telemetry objects even when APM/logging is disabled and other similar changes that lie on the hot path for such workloads.
This PR attempts to optimize all of those regressions away and return performance to its point before their addition.
Test Plan
Standard test suite, benchmarks.
Results of a simple ping workload benchmark:
Checklist
Checklist for Author
[ ] Did you update the changelog (if necessary)?[ ] Is there test coverage?[ ] Is any followup work tracked in a JIRA ticket? If so, add link(s).Checklist for Reviewer