Replace jsonpickle with json to serialize entity #275
Conversation
Codecov Report
@@ Coverage Diff @@
## master #275 +/- ##
==========================================
+ Coverage 79.34% 79.52% +0.17%
==========================================
Files 82 83 +1
Lines 3249 3277 +28
==========================================
+ Hits 2578 2606 +28
Misses 671 671
Continue to review full report at Codecov.
|
| @@ -18,8 +18,8 @@ deps = | |||
| requests | |||
| bottle >= 0.10 | |||
| flask >= 0.10 | |||
| sqlalchemy | |||
| Flask-SQLAlchemy | |||
| sqlalchemy==1.3.* | |||
There was a problem hiding this comment.
I think we should only pin for py 3.4 and 3.5 and should still test with the latest version of sqlalchemy and flask-sqlalchemy for the supported python. Ideally, the SDK should work with every sqlalchemy version supported for each python version.
There was a problem hiding this comment.
The other py SDKs will fail the test with the latest sqlalchemy and flask-sqlalchemy version.
There was a problem hiding this comment.
Why does the X-Ray SDK tests fail for the latest version of sqlalchemy? we are not using any of the internal APIs of sqlalchemy in our tests right? Does that mean the SDK is not compatible with latest sqlalchemy for supported python versions?
There was a problem hiding this comment.
Some of the unit tests are using the API(for example), which will fail the test for the latest sqlalchemy and flask-sqlalchemy. Not sure if the Python SDK is compatible with the latest sqlalchemy or not, but at least some unit tests need to update the API they use from sqlalchemy and flask-sqlalchemy.
|
|
||
| for key, value in vars(self).items(): | ||
| if isinstance(value, bool) or value: | ||
| if key == 'subsegments': |
There was a problem hiding this comment.
How do we handle the case if key is annotations, aws or http ? I see that you're handling metadata case separately. Shouldn't we do the same with annotations too? since in that case also we don't know how many key-value pairs would be there.
There was a problem hiding this comment.
annotations, aws or http are all dict with serializable values like string, int or boolean, so there is no need to handle these cases.
We can do a follow-up PR to address this in future as seems like currently the |
Removal of jsonpickle: aws/aws-xray-sdk-python#275 git-svn-id: file:///srv/repos/svn-community/svn@904452 9fca08f4-af9d-4005-b8df-a31f2cc04f65
Removal of jsonpickle: aws/aws-xray-sdk-python#275 git-svn-id: file:///srv/repos/svn-community/svn@904452 9fca08f4-af9d-4005-b8df-a31f2cc04f65
| entity_dict[key] = subsegments | ||
| elif key == 'cause': | ||
| entity_dict[key] = {} | ||
| entity_dict[key]['working_directory'] = self.cause['working_directory'] |
There was a problem hiding this comment.
I couldn't dive into this implementation yet to provide a fix, but this is causing a regression in the AWS Lambda Powertools Tracer feature leading to a TypeError: string must be a list of integers
* Replace jsonpickle with json to serialize entity * Added workflow to create release tag * Pinned sqlalchemy and Flask-SQLAlchemy for unit test * Fixed version * Changed log to debug level * Update logging * Added empty line
Issue #, if available:
Security issue regarding
decodeAPI injsonpickle: jsonpickle/jsonpickle#335X-Ray Python SDK doesn't use
decodeAPI, but better find a replacementDescription of changes:
Replace
jsonpicklewithjsonto serialize entity dataBenchmark:
Some initial benchmark results using
pytest-benchmarkon Python 2.7:Will automate the benchmark across the library shortly
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.