Skip to content

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

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