Added patching logic to main.py #9

juliusgeo · 2020-07-16T02:20:40Z

No description provided.

ShaneHarvey · 2020-07-16T18:28:36Z

pymongoexplain/__main__.py


-    def succeeded(self, event):
-        pass
+def update_one(self: Collection, filter, update, upsert=False,


What do you think about doing this instead?

def update_one(self, *args, **kwargs): ExplainCollection(self).update_one(*args, **kwargs) return old_update_one(self, *args, **kwargs)

Then we might also be able to auto generate all the methods too!

ShaneHarvey

Nice work! Just a few comments on style and the output format.

ShaneHarvey · 2020-07-16T21:40:48Z

pymongoexplain/__main__.py

-from pymongo.collection import Collection
+from pymongo.collection import Collection, ReturnDocument
+from pymongo.cursor import CursorType
+from pymongo.common import BaseObject


Let's remove the imports that are no longer used.

ShaneHarvey · 2020-07-16T21:43:32Z

pymongoexplain/__main__.py

-        pass
+def make_func(old_func, old_func_name):
+    def new_func(self: Collection, *args, **kwargs):
+        print(getattr(ExplainCollection(self), old_func_name)(*args,


What do you think about using the logging module instead of print? See https://docs.python.org/3/howto/logging.html#logging-basic-tutorial

ShaneHarvey

Can you show use some sample output of this script?

ShaneHarvey · 2020-07-16T22:09:21Z

pymongoexplain/__main__.py


+for old_func, old_func_name in zip(old_functions, old_function_names):
+    setattr(Collection, old_func_name, make_func(old_func,
+                                                 old_func_name))


I think this can be one line:

setattr(Collection, old_func_name, make_func(old_func, old_func_name))

ShaneHarvey · 2020-07-16T22:10:14Z

pymongoexplain/__main__.py

-
-
-    print(sys.argv)
+            exec(f.read())


Newline at end of file.

juliusgeo · 2020-07-17T13:47:47Z

Can you show use some sample output of this script?

from pymongo import MongoClient
client = MongoClient(serverSelectionTimeoutMS=1000)
collection = client.db.products
collection.update_one({"quantity": 1057, "category": "apparel"},{"$set": {"reorder": True}})

This outputs the following:

2020-07-16 18:02:14,209 INFO __main__ {'queryPlanner': {'plannerVersion': 1, 'namespace': 'db.products', 'indexFilterSet': False, 'parsedQuery': {'$and': [{'category': {'$eq': 'apparel'}}, {'quantity': {'$eq': 1057}}]}, 'queryHash': 'CD8F6D8F', 'planCacheKey': 'CD8F6D8F', 'winningPlan': {'stage': 'UPDATE', 'inputStage': {'stage': 'COLLSCAN', 'filter': {'$and': [{'category': {'$eq': 'apparel'}}, {'quantity': {'$eq': 1057}}]}, 'direction': 'forward'}}, 'rejectedPlans': []}, 'serverInfo': {'host': 'Juliuss-MBP.verizon.net', 'port': 27017, 'version': '4.4.0-rc10', 'gitVersion': '5218ee3e883b0230e121ae13a7640e0bc4a313ae'}, 'ok': 1.0, '$clusterTime': {'clusterTime': Timestamp(1594936925, 1), 'signature': {'hash': b'\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00', 'keyId': 0}}, 'operationTime': Timestamp(1594936925, 1)}

ShaneHarvey · 2020-07-17T19:29:09Z

pymongoexplain/__main__.py

-        pass
+def make_func(old_func, old_func_name):
+    def new_func(self: Collection, *args, **kwargs):
+        logging.info(getattr(ExplainCollection(self), old_func_name)(*args,


Could you add some context to the long line? Maybe something as simple as "update_one explained: {...}"?

Context on what it does? Or just an example of the output?

More context in the log line itself. Right now we just log the response:

2020-07-16 18:02:14,209 INFO __main__ {'queryPlanner': ...}

It would be nicer to include the method name being explained so that the user know what the output is for. Like this:

2020-07-16 18:02:14,209 INFO __main__ update_one explain response: {'queryPlanner': ...} 2020-07-16 18:02:14,209 INFO __main__ find_one explain response: {'queryPlanner': ...}

oh I see what you mean. Will do

ShaneHarvey · 2020-07-17T21:25:48Z

pymongoexplain/__main__.py

+def make_func(old_func, old_func_name):
+    def new_func(self: Collection, *args, **kwargs):
+        logging.info("%s explain response: %s"% (old_func_name, getattr(
+            ExplainCollection(self),old_func_name)(*args, **kwargs)))


I suggest refactoring this slightly to make the code a bit more readable:

res = getattr(ExplainCollection(self), old_func_name)(*args, **kwargs) logging.info("%s explain response: %s", old_func_name, res)

Note that with the logging methods you should pass the format string arguments without using % on the format string directly. See https://docs.python.org/3/howto/logging.html#logging-variable-data

ShaneHarvey

Nice work! I think there's some follow up work we need to do in order to:

Test __main__.py on travis.
Enable support for script arguments:

$ python -m pymongoexplain script.py # This works
$ python -m pymongoexplain script.py --arg=scriptArgument ...  # This does not

Could you open an issue to track those two improvements?

prashantmital

Good start - we can merge this for now but it definitely needs some more testing.

prashantmital · 2020-07-20T16:16:55Z

pymongoexplain/__main__.py

-    def __init__(self):
-        self.payloads = []
+FORMAT = '%(asctime)s %(levelname)s %(module)s %(message)s'
+logging.basicConfig(format=FORMAT, level=logging.INFO)


We need to investigate how us configuring logging in __main__ might interact with the script being run configuring logging as well. I think if it is technically feasible, we should try to ensure:

that the logging configuration in main does not make the logger less verbose than as configured by the script, and

that the script cannot configure logging in a manner that would inadvertently suppress explain output.

I don't necessarily think that these objectives need to be achieved in code - good documentation should also suffice. Please open an issue to track this work.

prashantmital · 2020-07-20T16:21:18Z

pymongoexplain/__main__.py

 import sys
-from bson.son import SON
+import logging


Can you add a brief module-level docstring?

prashantmital · 2020-07-20T16:24:08Z

pymongoexplain/__main__.py


+for old_func, old_func_name in zip(old_functions, old_function_names):
+    setattr(Collection, old_func_name, make_func(old_func, old_func_name))


Did you confirm that this patches Collection no matter what namespace it is imported from? In other words, do we know that the following two cases would both see the same behavior:

import pymongo pymongo.Collection(...).find(...)

and

from pymongo import Collection Collection(...).find(...)

We also want the patching to work when PyMongo itself returns a Collection (e.g. via Database.get_collection). Checking once manually on your local machine should suffice for now, though maybe you can add a note about testing these cases to the ticket Shane has asked you to open above.

I tested those two cases and it still works.

Added patching

961a290

juliusgeo requested review from prashantmital and ShaneHarvey July 16, 2020 18:21

ShaneHarvey reviewed Jul 16, 2020

View reviewed changes

Made monkey patching much more compact

977fbdb

ShaneHarvey reviewed Jul 16, 2020

View reviewed changes

juliusgeo added 2 commits July 16, 2020 17:53

fix formatting and unused imports

3ebabe0

added logging

44d75d8

ShaneHarvey reviewed Jul 16, 2020

View reviewed changes

fixed whitespace

900fd81

ShaneHarvey reviewed Jul 17, 2020

View reviewed changes

add more details to logging

ac8cf95

ShaneHarvey reviewed Jul 17, 2020

View reviewed changes

shane fix formatting

2c9e31c

juliusgeo requested a review from ShaneHarvey July 17, 2020 22:32

ShaneHarvey approved these changes Jul 17, 2020

View reviewed changes

prashantmital approved these changes Jul 20, 2020

View reviewed changes

add docstring

309695f

juliusgeo merged commit b7d547f into mongodb-labs:master Jul 20, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added patching logic to main.py #9

Added patching logic to main.py #9

juliusgeo commented Jul 16, 2020

ShaneHarvey Jul 16, 2020 •

edited

Loading

juliusgeo Jul 16, 2020

ShaneHarvey left a comment

ShaneHarvey Jul 16, 2020

ShaneHarvey Jul 16, 2020

ShaneHarvey left a comment

ShaneHarvey Jul 16, 2020

ShaneHarvey Jul 16, 2020

juliusgeo commented Jul 17, 2020

ShaneHarvey Jul 17, 2020

juliusgeo Jul 17, 2020

ShaneHarvey Jul 17, 2020 •

edited

Loading

juliusgeo Jul 17, 2020

ShaneHarvey Jul 17, 2020

ShaneHarvey left a comment •

edited

Loading

prashantmital left a comment

prashantmital Jul 20, 2020

prashantmital Jul 20, 2020

juliusgeo Jul 20, 2020

prashantmital Jul 20, 2020

prashantmital Jul 20, 2020

juliusgeo Jul 20, 2020


		for old_func, old_func_name in zip(old_functions, old_function_names):
		setattr(Collection, old_func_name, make_func(old_func, old_func_name))

Added patching logic to __main__.py #9

Added patching logic to __main__.py #9

Conversation

juliusgeo commented Jul 16, 2020

ShaneHarvey Jul 16, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ShaneHarvey left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ShaneHarvey left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

juliusgeo commented Jul 17, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ShaneHarvey Jul 17, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ShaneHarvey left a comment • edited Loading

Choose a reason for hiding this comment

prashantmital left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Added patching logic to main.py #9

Added patching logic to main.py #9

ShaneHarvey Jul 16, 2020 •

edited

Loading

ShaneHarvey Jul 17, 2020 •

edited

Loading

ShaneHarvey left a comment •

edited

Loading