Add support for tornado.web #661

basepi · 2019-12-06T20:56:08Z

Figured I'd open a draft even though this is barely more than a skeleton so far.

What does this pull request do?

Adds Tornado support (or it will, anyway, once it's done)

Related issues

Refs #598

beniwohli

Great stuff so far!

elasticapm/contrib/tornado/__init__.py

elasticapm/contrib/tornado/utils.py

elasticapm/instrumentation/packages/tornado.py

beniwohli

I like the creates_transactions flag! Good idea. And great job on adding sorely needed comments to AbstractIntrumentedModule!

just a couple more comments :) I'll stop reviewing now until you think it's ready for review

elasticapm/instrumentation/packages/base.py

elasticapm/instrumentation/packages/tornado.py

Not sure how I managed this one. Been toying with VSCode and obviously did something wrong

Based on the spec at https://github.com/elastic/apm-server/blob/master/docs/spec/request.json

Since vscode invokes flake8 with a full path on save, the excludes weren't being observed. The /** format seems to fix the issue

Makes it easier to see my own notes for this PR independent of existing TODOs in the project

Also fix a couple of small typos

Phew

basepi · 2020-01-16T22:43:59Z

This is code complete. I need to work out our support matrix, fix up the test suite (for async errors in py2 mostly), and write up some documentation.

The primary instrumentation point, tornado.web.RequestHandler._execute, is `async def` in 6.0, and `def` with `@gen.coroutine` in 5.0. I'm not sure how to decorate both at the moment, so until a community member asks we can just support 6.0+

basepi · 2020-01-18T02:30:52Z

This is ready for review! We only support tornado 6.0+ (see reasons here.

@beniwohli I'm not sure what's going wrong with the tests -- I added all the exclusions to the jenkins-exclude file, but I'm still getting a bunch of syntax errors in non-3.7 tests due to async def. What am I missing?

beniwohli · 2020-01-18T18:42:22Z

@basepi the SyntaxErrors are due to pytest's test discovery. Even when excluded via Jenkins excludes, pytest will still cause Python to parse the code. You have to tell pytest to ignore the the code. We currently do this for all files that are prefixed with py3_, as well for all files that are in an asyncio folder, including subfolders.

apm-agent-python/Makefile

Lines 11 to 13 in 721601e

    
           if [[ "$$PYTHON_VERSION" =~ ^(3.5|3.6|3.7|3.8|nightly|pypy3)$$ ]] ; then \ 
        
           py.test -v $(PYTEST_ARGS) $(PYTEST_MARKER) $(PYTEST_JUNIT); \ 
        
           else py.test -v $(PYTEST_ARGS) $(PYTEST_MARKER) $(PYTEST_JUNIT) --ignore-glob='*/py3_*.py' --ignore-glob='*/asyncio/*'; fi

If you move the Tornado tests into tests/contrib/asyncio, it should work.

I'll do a proper code review on Monday :)

beniwohli

Awesome work!

.flake8

basepi · 2020-01-21T17:12:10Z

Pretty sure the remaining test failures are the same ones as are failing on master, but I'm going to wait until we get them fixed because I like green. :)

beniwohli · 2020-01-21T17:27:32Z

@basepi I think most failures are due to an import error: https://apm-ci.elastic.co/blue/organizations/jenkins/apm-agent-python%2Fapm-agent-python-mbp/detail/PR-661/19/pipeline/158#step-1296-log-1482

Looks like you might need to move the import of elasticapm.contrib.tornado.utils into the call methods. Not pretty, but I don't see a better solution.

basepi · 2020-01-21T19:53:20Z

Good catch. I saw those client errors and stopped reading before I got to the "good ones."

Fixed now, I think.

bmorelli25

Docs look good. Sorry for the delay!

docs/supported-technologies.asciidoc

docs/tornado.asciidoc

Co-Authored-By: Brandon Morelli <[email protected]>

docs/supported-technologies.asciidoc

Co-Authored-By: Brandon Morelli <[email protected]>

beniwohli · 2020-01-23T21:25:45Z

Awesome work getting this done @basepi!

lreuven mentioned this pull request Dec 6, 2019

Add support to asyncio — Asynchronous I/O #598

Closed

6 tasks

beniwohli reviewed Dec 11, 2019

View reviewed changes

beniwohli reviewed Dec 13, 2019

View reviewed changes

elasticapm/instrumentation/packages/base.py Outdated Show resolved Hide resolved

elasticapm/instrumentation/packages/tornado.py Outdated Show resolved Hide resolved

basepi added 18 commits January 14, 2020 14:56

Add vscode files and pytest_cache to gitignore

e3c00c2

Add tornado integration

6e4e884

Add skeleton for tornado instrumentation

a7ab699

Fix path

cfe5bda

Not sure how I managed this one. Been toying with VSCode and obviously did something wrong

First pass at instrumenting RequestHandler._execute

ca4c821

Finish up request context info

f5ab53b

Based on the spec at https://github.com/elastic/apm-server/blob/master/docs/spec/request.json

Implement review suggestions

0202fec

Add documentation and create_transactions override for instrumentation

6c5d688

Fix a typo

1400737

Fix flake8 excludes for vscode

06ecbbb

Since vscode invokes flake8 with a full path on save, the excludes weren't being observed. The /** format seems to fix the issue

Use FIXME for now

b31d615

Makes it easier to see my own notes for this PR independent of existing TODOs in the project

Add instrumentation to the register

50f2cf2

Also fix a couple of small typos

Fix (some very hard to track down) copy pasta

d6cb2bf

Add first passing test

18d81db

Phew

Fix transaction name and add more docs about sampling

1f86df5

Fix test for new transaction name

3e28231

Add instrumentation for tornado exception handling

c25da7e

Add tests for exception and TraceParent

b4f02be

basepi force-pushed the tornado branch from 2a96bf0 to b4f02be Compare January 15, 2020 00:38

Merge remote-tracking branch 'upstream/master' into tornado

380dd65

elastic deleted a comment from houndci-bot Jan 16, 2020

basepi added 4 commits January 16, 2020 12:53

Instrument template rendering

9feb2c5

Add render test for tornado

35b345c

Finish implementing request/response capture

fef8f8b

Merge remote-tracking branch 'upstream/master' into tornado

4409762

elastic deleted a comment from houndci-bot Jan 16, 2020

basepi added 3 commits January 17, 2020 17:44

Add documentation, fix up jenkins configs

4026292

Remove tornado 5.0 support

bfe3546

The primary instrumentation point, tornado.web.RequestHandler._execute, is `async def` in 6.0, and `def` with `@gen.coroutine` in 5.0. I'm not sure how to decorate both at the moment, so until a community member asks we can just support 6.0+

Fix requirements-tornado-newest.txt to be 6.0+

a20b38c

basepi marked this pull request as ready for review January 18, 2020 02:27

basepi requested review from beniwohli and bmorelli25 January 18, 2020 02:28

basepi mentioned this pull request Jan 18, 2020

Tornado framework #323

Closed

beniwohli approved these changes Jan 20, 2020

View reviewed changes

.flake8 Show resolved Hide resolved

Move tornado tests under asyncio for proper pytest discovery

d2a9218

basepi added 2 commits January 21, 2020 12:46

Rename tornado tests

c94cbb7

Fix tests with late import

87fd851

basepi added 2 commits January 21, 2020 12:55

Add setup.py changes I missed

a6eaed1

Add tornado to known_third_party

636beaf

bmorelli25 reviewed Jan 22, 2020

View reviewed changes

docs/supported-technologies.asciidoc Show resolved Hide resolved

docs/tornado.asciidoc Outdated Show resolved Hide resolved

Apply suggestions from code review

2866c64

Co-Authored-By: Brandon Morelli <[email protected]>

bmorelli25 reviewed Jan 23, 2020

View reviewed changes

docs/supported-technologies.asciidoc Show resolved Hide resolved

basepi and others added 2 commits January 22, 2020 18:54

Add blank line after final bullet

612a7e8

Co-Authored-By: Brandon Morelli <[email protected]>

Merge remote-tracking branch 'upstream/master' into tornado

58398bf

basepi merged commit 307f34d into elastic:master Jan 23, 2020

basepi mentioned this pull request Feb 4, 2020

WIP: feat: add sanic support for elastic-apm python agent #522

Closed

3 tasks

Add support for tornado.web #661

Add support for tornado.web #661

Uh oh!

Conversation

basepi commented Dec 6, 2019

What does this pull request do?

Related issues

Uh oh!

beniwohli left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

beniwohli left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

basepi commented Jan 16, 2020

Uh oh!

basepi commented Jan 18, 2020

Uh oh!

beniwohli commented Jan 18, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

beniwohli left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

basepi commented Jan 21, 2020

Uh oh!

beniwohli commented Jan 21, 2020

Uh oh!

basepi commented Jan 21, 2020

Uh oh!

bmorelli25 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

beniwohli commented Jan 23, 2020

Uh oh!

Uh oh!

beniwohli commented Jan 18, 2020 •

edited

Loading