Stream Django SQL queries and add flag to toggle their streaming

[hasier]

Currently, only SQLAlchemy reports a sanitized SQL query in the subsegment metadata. Also, it is always attached, with no option to turn it off.

These changes introduce a flag (enabled by default to keep the current behaviour) to allow toggling that feature. There is also new functionality to attach the SQL queries coming from the Django ORM.

A new variable is also read from the Django settings for convenience, which functions the previously mentioned toggle.

[hasier]

Sorry didn't intend to open this PR here, just testing around in own fork

[hasier]

Updated the branch, should be ready for review.

[haotianw465]

Sorry for the late response. If by default the sql should captured, should this be set to True?

[hasier]

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.

[haotianw465]

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?

[hasier]

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?

[haotianw465]

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

[hasier]

I removed the patch for DBAPI2 and just did so for Django. Please let me know if it makes sense now.