-
Notifications
You must be signed in to change notification settings - Fork 648
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
SQLite3 Instrumentation #719
SQLite3 Instrumentation #719
Conversation
11e94e5
to
c906159
Compare
c906159
to
ece7b41
Compare
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.
Looks great!
|
||
cnx = sqlite3.connect('example.db') | ||
cursor = cnx.cursor() | ||
cursor.execute("INSERT INTO test (testField) VALUES (123)" |
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.
Missing closing ")"?
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.
fixed
tox.ini
Outdated
@@ -192,6 +197,7 @@ commands_pre = | |||
flask,django: pip install {toxinidir}/opentelemetry-auto-instrumentation | |||
flask: pip install {toxinidir}/ext/opentelemetry-ext-flask[test] | |||
|
|||
dbapi: pip install {toxinidir}/opentelemetry-auto-instrumentation |
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 need the auto-instrumentation module for this test suite?
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.
not anymore; removed
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. Some non-blocking comments
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, it would be nice to add docker tests for this integration, maybe creating an issue for it so we don't forget.
@cnnradams if that's the case, can you file a ticket and pick this up as a followup PR? Would love to pay down this debt as we go along. Regarding where to put this code, I'm open to either a more specific shared package (e.g. opentelemetry-instrumentation-dbapi), or a mega-shared package for all shared utils across all instrumentations. @codeboten @c24t for their thoughts. In the meantime I'll merge this PR. Thanks! |
test: fix and add tests closes open-telemetry#642 Signed-off-by: Olivier Albertini <[email protected]>
Adds instrumentation for python sqlite3 library.
There is a lot of repeat code in the SQLite3Instrumentor (shared with every other DBAPI instrumentor), it could possibly be pulled out into a DBAPIInstrumentor that can be subclassed in this PR or another.
Resolves #683