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

feat(aiohttp): add client integration #3362

Merged
merged 2 commits into from
Mar 9, 2022

Conversation

Kyle-Verhoog
Copy link
Member

@Kyle-Verhoog Kyle-Verhoog commented Mar 3, 2022

Integration

This PR adds support for aiohttp client.

Work was long ago started on this in (#294 (thank you @thehesiod!!!). There were a number of issues in the library that led to not being able to merge in the work such as async context management, service naming, integration configuration and more which have all since been addressed.

#294 and later #1372 included additional support for ClientResponse and StreamReader which are omitted here with the intention of introducing them as follow ups.

Links

Checklist

  • Usage and configuration documentation added in __init__.py, docs/index.rst and docs/integrations.rst.
  • Corp docs PR to add new integration to documentation.
  • Span metadata
  • Global configuration
    • ddtrace.config entry is specified.
    • Environment variables are provided for config options.
  • Instance configuration
    • Pin overriding.
    • Service name override (if applicable).
  • Async
    • Span parenting behaves as expected.
    • Context propagation across async contexts.
  • HTTP
  • Tests
    • Use pytest fixtures found in tests/conftest.py or the test helpers on TracerTestCase if
      writing unittest style test cases.
    • Tests are provided for all the above.
    • Tests are added to CI (.circleci/config.yml).
    • Functionality is maintained from original library.
    • Patch test cases are added (see test_django_patch.py for an example).
    • All Python versions that the library supports are tested.
    • All significant library versions (including the latest) are tested. This typically includes every minor release going back a few years.

@Kyle-Verhoog Kyle-Verhoog changed the title Aiohttp client feat(aiohttp): add client integration Mar 3, 2022
@mergify
Copy link
Contributor

mergify bot commented Mar 4, 2022

@Kyle-Verhoog this pull request is now in conflict 😩

ddtrace/contrib/aiohttp/__init__.py Outdated Show resolved Hide resolved
ddtrace/contrib/aiohttp/patch.py Show resolved Hide resolved
ddtrace/contrib/aiohttp/patch.py Outdated Show resolved Hide resolved
releasenotes/notes/aiohttp-98ae9ce70dda1dbc.yaml Outdated Show resolved Hide resolved
ddtrace/contrib/aiohttp/patch.py Outdated Show resolved Hide resolved
ddtrace/contrib/aiohttp/patch.py Show resolved Hide resolved
tests/contrib/aiohttp/test_aiohttp_client.py Outdated Show resolved Hide resolved
@mergify mergify bot removed the conflict label Mar 4, 2022
@Kyle-Verhoog Kyle-Verhoog force-pushed the aiohttp_client branch 2 times, most recently from c9f3a6e to 38dff42 Compare March 4, 2022 18:05
@Kyle-Verhoog Kyle-Verhoog added this to the 0.60.0 milestone Mar 4, 2022
ddtrace/contrib/aiohttp/patch.py Outdated Show resolved Hide resolved
ddtrace/contrib/aiohttp/patch.py Outdated Show resolved Hide resolved
ddtrace/contrib/aiohttp/patch.py Outdated Show resolved Hide resolved
ddtrace/contrib/aiohttp/patch.py Outdated Show resolved Hide resolved
ddtrace/contrib/aiohttp/patch.py Show resolved Hide resolved
releasenotes/notes/aiohttp-98ae9ce70dda1dbc.yaml Outdated Show resolved Hide resolved
@Kyle-Verhoog Kyle-Verhoog force-pushed the aiohttp_client branch 4 times, most recently from dab7b75 to 38ab5c9 Compare March 8, 2022 16:09
Copy link
Member

@brettlangdon brettlangdon left a comment

Choose a reason for hiding this comment

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

some minor nits, otherwise looking good

ddtrace/contrib/aiohttp/patch.py Outdated Show resolved Hide resolved
tests/contrib/aiohttp/test_aiohttp_client.py Outdated Show resolved Hide resolved
tests/contrib/aiohttp/test_aiohttp_client.py Outdated Show resolved Hide resolved
brettlangdon
brettlangdon previously approved these changes Mar 8, 2022
Copy link
Member

@brettlangdon brettlangdon left a comment

Choose a reason for hiding this comment

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

lgtm

Since the integration was orignially written a bunch of things have
changed including integration conventions, async support, 1.0
deprecations/removals/moves and more.

The StreamReader patching is omitted for now and can be introduced
later.
Copy link
Contributor

@mabdinur mabdinur left a comment

Choose a reason for hiding this comment

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

3rd ✅

@codecov-commenter
Copy link

Codecov Report

Merging #3362 (192b61e) into 1.x (9dab78a) will increase coverage by 0.06%.
The diff coverage is 95.72%.

Impacted file tree graph

@@            Coverage Diff             @@
##              1.x    #3362      +/-   ##
==========================================
+ Coverage   79.46%   79.53%   +0.06%     
==========================================
  Files         615      617       +2     
  Lines       46427    46609     +182     
==========================================
+ Hits        36895    37069     +174     
- Misses       9532     9540       +8     
Impacted Files Coverage Δ
ddtrace/contrib/aiohttp/__init__.py 100.00% <ø> (ø)
ddtrace/contrib/trace_utils_async.py 80.00% <80.00%> (ø)
ddtrace/contrib/aiohttp/patch.py 93.67% <92.75%> (-6.33%) ⬇️
tests/contrib/aiohttp/conftest.py 91.30% <100.00%> (ø)
tests/contrib/aiohttp/test_aiohttp_client.py 100.00% <100.00%> (ø)
tests/contrib/config.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9dab78a...192b61e. Read the comment docs.

@brettlangdon brettlangdon modified the milestones: v0.60.0, v1.0.0 Mar 9, 2022
@Kyle-Verhoog
Copy link
Member Author

@Mergifyio backport 0.x 1.0

@Kyle-Verhoog Kyle-Verhoog modified the milestones: v1.0.0, v1.1.0 Mar 9, 2022
@Kyle-Verhoog Kyle-Verhoog merged commit 2ac1fc9 into DataDog:1.x Mar 9, 2022
@Kyle-Verhoog Kyle-Verhoog deleted the aiohttp_client branch March 9, 2022 21:42
@mergify
Copy link
Contributor

mergify bot commented Mar 9, 2022

backport 0.x 1.0

✅ Backports have been created

Kyle-Verhoog added a commit that referenced this pull request Mar 9, 2022
This PR adds support for aiohttp client.

Work was long ago started on this in (#294 (thank you @thehesiod!!!). There were a number of issues in the library that led to not being able to merge in the work such as async context management, service naming, integration configuration and more which have all since been addressed.

#294 and later #1372 included additional support for ClientResponse and StreamReader which are omitted here with the intention of introducing them as follow ups.

Co-authored-by: Alexander Mohr <[email protected]>
brettlangdon pushed a commit that referenced this pull request Mar 9, 2022
This PR adds support for aiohttp client.

Work was long ago started on this in (#294 (thank you @thehesiod!!!). There were a number of issues in the library that led to not being able to merge in the work such as async context management, service naming, integration configuration and more which have all since been addressed.

Co-authored-by: Alexander Mohr <[email protected]>
(cherry picked from commit 2ac1fc9)
brettlangdon pushed a commit that referenced this pull request Mar 9, 2022
This PR adds support for aiohttp client.

Work was long ago started on this in (#294 (thank you @thehesiod!!!). There were a number of issues in the library that led to not being able to merge in the work such as async context management, service naming, integration configuration and more which have all since been addressed.

#294 and later #1372 included additional support for ClientResponse and StreamReader which are omitted here with the intention of introducing them as follow ups.

Co-authored-by: Alexander Mohr <[email protected]>

Co-authored-by: Kyle Verhoog <[email protected]>
Co-authored-by: Alexander Mohr <[email protected]>
brettlangdon added a commit that referenced this pull request Mar 9, 2022
* feat(aiohttp): add client integration (#3362)

This PR adds support for aiohttp client.

Work was long ago started on this in (#294 (thank you @thehesiod!!!). There were a number of issues in the library that led to not being able to merge in the work such as async context management, service naming, integration configuration and more which have all since been addressed.

Co-authored-by: Alexander Mohr <[email protected]>
(cherry picked from commit 2ac1fc9)

* Update ddtrace/contrib/aiohttp/__init__.py

Co-authored-by: Kyle Verhoog <[email protected]>
Co-authored-by: Brett Langdon <[email protected]>
@thehesiod
Copy link
Contributor

@Kyle-Verhoog are there links for follow-up PR or issues?

@Kyle-Verhoog
Copy link
Member Author

hey @thehesiod! We have not followed up yet for the ClientResponse or StreamReader. Issues for these might help us gauge the need for these features so feel free to open an issue to track those 🙂

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.

6 participants