opentracing-python-instrumentation icon indicating copy to clipboard operation
opentracing-python-instrumentation copied to clipboard

Published 20 hours ago • verified publisher icon uber-common

Remove Tornado upper bound

Open condorcet opened this issue 3 years ago • 5 comments

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 https://github.com/uber-common/opentracing-python-instrumentation/issues/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 https://github.com/uber-common/opentracing-python-instrumentation/issues/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.

condorcet avatar May 10 '21 20:05 condorcet

Coverage Status

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

coveralls avatar May 10 '21 20:05 coveralls

How could we get someone to review this?

nicholasamorim avatar Jun 15 '21 13:06 nicholasamorim

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

vinayan3 avatar Dec 16 '21 21:12 vinayan3

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

dalepotter avatar Feb 07 '22 17:02 dalepotter

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.

yurishkuro avatar Feb 07 '22 17:02 yurishkuro