-
Notifications
You must be signed in to change notification settings - Fork 145
Stream Django SQL queries and add flag to toggle their streaming #111
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from 3 commits
Commits
Show all changes
7 commits
Select commit
Hold shift + click to select a range
0ba9210
Add params and functions to allow recording and streaming SQL queries
hasier ea9644f
Set general default to True and adapt SQLAlchemy to follow the flag
hasier 1d1783d
Update docs
hasier c1b6e05
Merge branch 'master' into stream-sql-query
haotianw465 1bf9472
Set stream_sql to True by default
hasier 202cc60
Unpatch dbapi2, patch use custom cursor for Django and chunked_cursor
hasier 06c61ac
Merge branch 'master' into stream-sql-query
haotianw465 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the late response. If by default the sql should captured, should this be set to
True
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No worries, thanks for your comment. I meant it is enabled by default as this is the current behaviour for SQLAlchemy, so I wanted to keep it. But for the rest (in this case Django), I have set it off by default, as it is the behaviour previous to this PR. Hope this makes sense.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi, I would not consider an enhancement to be concerned. We have some other enhancements that added more data and haven't seen any issue.
The SQLAlchemy query capture was also submitted as a PR. In fact the SDK means to have query capture as the default behavior. It's just due to security concerns it has different development and review cycle. I would suggest to keep the query capture behavior consistent, which that behavior is to have parameterized query ready whenever possible. Thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Regarding the default behaviour, I agree, I'll change it so that capture is enabled by default.
Regarding the parametrised queries, afaik there is no way to ensure that the incoming query is a parametrised one... DBAPI2 (PEP 249) recommends this behaviour, but then it is up to the implementation to decide how the queries and parameters are formatted.
Is this the concern you have? Would it be better to attempt the patch in a different place? The only one I can think of is to attempt to patch the Django ORM (I haven't tried, so I don't know how feasible it is). I mention the Django ORM as, for example, psycopg2 also allows different Cursor classes to be used with its connections, which might lead to the same situation again.
But it is also true that it is this XRay SDK the one that controls which drivers are patched, as the patch needs to be included in a module here. As far as I can see, those that implement DBAPI2 and are included here for now are just Django ORM, SQLAlchemy and SQLite, so I guess it should not be that big a concern?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. That's another thing I'm going to mention besides the default config value. For SQLAlchemy the patcher actually works on the implementation level and there are tests against parameterized queries. https://github.com/aws/aws-xray-sdk-python/blob/master/tests/ext/sqlalchemy/test_query.py.
DBAPI level query capture probably will not always work but if it does work for the current patchers (Django or SQlite) I'm OK to move forward with an internal security review. If not I would suggest to have the toggle function just for SQLAlchemy and everything else in the separate PRs for each actual patcher.
You can see the PR for SQLAlchemy query capture PR here: #34
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I removed the patch for DBAPI2 and just did so for Django. Please let me know if it makes sense now.