Fixing broken instrumentation for sqlalchemy >= 1.4.0 #289

srprash · 2021-04-19T20:31:05Z

Issue #, if available: #281

Description of changes:
For context, please read the following comments:
#281 (comment)
#289 (comment)
#289 (comment)

The changes being done in this PR are:

Fixing the function decorator logic to check if the obj is of type function.
Patching Session.execute method to capture subsegments for 2.0 style execution introduced in sqlalchemy v1.4.0. And some refactoring of the patch code.
Adding a test case for the 2.0 style execution as per the example in sqlalchemy docs. Since this test case would work only on v1.4.0 which is not available for py34 and py35, I added some test environments with conditions in tox.ini for the same.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

codecov-commenter · 2021-04-19T22:00:13Z

Codecov Report

Merging #289 (724b301) into master (1294dee) will decrease coverage by 47.65%.
The diff coverage is 76.19%.

@@             Coverage Diff             @@
##           master     #289       +/-   ##
===========================================
- Coverage   79.63%   31.98%   -47.66%     
===========================================
  Files          83       83               
  Lines        3276     3286       +10     
===========================================
- Hits         2609     1051     -1558     
- Misses        667     2235     +1568

Impacted Files	Coverage Δ
aws_xray_sdk/ext/sqlalchemy/util/decorators.py	`0.00% <0.00%> (-88.89%)`	⬇️
aws_xray_sdk/ext/sqlalchemy_core/patch.py	`73.23% <84.21%> (+0.65%)`	⬆️
aws_xray_sdk/ext/pg8000/patch.py	`0.00% <0.00%> (-100.00%)`	⬇️
aws_xray_sdk/ext/psycopg2/patch.py	`0.00% <0.00%> (-100.00%)`	⬇️
aws_xray_sdk/ext/requests/patch.py	`0.00% <0.00%> (-100.00%)`	⬇️
aws_xray_sdk/ext/pg8000/__init__.py	`0.00% <0.00%> (-100.00%)`	⬇️
aws_xray_sdk/ext/httplib/__init__.py	`0.00% <0.00%> (-100.00%)`	⬇️
aws_xray_sdk/ext/pymysql/__init__.py	`0.00% <0.00%> (-100.00%)`	⬇️
aws_xray_sdk/ext/sqlalchemy/query.py	`0.00% <0.00%> (-100.00%)`	⬇️
aws_xray_sdk/ext/sqlite3/__init__.py	`0.00% <0.00%> (-100.00%)`	⬇️
... and 51 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1294dee...724b301. Read the comment docs.

srprash · 2021-04-20T17:20:17Z

Looks like sqlalchemy 1.4.0 broke two functionalities in the x-ray SDK.

instrumentation of sqlalchemy using ORM which is mentioned in this issue.
instrumentation of sqlalchemy_core using patch where the new ORM type statements are not getting patched by this code.

As mentioned in the Session class API doc:
"Changed in version 1.4: the Session.execute() method is now the primary point of ORM statement execution when using 2.0 style ORM usage."
So this means that any customer using sqlalchemy_core patching for ORM type calls with something like session.add(user) will not create subsegments for sqlalchemy >= 1.4.0 but will work fine for db calls using connection.execute(statement)

srprash · 2021-04-20T18:38:48Z

One thing we can do is since Session.execute() method is now the primary point of ORM statement execution, we can add a patch for it in the patch.py with something like:

wrapt.wrap_function_wrapper(
        'sqlalchemy.orm.session',
        'Session.execute',
        _xray_traced_sqlalchemy_session
    )

This seem to work fine and record a subsegment for session.query() but it doesn't record the subsegment for
session.add() for some reason.

ATM, I can say that for ORM style statement execution, it is safest to use the SQLAlchemy integration rather than patching sqlalchemy_core which was added in this PR.

srprash · 2021-04-28T04:09:28Z

I believe the reason for such a massive drop in the coverage is due to the fact that every test is run with coverage run py.test ... with different set of test cases. For each run of such command, only a single .coverage file is produced and overwriten by the run of next test cases.
Depending on which set of test cases are run as the command in the very last, the .coverage file (and thus the generated coverage.xml) will contain the coverage report about that run. In this case, I believe the last command to overwrite the .coverage was coverage run --source aws_xray_sdk -m py.test tests/ext/sqlalchemy_core/ --ignore tests/ext/sqlalchemy_core/test_sqlalchemy_core_2.py and so other modules' coverage dropped to 0% as per this report.

This makes the coverage report quite unreliable, and the solution is to generate different coverage report for each test run and aggregate them together in a codecov report.

anuraaga

@srprash Is the coverage issue introduced by this PR or it already was there? The tox.ini change doesn't seem to be adding any new patterns

aws_xray_sdk/ext/sqlalchemy_core/patch.py

srprash · 2021-04-28T05:24:29Z

Is the coverage issue introduced by this PR or it already was there? The tox.ini change doesn't seem to be adding any new patterns

@anuraaga My suspicion is that the issue was always there and was sometimes encountered in PRs like this which saw decreased in coverage despite adding more tests.
I experimented by rearranging the command order and saw the coverage change from 79.42% to 31.98% between this and this commit.

anuraaga

Cool in that case I don't think coverage issue blocks this PR, will be good to fix it separately at some point.

* Drop version pinning sqlalchemy and flask_sqlalchemy * Check obj type instead of callable for functions * testing * Adding patch for Session.execute. Removing session.add test * Remove unused variable * Adding 2.0 style test case. Some refactor. Unwrap session. * Splitting out tests * Typo fix * Refactored some tests files * minor renaming of function

Drop version pinning sqlalchemy and flask_sqlalchemy

439f371

srprash marked this pull request as ready for review April 19, 2021 20:59

Check obj type instead of callable for functions

4aba946

testing

d89eb37

srprash marked this pull request as draft April 19, 2021 22:36

Merge branch 'master' into sqlalchemy_140_fix

74b6d8f

srprash added 2 commits April 20, 2021 14:42

Adding patch for Session.execute. Removing session.add test

bf1149d

Remove unused variable

2f216be

srprash changed the title ~~Sqlalchemy instrumentation - Fixing condition for patching all functions~~ Fixing broken instrumentation for sqlalchemy >= 1.4.0 Apr 20, 2021

srprash marked this pull request as ready for review April 20, 2021 22:15

srprash and others added 5 commits April 21, 2021 08:02

Adding 2.0 style test case. Some refactor. Unwrap session.

650ac53

Splitting out tests

8d2956a

Typo fix

e1d5075

Refactored some tests files

173ccdc

Merge branch 'master' into sqlalchemy_140_fix

33da1af

srprash requested a review from anuraaga April 26, 2021 20:35

Merge branch 'master' into sqlalchemy_140_fix

335b778

anuraaga reviewed Apr 28, 2021

View reviewed changes

aws_xray_sdk/ext/sqlalchemy_core/patch.py Outdated Show resolved Hide resolved

minor renaming of function

724b301

anuraaga approved these changes Apr 28, 2021

View reviewed changes

antdking mentioned this pull request Apr 28, 2021

feat(sqla-core): Add support for rendering Database Specific queries #291

Merged

Merge branch 'master' into sqlalchemy_140_fix

2d4a1ae

srprash mentioned this pull request Apr 28, 2021

Coverage depending on the order of tests run #296

Closed

srprash merged commit d807884 into aws:master Apr 28, 2021

srprash deleted the sqlalchemy_140_fix branch April 28, 2021 15:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixing broken instrumentation for sqlalchemy >= 1.4.0 #289

Fixing broken instrumentation for sqlalchemy >= 1.4.0 #289

srprash commented Apr 19, 2021 •

edited

Loading

codecov-commenter commented Apr 19, 2021 •

edited

Loading

srprash commented Apr 20, 2021 •

edited

Loading

srprash commented Apr 20, 2021 •

edited

Loading

srprash commented Apr 28, 2021 •

edited

Loading

anuraaga left a comment

srprash commented Apr 28, 2021 •

edited

Loading

anuraaga left a comment

Fixing broken instrumentation for sqlalchemy >= 1.4.0 #289

Fixing broken instrumentation for sqlalchemy >= 1.4.0 #289

Conversation

srprash commented Apr 19, 2021 • edited Loading

codecov-commenter commented Apr 19, 2021 • edited Loading

Codecov Report

srprash commented Apr 20, 2021 • edited Loading

srprash commented Apr 20, 2021 • edited Loading

srprash commented Apr 28, 2021 • edited Loading

anuraaga left a comment

Choose a reason for hiding this comment

srprash commented Apr 28, 2021 • edited Loading

anuraaga left a comment

Choose a reason for hiding this comment

srprash commented Apr 19, 2021 •

edited

Loading

codecov-commenter commented Apr 19, 2021 •

edited

Loading

srprash commented Apr 20, 2021 •

edited

Loading

srprash commented Apr 20, 2021 •

edited

Loading

srprash commented Apr 28, 2021 •

edited

Loading

srprash commented Apr 28, 2021 •

edited

Loading