PYTHON-3459 Add log messages to Server selection spec #1511
Conversation
pymongo/operations.py
Outdated
| @@ -44,6 +45,35 @@ | |||
| _IndexKeyHint = Union[str, _IndexList] | |||
|
|
|||
|
|
|||
| class _Operations(str, enum.Enum): | |||
| ABORT_TRANSACTION_OP = "abortTransaction" | |||
There was a problem hiding this comment.
I'd optimzie for shorter names here. We can ditch the _OP suffix since these are all nested a named enum. We could also shorten _Operations to _Op so these become: _Op.ABORT
pymongo/collection.py
Outdated
| @@ -970,6 +979,7 @@ def replace_one( | |||
| session=session, | |||
| let=let, | |||
| comment=comment, | |||
| operation="replace", | |||
There was a problem hiding this comment.
Does this spec require this to be "replace" or should it instead be "update" (the command name).
There was a problem hiding this comment.
It should be update, good catch.
pymongo/mongo_client.py
Outdated
| server = topology.select_server_by_address(tuple(address), operation="killCursors") # type: ignore[arg-type] | ||
| else: | ||
| # Application called close_cursor() with no address. | ||
| server = topology.select_server(writable_server_selector) | ||
| server = topology.select_server(writable_server_selector, operation="killCursors") |
There was a problem hiding this comment.
Should killCursors be in the Enum?
pymongo/bulk.py
Outdated
| @@ -408,6 +408,7 @@ def execute_command( | |||
| generator: Iterator[Any], | |||
| write_concern: WriteConcern, | |||
| session: Optional[ClientSession], | |||
| operation: Optional[str] = "TEST_OPERATION", | |||
There was a problem hiding this comment.
Can we just have this be _Op.TEST?
Additionally, for typing, is there a case where we would pass operation=None because if not, we could also type this to just be of type str.
There was a problem hiding this comment.
Found a case reading through, so ignore the second part :)
There was a problem hiding this comment.
Good catch. Agree we should remove "TEST_OPERATION" and make this a required arg.
There was a problem hiding this comment.
I've made operation required everywhere it appears for consistency with the spec and for ease of future refactors/development.
pymongo/bulk.py
Outdated
| @@ -408,6 +408,7 @@ def execute_command( | |||
| generator: Iterator[Any], | |||
| write_concern: WriteConcern, | |||
| session: Optional[ClientSession], | |||
| operation: Optional[str] = "TEST_OPERATION", | |||
There was a problem hiding this comment.
Found a case reading through, so ignore the second part :)
pymongo/database.py
Outdated
| @@ -786,6 +790,7 @@ def command( | |||
| codec_options: Optional[bson.codec_options.CodecOptions[_CodecDocumentType]] = None, | |||
| session: Optional[ClientSession] = None, | |||
| comment: Optional[Any] = None, | |||
| operation: Optional[str] = None, | |||
There was a problem hiding this comment.
We'll have to update the docstring here.
| if read_preference is None: | ||
| read_preference = (session and session._txn_read_preference()) or ReadPreference.PRIMARY | ||
| with self.__client._conn_for_reads(read_preference, session) as ( | ||
| with self.__client._conn_for_reads(read_preference, session, operation=command_name) as ( |
There was a problem hiding this comment.
why do we pass command_name here instead of operation?
There was a problem hiding this comment.
When explicitly using database.command, we always want to use the command name in the command document. I've removed operation entirely here as it's unneeded.
For additional context, the spec for server selection logging requires that every time we select a server, we log the operation we plan to send to the server. Since we do not have a unified operations layer (see PYTHON-1833), this requires a large number of scattered changes to pass the operation name all the way down to server selection.
The spec tests are quite minimal, presumably because they assume drivers have a unified operations layer that makes logging operation names easy. I've added additional unified format tests for most of the operations we have separate handlers for. Depending on other driver implementations, these tests may be useful--I'll open a DRIVERS ticket to start the discussion for possible inclusion in the spec tests.