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

Use the official middleware pattern for Aiohttp ext #29

Merged
merged 3 commits into from
Mar 6, 2018

Conversation

pfreixes
Copy link
Contributor

@pfreixes pfreixes commented Mar 1, 2018

The previous implementation was using the former version of Aiohttp that
got deprecated since 2.3.0. Related to #28.

The interface didn't change, so from the point of the view of the user, it does not suppose a breaking change. I would suggest using a minor release.

Previous implementation was using the former version of Aiohttp that
got deprecated since 2.3.0.
@haotianw465
Copy link
Contributor

I have one question. Based on https://aiohttp.readthedocs.io/en/stable/web_advanced.html#aiohttp-web-middlewares: "Prior to v2.3 middleware required an outer middleware factory which returned the middleware coroutine."

Does it mean this PR drops support for aiohttp pre-2.3 versions? What happens if one consumer is using 2.2 and upgrade the X-Ray SDK to 0.97 with this change?

@pfreixes
Copy link
Contributor Author

pfreixes commented Mar 2, 2018

What is expected is people having installed at leasr the last version of 2 major release, they wont have breaking changes and will be "free" of known bugs.

If somebody is still in an older release the update to the last 2.X should be friction less.

In any case the 3.X version is getting mature, 3.0.5 is out and sooner than later a 3.1 would be released. What IMHO might be the good candidate for production sysy
tems.

@haotianw465
Copy link
Contributor

I totally understand what you are saying. The issue is not about whether they upgrade to 2.3 or 3.x, it is about when. We cannot assume upgrading aiohttp is at top priorities for all of our customers who depend on aiohttp. And a minor release with this also breaks semver convention and the expectations on minor upgrade.

We definitely want the aiohttp support to use the best practices. I purpose to copy the current middleware to another class and make necessary changes, and then note the old one is deprecated. In the user guide and README we can advertising only the new class.

This way whoever new to X-Ray and have aiohttp 2.3+ can always get the latest and greatest, and existing X-Ray users running older aiohttp can upgrade X-Ray SDK freely.

Please let me know your thoughts.

@pfreixes
Copy link
Contributor Author

pfreixes commented Mar 5, 2018

Looking into the middleware code looks like that there is a dependency - the remote attrribute [1] - that was introduced in the 2.3.0 series. So, having this dependency I guess that the backward compatibility does not make sense.

Thoughts ?

[1] aio-libs/aiohttp@44edff8

@haotianw465
Copy link
Contributor

You are right. It looks like the first PR for aiohttp has implicit requirement on 2.3+.

Could you add new section "unreleased" on CHANGELOG and put this change there? This PR is the first change after 0.96. Then I will go a head and merge it in.

Also it would be helpful if you can add a ">2.3" at https://github.com/aws/aws-xray-sdk-python/blob/master/docs/index.rst "aiohttp item"

@pfreixes
Copy link
Contributor Author

pfreixes commented Mar 6, 2018

Done. In any case Im working also with the Client part that is only compatible with Aiohttp 3.X series, but lets discuss that in the next MR.

@haotianw465
Copy link
Contributor

Yes the client part for 3.x is totally fine.

@haotianw465 haotianw465 merged commit eb228b9 into aws:master Mar 6, 2018
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.

2 participants