Skip to content
This repository was archived by the owner on Sep 17, 2025. It is now read-only.

Componentize the remaining integration extensions (2nd part)#540

Merged
reyang merged 6 commits intomasterfrom
componentization
Mar 6, 2019
Merged

Componentize the remaining integration extensions (2nd part)#540
reyang merged 6 commits intomasterfrom
componentization

Conversation

@reyang
Copy link
Copy Markdown
Contributor

@reyang reyang commented Mar 5, 2019

This PR follows the approach in #535 for the remaining extensions.
This is part of the effort for #445.

  1. Reverted opencensus/trace/config_integration.py (which has been changed in Componentize the remaining integration extensions (1st part) #535) back since all the extensions are now under the same namespace.
  2. Put test_utils.py under tests/unit/common.
  3. Abandoned the opencensus/trace/ext folder.
  4. Updated CHANGELOG.md.

@reyang reyang requested review from a team, c24t and songy23 as code owners March 5, 2019 21:51
'requests': 'opencensus.trace.ext.requests.trace',
'google_cloud_clientlibs': 'opencensus.trace.ext.google_cloud_clientlibs.trace', # noqa
'threading': 'opencensus.trace.ext.threading.trace',
SUPPORTED_INTEGRATIONS = ['httplib', 'mysql', 'postgresql', 'pymysql',
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@c24t I wish to know your opinion on this.

This "config integration" seems to be introduced for:

  1. Frameworks such like Django which prefers configuration.
  2. Convenience, e.g. having settings.py file (or even environment variable) which can be updated easily.

From dependency perspective, this is like "reflection", the core package is "depending" on contrib packages.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I would prefer to keep this feature, but refactor in the following way:

  1. remove hard-coded integration names.
  2. scan opencensus.ext namespace, try to import the extensions.
  3. log warning if module not found.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

That sounds like a good plan to me, although scanning installed packages may introduce some problems. Another complicating factor is that not every integration implements trace.trace_integration.

session.install('-e', 'contrib/opencensus-ext-requests')
session.install('-e', 'contrib/opencensus-ext-sqlalchemy')
session.install('-e', 'contrib/opencensus-ext-threading')
session.install('-e', 'contrib/opencensus-ext-google-cloud-clientlibs')
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Here we don't follow the alphabetic order since ext-google-cloud-clientlibs has dependency on ext-grpc and ext-requests.
Ideally we don't want to maintain the order here, just having someting like "install all the packages under contrib, let pip figure out the dependency, resolve package dependency from contrib folder before consulting PyPI". Currently I haven't figured out how to do this.

Copy link
Copy Markdown
Member

@c24t c24t left a comment

Choose a reason for hiding this comment

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

Can you remove wrapt from setup.py now that requests is out in its own package?

Copy link
Copy Markdown
Member

@c24t c24t left a comment

Choose a reason for hiding this comment

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

This LGTM, although we may need to revisit the maximum versions for contrib packages as we update the library. Glad to see all these packages moved!

@reyang
Copy link
Copy Markdown
Contributor Author

reyang commented Mar 6, 2019

Can you remove wrapt from setup.py now that requests is out in its own package?

Yep! I'll do the clean up in my next PR (clean up dependencies, move the samples).

@reyang reyang merged commit 80455ed into master Mar 6, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants