Skip to content

[core] vendor wrapt and six dependencies#755

Merged
brettlangdon merged 28 commits into
0.22-devfrom
brettlangdon/wrapt
Feb 21, 2019
Merged

[core] vendor wrapt and six dependencies#755
brettlangdon merged 28 commits into
0.22-devfrom
brettlangdon/wrapt

Conversation

@brettlangdon

@brettlangdon brettlangdon commented Dec 3, 2018 •
edited
Loading

Copy link
Copy Markdown
Member

This PR is a POC moving six and wrapt to be vendored under ddtrace.vendor package.

@brettlangdon

brettlangdon commented Dec 3, 2018

Copy link
Copy Markdown
Member Author

... we import wrapt in a ton of places

@brettlangdon brettlangdon mentioned this pull request Dec 3, 2018
Merged
@palazzem

palazzem commented Dec 4, 2018

Copy link
Copy Markdown

can't we have everything under one package? so ddtrace.vendors or ddtrace.packages or ddtrace.internal? I don't like a lot ddtrace_vendors. Also at some point could make sense to provide a "patching API" that we can use internally to add some extra logic, shared across integrations (i.e. safety guards to bail out if we fail to execute a wrapped function). This API will use wrapt under the hood so that it's just an implementation detail.

Thoughts?

@brettlangdon

brettlangdon commented Dec 4, 2018

Copy link
Copy Markdown
Member Author

The issue we have with ddtrace.vendor (which is what I initially introduced in #751) is that when you from ddtrace.vendor import <package> it also loads ddtrace/__init__.py which breaks a bunch of our tests since a lot of our tests will import from ddtrace.vendor import wrapt

The tests are asserting that 'ddtrace' not in sys.modules, we might be able to find another way this particular issue, which means we can go back to ddtrace.vendor. As well Kyle has been working on a base test class that will run all test cases in a subprocess, which gives us a clean sys.modules and we can better test module loading without polluting other tests (the work is actually really cool #720)

This PR is cool to have here and have open since we can now talk about these edge cases and poke at this, but definitely not a high priority to move wrapt into ddtrace.vendor at this time.

Also re: internal API for wrapping, this is something we are planning on at some point :)

@brettlangdon brettlangdon changed the title [core] vendor wrapt==1.10.11 [core] vendor wrapt and six dependencies Feb 5, 2019
@brettlangdon
brettlangdon changed the base branch from brettlangdon/six to 0.21-dev February 5, 2019 01:41
@brettlangdon

brettlangdon commented Feb 5, 2019

Copy link
Copy Markdown
Member Author

@palazzem and @majorgreys this is good to review.

There are a ton of changes, these are mostly s/import wrapt/from ddtrace.vendor import wrapt/ and a few changes that were needed in some tests.

@brettlangdon brettlangdon added this to the 0.22.0 milestone Feb 5, 2019
@brettlangdon
brettlangdon requested a review from a team as a code owner February 15, 2019 20:40
@brettlangdon
brettlangdon changed the base branch from 0.21-dev to 0.22-dev February 15, 2019 20:40
Kyle-Verhoog
Kyle-Verhoog reviewed Feb 15, 2019
View reviewed changes

@Kyle-Verhoog Kyle-Verhoog left a comment

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.

Just a couple minor concerns with setup.py should be good to go once those are addressed.

Comment thread setup.py
Comment thread setup.py
@brettlangdon
brettlangdon merged commit d96223a into 0.22-dev Feb 21, 2019
@brettlangdon
brettlangdon deleted the brettlangdon/wrapt branch February 21, 2019 23:00
@brettlangdon brettlangdon mentioned this pull request Mar 1, 2019
Merged
@majorgreys majorgreys mentioned this pull request Aug 11, 2023
Merged
16 tasks
majorgreys added a commit that referenced this pull request Sep 13, 2023
@majorgreys @emmettbutler
chore: unvendor psutil and wrapt (#6651)
53ac961 
We had vendored psutil in #1160 due to a lack of wheels for all support
architectures and OSes. This has since been resolved.

At the same time, we can also unvendor wrapt which was added in #755,
which also releases all wheels we need.

We pin the minimum version to the previously vendored version.

## Checklist

- [x] Change(s) are motivated and described in the PR description.
- [x] Testing strategy is described if automated tests are not included
in the PR.
- [x] Risk is outlined (performance impact, potential for breakage,
maintainability, etc).
- [x] Change is maintainable (easy to change, telemetry, documentation).
- [x] [Library release note
guidelines](https://ddtrace.readthedocs.io/en/stable/releasenotes.html)
are followed. If no release note is required, add label
`changelog/no-changelog`.
- [x] Documentation is included (in-code, generated user docs, [public
corp docs](https://github.com/DataDog/documentation/)).
- [x] Backport labels are set (if
[applicable](https://ddtrace.readthedocs.io/en/latest/contributing.html#backporting))

## Reviewer Checklist

- [x] Title is accurate.
- [x] No unnecessary changes are introduced.
- [x] Description motivates each change.
- [x] Avoids breaking
[API](https://ddtrace.readthedocs.io/en/stable/versioning.html#interfaces)
changes unless absolutely necessary.
- [x] Testing strategy adequately addresses listed risk(s).
- [x] Change is maintainable (easy to change, telemetry, documentation).
- [x] Release note makes sense to a user of the library.
- [x] Reviewer has explicitly acknowledged and discussed the performance
implications of this PR as reported in the benchmarks PR comment.
- [x] Backport labels are set in a manner that is consistent with the
[release branch maintenance
policy](https://ddtrace.readthedocs.io/en/latest/contributing.html#backporting)

---------

Co-authored-by: Emmett Butler <[email protected]>
majorgreys added a commit that referenced this pull request Sep 15, 2023
@majorgreys @emmettbutler
chore: unvendor psutil and wrapt (#6651)
5a2c0f9 
We had vendored psutil in #1160 due to a lack of wheels for all support
architectures and OSes. This has since been resolved.

At the same time, we can also unvendor wrapt which was added in #755,
which also releases all wheels we need.

We pin the minimum version to the previously vendored version.

## Checklist

- [x] Change(s) are motivated and described in the PR description.
- [x] Testing strategy is described if automated tests are not included
in the PR.
- [x] Risk is outlined (performance impact, potential for breakage,
maintainability, etc).
- [x] Change is maintainable (easy to change, telemetry, documentation).
- [x] [Library release note
guidelines](https://ddtrace.readthedocs.io/en/stable/releasenotes.html)
are followed. If no release note is required, add label
`changelog/no-changelog`.
- [x] Documentation is included (in-code, generated user docs, [public
corp docs](https://github.com/DataDog/documentation/)).
- [x] Backport labels are set (if
[applicable](https://ddtrace.readthedocs.io/en/latest/contributing.html#backporting))

## Reviewer Checklist

- [x] Title is accurate.
- [x] No unnecessary changes are introduced.
- [x] Description motivates each change.
- [x] Avoids breaking
[API](https://ddtrace.readthedocs.io/en/stable/versioning.html#interfaces)
changes unless absolutely necessary.
- [x] Testing strategy adequately addresses listed risk(s).
- [x] Change is maintainable (easy to change, telemetry, documentation).
- [x] Release note makes sense to a user of the library.
- [x] Reviewer has explicitly acknowledged and discussed the performance
implications of this PR as reported in the benchmarks PR comment.
- [x] Backport labels are set in a manner that is consistent with the
[release branch maintenance
policy](https://ddtrace.readthedocs.io/en/latest/contributing.html#backporting)

---------

Co-authored-by: Emmett Butler <[email protected]>
majorgreys added a commit that referenced this pull request Sep 18, 2023
@majorgreys @emmettbutler
chore: unvendor psutil and wrapt (#6651)
231a799 
We had vendored psutil in #1160 due to a lack of wheels for all support
architectures and OSes. This has since been resolved.

At the same time, we can also unvendor wrapt which was added in #755,
which also releases all wheels we need.

We pin the minimum version to the previously vendored version.

## Checklist

- [x] Change(s) are motivated and described in the PR description.
- [x] Testing strategy is described if automated tests are not included
in the PR.
- [x] Risk is outlined (performance impact, potential for breakage,
maintainability, etc).
- [x] Change is maintainable (easy to change, telemetry, documentation).
- [x] [Library release note
guidelines](https://ddtrace.readthedocs.io/en/stable/releasenotes.html)
are followed. If no release note is required, add label
`changelog/no-changelog`.
- [x] Documentation is included (in-code, generated user docs, [public
corp docs](https://github.com/DataDog/documentation/)).
- [x] Backport labels are set (if
[applicable](https://ddtrace.readthedocs.io/en/latest/contributing.html#backporting))

## Reviewer Checklist

- [x] Title is accurate.
- [x] No unnecessary changes are introduced.
- [x] Description motivates each change.
- [x] Avoids breaking
[API](https://ddtrace.readthedocs.io/en/stable/versioning.html#interfaces)
changes unless absolutely necessary.
- [x] Testing strategy adequately addresses listed risk(s).
- [x] Change is maintainable (easy to change, telemetry, documentation).
- [x] Release note makes sense to a user of the library.
- [x] Reviewer has explicitly acknowledged and discussed the performance
implications of this PR as reported in the benchmarks PR comment.
- [x] Backport labels are set in a manner that is consistent with the
[release branch maintenance
policy](https://ddtrace.readthedocs.io/en/latest/contributing.html#backporting)

---------

Co-authored-by: Emmett Butler <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Kyle-Verhoog Kyle-Verhoog Kyle-Verhoog left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

0.22.0

Development

Successfully merging this pull request may close these issues.

3 participants

@brettlangdon @palazzem @Kyle-Verhoog