Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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/pymysql: Add Instrumentor #611
ext/pymysql: Add Instrumentor #611
Changes from 15 commits
a27027b
6ddbeb5
2ab5e5b
3a54847
7123ebb
7ea6b3b
c76ade0
03fa1d5
e0be0de
4a4c167
7a070af
2722ebb
3430446
9f3120e
249d87f
3625a85
7712252
00e29a3
bede60b
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Any reason to not use BaseInstrumentor in here as well?, people could be using dbapi instrumentor directly
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.
BaseInstrumentor
is used to implement an interface that allows to instrument all the calls made to a given library, for instance pymysql. AFAIU dbapi is a specification and not a library, so having an instrumentor here doesn't make sense. If a person wants to use dpapi directly, they should use thewrap_connect
andunwrap_connect
functions.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.
Should document this? In the docstrings
trace_integration()
is the recommended way to instrument. Perhaps we should expose an API method to uninstrument (since there isn't really a semantic similarity betweentrace_integration
andunwrap_connect
.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.
The PyMySQL docstrings was wrong,
trace_integration
doesn't exist anymore.PymysqlInstrumentor
offers two methods,instrument
anduninstrument
.I'm not sure what to do with the
trace_integration
on dpapi, it's very similar towrap_connect
but creates a tracer before. Maybe we could remove it and let the user play directly withwrap_connect
...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.
User is not returned as string in some cases?
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.
Right, PyMySQL returns bytes and it was causing problems in the console exporter. I opened an issue about it #623.
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.
Please enclose this between
logging.disable
calls to suppress unnecessary logging: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.
Thanks, I followed a similar approach.
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.
Is the [test] for using TestBase? How does this work?
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.
It's a way to tell pip to install opentelemetry-ext-pymysql and the dependencies on the extra test section: https://github.com/open-telemetry/opentelemetry-python/pull/611/files#diff-3874eac15d9a093a3db07c9283d97d12R48