Skip to content
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

fix enable_commenter functionality #1440

Merged

Conversation

avzis
Copy link
Contributor

@avzis avzis commented Nov 13, 2022

Description

Fix enable_commenter functionality.
Fixes #1368

Now you can either:

  1. call instrument with an existing engine, and enable or disable sql commenter with the enable_commenter param,
    like this:
SQLAlchemyInstrumentor().instrument(
    engine=engine,
    enable_commenter=True,
)
  1. call instrument without an engine, and create it separately,
    note: it is important to import the create_engine function after calling instrument():
SQLAlchemyInstrumentor().instrument(enable_commenter=True)
from sqlalchemy import create_engine
engine = create_engine("sqlite:///:memory:")

Type of change

Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

Now the tests are covering all 4 cases:
sqlcommenter enabled and disabled, both when instrumenting with engine, and when creating the engine after instrumentation.

  • test_sqlcommenter_disabled()
  • test_sqlcommenter_enabled()
  • test_sqlcommenter_enabled_create_engine_after_instrumentation()
  • test_sqlcommenter_disabled_create_engine_after_instrumentation()

Does This PR Require a Core Repo Change?

No.

@avzis avzis marked this pull request as ready for review December 4, 2022 15:21
@avzis avzis requested a review from a team December 4, 2022 15:21
@@ -49,27 +49,29 @@ def _get_tracer(tracer_provider=None):
)


def _wrap_create_async_engine(tracer_provider=None):
def _wrap_create_async_engine(tracer_provider=None, enable_commenter=False):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the default parameters here are redundant, since they are always passed from the callee funtions

@srikanthccv srikanthccv enabled auto-merge (squash) December 5, 2022 17:00
auto-merge was automatically disabled December 6, 2022 12:26

Head branch was pushed to by a user without write access

@avzis avzis requested a review from srikanthccv December 6, 2022 13:10
@srikanthccv srikanthccv merged commit cfd017e into open-telemetry:main Dec 6, 2022
@avzis avzis deleted the fix-enable_commenter-in-sql-alchemy branch December 27, 2022 13:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SQLAlchemy Instrumentation: enable_commenter can't be disabled.
4 participants