-
Notifications
You must be signed in to change notification settings - Fork 595
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
ext/pymongo: Add instrumentor #612
ext/pymongo: Add instrumentor #612
Conversation
The current Pymongo integration uses the monitoring.register() [1] to hook the different internal calls of Pymongo. This integration doesn't allow to unregister a monitor. This commit workaround that limitation by adding an enable flag to the CommandTracer class and adds a logic to disable the integration. This solution is not perfect becasue there will be some overhead even when the instrumentation is disabled, but that's what we can do with the current approach. [1] https://api.mongodb.com/python/current/api/pymongo/monitoring.html#pymongo.monitoring.register
Pop span instead of getting and then removing it.
ext/opentelemetry-ext-pymongo/src/opentelemetry/ext/pymongo/__init__.py
Outdated
Show resolved
Hide resolved
ext/opentelemetry-ext-pymongo/src/opentelemetry/ext/pymongo/__init__.py
Outdated
Show resolved
Hide resolved
ext/opentelemetry-ext-pymongo/src/opentelemetry/ext/pymongo/__init__.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM only added some small suggestions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Thanks.
ext/opentelemetry-ext-pymongo/src/opentelemetry/ext/pymongo/__init__.py
Outdated
Show resolved
Hide resolved
|
||
def _remove_span(self, event): | ||
self._span_dict.pop(_get_span_dict_key(event)) | ||
if not self.enable: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I notice this snippet is almost identical to the above code. Don't know if it's worth refactoring into a utility method though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you mean the following snippet?
if not self.is_enabled:
return
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nvm. I maybe be going crazy, but I thought a diff verifying significantly more standard attributes like db type.
closes open-telemetry#612 Signed-off-by: Olivier Albertini <olivier.albertini@montreal.ca>
This PR implements the Instrumentor interface for the Pymongo integration. It also implements a way to disable instrumentation.
I compared with the DataDog donated implementation available at https://github.com/open-telemetry/opentelemetry-python-contrib/tree/master/reference/ddtrace/contrib/pymongo, the already existing implementation is good enough and we do not need to take any from that in this case.
I'm opening a new PR to supersedes #598 to avoid reviewing and approving my own changes there.
How to try:
pymongo_example.py:
If everything went fine you see some spans on the terminal.