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

Remove Tornado upper bound #115

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

condorcet
Copy link
Contributor

Tornado 6 doesn't have stack context mechanism and became asyncio framework. But TornadoScopeManager and helper span_in_stack_context using stack context explicitly and limiting version of Tornado that can be used. That's why we have upper bound for Tornado.
In this PR TornadoScopeManager and helpers has been detached and moved to separate module. This changes doesn't breaks backward compatibility and we can use helpers with old Tornado.
Using helpers with newest version of Tornado has no sense and will raise runtime error.

Firstly I tried to remove Tornado from dependencies (see #113) but faced at least with two problems:

  1. Boto3 instrumentation relies on stack context and, as a result, on TornadoScopeManager. I don't have mush experience with this code, but I think it's possible to untie boto3 from this helpers. In general it's weird, that insturmentation of boto3 forces us to use old Tornado (by the way related issue Should span_in_stack_context be throwing exceptions? #111)
  2. urllib2 instrumentation uses one of tornado helpers for normalizing headers.

So I decided to keep Tornado in dependencies in this PR, but if you interesting, we can try to fix mentioned problems.

Also I add some tests for asyncio. Pathcher for Tornado HTTP client works fine with new versions.

Signed-off-by: Vasilii Novikov <[email protected]>
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.2%) to 87.58% when pulling 0fbcb02 on condorcet:remove-tornado-upper-bound into ddae1c5 on uber-common:master.

@nicholasamorim
Copy link
Contributor

How could we get someone to review this?

@vinayan3
Copy link

Is anyone able to review this and move toward getting this committed? Having tornado pinned at 5.x.x is a blocker.

@dalepotter
Copy link

Bumping the request for review since this one has been around for little while - would this be one for @yurishkuro ?

@yurishkuro
Copy link
Contributor

It appears Uber removed my admin permissions to this repo after I left the company.

The OpenTracing APIs & project have been deprecated in favor of OpenTelemetry, and by extension all instrumentation libraries including this one. I recommend not to expect any new releases. I will try to find someone at Uber who can archive this repo.

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