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

[mysql] distinguish mysql-python instrumentation #263

Merged
merged 1 commit into from May 9, 2017

Conversation

palazzem
Copy link

@palazzem palazzem commented May 7, 2017

What it does

The current mysql integration supports only mysql-connector package. Other libraries such as MySQL-Python, must be handled differently. However, the current integration loader ( available in the monkey module) doesn't give the right feedback to developers, and indeed the error output is misleading.

Testing the loader

import logging

logger = logging.getLogger()
logger.setLevel(logging.DEBUG)

from ddtrace import patch, patch_all
patch_all()

The library MySQL-Python is installed in the environment so patch_all() should not find this integration.

Before the patch

DEBUG:root:failed to patch mysql: module not installed

It seems that mysql is not installed while it is.

After the patch

DEBUG:root:failed to patch mysql: integration not available

The integration mysql is not available for this package.

@palazzem palazzem added this to the 0.8.2 milestone May 7, 2017
@palazzem palazzem requested a review from LotharSee May 7, 2017 18:41
# MySQL python package is not supported at the moment;
# here we raise an import error so that the external
# loader knows that the integration is not available
raise ImportError('No module named mysql-python')
Copy link
Author

Choose a reason for hiding this comment

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

The error must be an ImportError because it's like if we're trying to import a module that is not here. However, the message may be different?

@palazzem palazzem modified the milestones: 0.8.2, 0.8.3 May 7, 2017
Copy link
Contributor

@vlad-mh vlad-mh left a comment

Choose a reason for hiding this comment

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

I'm completely fine with this patch.

Though I'm having a hard time seing why we don't just do import _mysql and let the python runtime raise for us, instead of using utils.require_module. Did I miss something?

@palazzem
Copy link
Author

palazzem commented May 8, 2017

This change is only temporary and at some point we will have the right instrumentation here (so the __init__.py knows which instrumentation must be activated). I've left the structure as before so that we should only remove the ImportError and add our imports.

Anyway, here we still have 2 different libraries that should not fail fast at the first ImportError.

@vlad-mh
Copy link
Contributor

vlad-mh commented May 8, 2017

Makes sense. LGTM

@palazzem
Copy link
Author

palazzem commented May 8, 2017

Thanks for the review @MattHauglustaine ! We still have room of improvement here! 👍

Copy link
Contributor

@LotharSee LotharSee left a comment

Choose a reason for hiding this comment

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

LGTM

@palazzem palazzem changed the base branch from palazzem/patch_all-logger to master May 9, 2017 08:55
@palazzem palazzem merged commit c0be8e6 into master May 9, 2017
@palazzem palazzem deleted the palazzem/mysql-python-safeguard branch May 9, 2017 08:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants