Added instrumentation for Tornado 6 and above #1018
Conversation
owais
force-pushed
the
tornado
branch
11 times, most recently
from
598eb6d to
4d36cfc
Compare
owais
force-pushed
the
tornado
branch
4 times, most recently
from
b6d2754 to
31a616d
Compare
|
|
||
| ## Unreleased | ||
|
|
||
| - Initial release. Supports Tornado 4.x and 5.x on Python 3.7 and newer. |
There was a problem hiding this comment.
This says tornado 4/5 but the PR title calls for tornado 6+
There was a problem hiding this comment.
Yea, it's 6+. Need to fix that.
...n/opentelemetry-instrumentation-tornado/src/opentelemetry/instrumentation/tornado/version.py
Outdated
Show resolved
Hide resolved
owais
force-pushed
the
tornado
branch
4 times, most recently
from
66708d7 to
82ba264
Compare
instrumentation/opentelemetry-instrumentation-tornado/README.rst
Outdated
Show resolved
Hide resolved
| Programming Language :: Python :: 3.8 | ||
|
|
||
| [options] | ||
| python_requires = >=3.5 |
There was a problem hiding this comment.
Yes, it works for Tornado 6+ for my demo app which uses all possible types of handlers and also passes all the tests. If there are concerns with contextvars based context not being available on 3.5 or other reasons, we can support >=3.6 or even 3.7 officially. That said, so far I've found out that everything works on 3.5 without any errors/warnings and the traces generated are correct as well.
| import tornado.web | ||
| from opentelemetry.instrumentation.tornado import TornadoInstrumentor | ||
|
|
||
| # apply tornado auto-instrumentation |
There was a problem hiding this comment.
I don't think we should say "auto-instrumentation" since we usually use this to refer to not having to instrument manually.
owais
force-pushed
the
tornado
branch
4 times, most recently
from
8daa5e6 to
b25396a
Compare
codeboten
added
the
instrumentation
|
@open-telemetry/python-approvers would appreciate if someone could help review this. Thanks. |
opentelemetry-instrumentation/src/opentelemetry/instrumentation/bootstrap.py
Outdated
Show resolved
Hide resolved
instrumentation/opentelemetry-instrumentation-tornado/README.rst
Outdated
Show resolved
Hide resolved
owais
force-pushed
the
tornado
branch
5 times, most recently
from
da2b60f to
60b3026
Compare
This commit adds auto-instrumentation for Tornado 6 and above on Python versions 3.5 and above.
|
Hi, Great work on the tornado instrumentation. May I ask, why is it specifically for Tornado 6+? What is it doing/using that isn't available in earlier versions or tornado? Thanks |
|
@IainAdamsLabs I think there are some subtle issues with context management and style of tornado handlers (async/await vs gen.coroutine etc). This was months ago so I don't remember the exact issues right now but I think at least one of them was related to this: tornadoweb/tornado#2716 Also, I think (but I'm not sure) if Tornado <6 was to be supported, we'd probably need a special context manager that is not based on contextvars but again I could be wrong. Tornado 6 was released on Mar 1, 2019 so it isn't brand new TBH. OpenTracing does support Tornado 4 and 5 so I thought it wasn't worth adding support for Tornado 5 here as people can still use OpenTracing to instrument their tornado app until they migrate to Tornado 6. It might also be possible to use OpenTracing with Tornado 5 suppot with the help of the OpenTracing shim we have in this project but I've never tried using it. That said, I don't think anyone would object if someone was to add and maintain code for Tornado 5 to this package. |
This commit adds auto-instrumentation for Tornado 6 and above on Python versions 3.5 and above.
Description
This commit adds auto-instrumentation for Tornado 6 and above on Python
versions 3.5 and above.
Fixes #627
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration
Checklist: