Fix TraceState to adhere to specs #1502
Conversation
srikanthccv
requested review from
aabmass and
hectorhdzg
and removed request for
a team
| typing.Sequence[typing.Tuple[str, str]] | ||
| ] = None, | ||
| ) -> None: | ||
| self._dict = collections.OrderedDict() |
There was a problem hiding this comment.
I wonder if we should keep the previous behaviour and have TraceState be the actual dictionary, instead of having to construct a TraceState object?
There was a problem hiding this comment.
That would make the TraceState to be mutable by anyone and insertion order remembering is not a language feature until python3.7. Specs says it should be immutable list of string key/value pairs, and all mutable operations (through add, update, delete) should validate the inputs and return a new TraceState and order of pairs should always be preserved.
API should provide the add, update, delete operations. To achieve this we would still need to inherit from mapping and implement them. In case if we consider to drop support for lower python versions we should override __setitem__ and __delitem__ to not allow mutating directly. Keeping all this in mind, I think we should have TraceState object and it wouldn't be feasible to follow spec with actual dictionary.
…ry-python into trace-state-fix
| A string that adheres to the w3c tracestate | ||
| header format. | ||
| """ | ||
| return ",".join(key + "=" + value for key, value in self._dict.items()) |
There was a problem hiding this comment.
Im not super familiar with the tracestate spec; do we need some escaping in the values here?
There was a problem hiding this comment.
From W3C Trace Context spec
The value is an opaque string containing up to 256 printable ASCII characters (i.e., the range 0x20 to 0x7E) except comma (,) and (=).
If I understand it correctly we don't need to do any escaping as this header will be ASCII only which is totally fine with HTTP Headers.
| # empty members are valid, but no need to process further. | ||
| if not member: | ||
| continue | ||
| match = _MEMBER_PATTERN.fullmatch(member) |
There was a problem hiding this comment.
We have util like _is_valid_pair(), why not have a is_valid_member()?
There was a problem hiding this comment.
All the mutation operations on trace state should validate the key/value. The member would be in format of key=value. If we go with is_valid_member() we will have construct member from key/value params. If we stick with is_valid_pair we need to split the member from header to give pair. Either way would be fine.
| _DELIMITER_PATTERN, | ||
| _MEMBER_PATTERN, | ||
| _TRACECONTEXT_MAXIMUM_TRACESTATE_KEYS, | ||
| _is_valid_pair, |
There was a problem hiding this comment.
These imports shouldn't be named with underscore prefixes right?
There was a problem hiding this comment.
I think these are for internal use and should have leading underscore. Ideally users should only rely on TraceState and inject/extract API.
Co-authored-by: Aaron Abbott <aaronabbott@google.com>
|
@lonewolf3739 |
Done. |
Description
Fixes #1487
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
tox -e test-core-apitox -e test-core-sdkDoes This PR Require a Contrib Repo Change?
Checklist: