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

Update outdated aiohttp middleware #651

Closed
halfdanrump opened this issue Oct 16, 2018 · 7 comments
Closed

Update outdated aiohttp middleware #651

halfdanrump opened this issue Oct 16, 2018 · 7 comments

Comments

@halfdanrump
Copy link

This:

from ddtrace.contrib.aiohttp import trace_app
from ddtrace import tracer, patch
patch(aiohttp=True)  # patch third-party modules like aiohttp_jinja2

causes this

DeprecationWarning: old-style middleware "<function trace_middleware at 0x1117ac2f0>" deprecated, see #2252
  self._middlewares_handlers = tuple(self._prepare_middleware())

when running pytest. I confirmed that the code is indeed using outdated middleware. See aiohttp docs for new-style middleware.

@labbati
Copy link
Member

labbati commented Oct 16, 2018

Hi @halfdanrump, thanks for taking some time to look into this issue. We will look into it as soon as possible. No workarounds are currently needed, as I can tell, as this is just a deprecation and it still work (based on aio-libs/aiohttp#2252). Something we want to address, though! In the meantime, any PR is well welcomed 😄

@labbati
Copy link
Member

labbati commented Oct 16, 2018

Also, this PR will resolve this issue: https://github.com/DataDog/dd-trace-py/pull/294/files#diff-4486ecdada144080e284937d7892bd76L16
Currently still work in progress, so I am not closing this issue for now.

@halfdanrump
Copy link
Author

Hi @labbati ,

Thanks for getting back to me! Cool, I don't mind to wait for the PR. As you say it's just a deprecation warning, so it's not a big deal. Mostly makes our test suite yellow instead of nice and green 😆 Will the PR work for aiohttp 3.x though? The PR code seems to have an extra @asyncio.coroutine decorator. At least that's not required in 3.x.

@labbati
Copy link
Member

labbati commented Oct 18, 2018

hey @halfdanrump:

makes our test suite yellow instead of nice and green

I know that feeling, it hurts 😄 but will come soon

Will the PR work for aiohttp 3.x though

Support for 3.x is in our radar, but it probably goes beyond the PR i linked. BTW, while we do not officially test such version, did you try to use it? Any specific error?

The PR code seems to have an extra @asyncio.coroutine decorator. At least that's not required in 3.x.

Actually that could be removed, I guess, as a part of 3.x support, as it is not required and is deprecated

@stj
Copy link

stj commented Jan 29, 2019

Created PR #805, with a different approach (it duplicates more code). Happy to address any feedback.

Next release of aiohttp will drop the old style middleware, would like to get dd-trace-py into a state that will allow us to update and test against aiohttp master.

@majorgreys
Copy link
Contributor

#294 has begun work on this. We will update this issue once it is merged.

@Kyle-Verhoog
Copy link
Member

Done! #3362

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants