-
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
Porting sqlalchemy instrumentation from contrib repo #591
Conversation
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 did a first pass and it looks great. I compared to the DataDog implementation and I think the porting you did is just right.
Most of the comments are just nits, maybe we only important one is about updating the assert states to self.assert*...
I tested it with the autoinstrumentation command by running and example and it works without issues.
Should we add an entry in the documentation for it?
...elemetry-instrumentation-sqlalchemy/src/opentelemetry/instrumentation/sqlalchemy/__init__.py
Outdated
Show resolved
Hide resolved
...ntelemetry-instrumentation-sqlalchemy/src/opentelemetry/instrumentation/sqlalchemy/engine.py
Outdated
Show resolved
Hide resolved
...ntelemetry-instrumentation-sqlalchemy/src/opentelemetry/instrumentation/sqlalchemy/engine.py
Outdated
Show resolved
Hide resolved
...ntelemetry-instrumentation-sqlalchemy/src/opentelemetry/instrumentation/sqlalchemy/engine.py
Outdated
Show resolved
Hide resolved
...ntelemetry-instrumentation-sqlalchemy/src/opentelemetry/instrumentation/sqlalchemy/engine.py
Outdated
Show resolved
Hide resolved
...ntelemetry-instrumentation-sqlalchemy/src/opentelemetry/instrumentation/sqlalchemy/engine.py
Outdated
Show resolved
Hide resolved
...ntelemetry-instrumentation-sqlalchemy/src/opentelemetry/instrumentation/sqlalchemy/engine.py
Outdated
Show resolved
Hide resolved
@@ -0,0 +1,13 @@ | |||
# Copyright The OpenTelemetry Authors |
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.
Nit: this file could just be empty.
ext/opentelemetry-ext-docker-tests/tests/sqlalchemy_tests/mixins.py
Outdated
Show resolved
Hide resolved
ext/opentelemetry-ext-docker-tests/tests/sqlalchemy_tests/test_patch.py
Outdated
Show resolved
Hide resolved
Co-Authored-By: Mauricio Vásquez <mauricio@kinvolk.io>
Is Manifest.in also missing? Example: https://github.com/open-telemetry/opentelemetry-python/blob/master/ext/opentelemetry-ext-dbapi/MANIFEST.in. |
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:
- I clearly can see how it is based on DataDog donation
- Documentation is present
- Unit tests are present
- Integration tests are implemented and working
ext/opentelemetry-ext-docker-tests/tests/sqlalchemy_tests/test_mysql.py
Outdated
Show resolved
Hide resolved
ext/opentelemetry-ext-docker-tests/tests/sqlalchemy_tests/test_postgres.py
Outdated
Show resolved
Hide resolved
Co-Authored-By: Mauricio Vásquez <mauricio@kinvolk.io>
.. |pypi| image:: https://badge.fury.io/py/opentelemetry-ext-sqlalchemy.svg | ||
:target: https://pypi.org/project/opentelemetry-ext-sqlalchemy/ | ||
|
||
This library allows tracing requests made by the SQLAlchemy library. |
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.
Maybe it might be good to call out popular databases that we don't have integrations for that are supported by SQLAlchemy? Or include a link? I get some customers that are confused on which integrations to use for their databases, and we should have a consistent recommendation. I believe we should recommend the specific integrations if they exist (like for mysql), and to try to use SQLAlchemy otherwise (if supported).
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 added a link to sqlalchemy to the reference section. Regarding recommendations, I wonder where the best place to put something like this is, seems like it could be part of the examples section 🤔
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.
Maybe we can just verbally indicate in the README: "For MYSQL use the integration(link), for postgres use the integration(link). For all other databases that support SQLAlchemy, use this..."
ext/opentelemetry-ext-sqlalchemy/src/opentelemetry/ext/sqlalchemy/__init__.py
Outdated
Show resolved
Hide resolved
ext/opentelemetry-ext-sqlalchemy/src/opentelemetry/ext/sqlalchemy/engine.py
Outdated
Show resolved
Hide resolved
import sqlalchemy | ||
|
||
trace.set_tracer_provider(TracerProvider()) | ||
engine = create_engine("sqlite:///:memory:") |
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 think you need to import create_engine
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! thanks
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! Few 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, probably just needs minor lint fixes to pass those tests again.
Porting the existing sqlalchemy instrumentation from the contrib repo to using the OpenTelemetry API and the OpenTelemetry Auto-instrumentation Patcher interface.
This is replacing open-telemetry/opentelemetry-python-contrib#22