Filter invalid characters from annotation key names; raise warning if found #22
Conversation
docs/basic.rst
Outdated
| @@ -51,7 +51,8 @@ You can add annotations and metadata to an active segment/subsegment. | |||
| Annotations are simple key-value pairs that are indexed for use with | |||
| `filter expressions <http://docs.aws.amazon.com/xray/latest/devguide/xray-console-filters.html>`_. | |||
| Use annotations to record data that you want to use to group traces in the console, | |||
| or when calling the GetTraceSummaries API. | |||
| or when calling the GetTraceSummaries API. Annotation keys may only use ASCII letters and the underscore(_) | |||
There was a problem hiding this comment.
Probably replace "may" with "should" as user needs to understand invalid keys are dropped, per RFC2119. Also ASCII digits are missing here.
aws_xray_sdk/core/models/entity.py
Outdated
| @@ -140,7 +141,12 @@ def put_annotation(self, key, value): | |||
| log.warning("ignoring unsupported annotation value type %s.", type(value)) | |||
| return | |||
|
|
|||
| self.annotations[key] = value | |||
| sanitized_key = ''.join([c for c in key if c in _valid_annotation_key_characters]) | |||
There was a problem hiding this comment.
Annotation key is difference to entity name. User can have 100 lines of code adding annotations to a single entity. Under worst case this could result in 100 times of string concatenation, which adds a lot of overhead.
Invalid key is a user error and the key should be dropped until the author explicit fix it using valid characters.
|
Thank you for the PR and we really appreciate it. I have left some comments and would expect this can be merged very soon. |
|
Thank you for the review, I will make revisions shortly.
…On Mon, Feb 19, 2018, 12:06 PM Haotian Wu ***@***.***> wrote:
Thank you for the PR and we really appreciate it. I have left some
comments and would expect this can be merged very soon.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#22 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ACWrSHpEmwZlfy5uc3heOIWg2KjPJIcHks5tWdRJgaJpZM4SJaFT>
.
|
…names and drop annotation if invalid
|
@haotianw465 revisions made. |
|
It looks all good. Sorry that I forgot to mention if you could add two line check on this unit test https://github.com/aws/aws-xray-sdk-python/blob/master/tests/test_trace_entities.py#L47 to cover the new key dropping logic. Thanks. |
|
Sure thing.
…On Mon, Feb 19, 2018, 12:32 PM Haotian Wu ***@***.***> wrote:
It looks all good. Sorry that I forgot to mention if you could add two
line check on this unit test
https://github.com/aws/aws-xray-sdk-python/blob/master/tests/test_trace_entities.py#L47
to cover the new key dropping logic. Thanks.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#22 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ACWrSIgn-1GPN06NHdca4MU7rjQteTTDks5tWdpFgaJpZM4SJaFT>
.
|
|
@haotianw465 test updated! |
Fixes #18
I took the approach of stripping invalid characters from annotation key names and raising a warning, rather than exiting the function.