allow empty headers #188
allow empty headers #188
Conversation
| @@ -207,6 +207,11 @@ Platforms and libraries MUST expect header name in any casing and SHOULD send he | |||
|
|
|||
| ## Header value | |||
|
|
|||
| Multiple `tracestate` headers are allowed. Values from multiple headers in | |||
| incoming requests SHOULD be combined in a single header according to the | |||
There was a problem hiding this comment.
The idea is to increase interoperability by making sure it's always a single string with defined order of elements and easy to calculate length. From implementation perspective it is also easier if we talking about libraries, not generic proxies.
What's your main concern with this "SHOULD"?
There was a problem hiding this comment.
In the HTTP protocol multiple headers and single header with comma-separated values are interchangeable. I don't see the point of putting more stringent restrictions, because a compliant HTTP middleware is free to swap those two representations.
There was a problem hiding this comment.
That's why it SHOULD, not MUST. Same as here:
Platforms and libraries MUST expect header name in any casing and SHOULD send header name in lower case.
There was a problem hiding this comment.
This is why I object to this. If you MUST treat different inbound formats anyway, putting additional restrictions on the outbound format adds 0 value to anyone, just becomes an arbitrary thing in the spec with no justification.
trace_context/HTTP_HEADER_FORMAT.md
Outdated
| @@ -215,16 +220,25 @@ The `OWS` rule defines an optional whitespace. It is used where zero or more whi | |||
|
|
|||
| The `tracestate` field value is a `list` as defined below. The `list` is a series of `list-members` separated by commas `,`, and a `list-member` is a key/value pair separated by an equals sign `=`. Spaces and horizontal tabs surrounding `list-member`s are ignored. There can be a maximum of 32 `list-member`s in a `list`. | |||
|
|
|||
| Empty and whitespace-only list elements are allowed as a list value. But the use | |||
There was a problem hiding this comment.
I recommend using consistent terminology: list members or list elements, not both.
trace_context/HTTP_HEADER_FORMAT.md
Outdated
| @@ -215,16 +220,25 @@ The `OWS` rule defines an optional whitespace. It is used where zero or more whi | |||
|
|
|||
| The `tracestate` field value is a `list` as defined below. The `list` is a series of `list-members` separated by commas `,`, and a `list-member` is a key/value pair separated by an equals sign `=`. Spaces and horizontal tabs surrounding `list-member`s are ignored. There can be a maximum of 32 `list-member`s in a `list`. | |||
|
|
|||
| Empty and whitespace-only list elements are allowed as a list value. But the use | |||
| of empty `tracestate` header should be considered an exception. The reason for | |||
There was a problem hiding this comment.
What does it mean "to be considered exception"? Does http even allow headers without values?
| value when multiple `tracestate` headers were sent. Whitespace | ||
| characters are allowed for a similar reason as some frameworks will | ||
| inject whitespace after `,` separator automatically even in case of an | ||
| empty header. |
There was a problem hiding this comment.
I understand the whitespace use case, but don't understand "empty list member" use case. Under what conditions would that occur?
There was a problem hiding this comment.
We tried python, C# and Go. And there are libraries which will merge headers for you instead of giving the list. Also some libraries will insert spaces with the comma, some wouldn't. So make spec loose to simplify implementors life
There was a problem hiding this comment.
if libraries join headers into a string, they may add whitespace, but they will not create empty list members unless the inbound headers have empty values, which I think is not allowed by HTTP spec.
There was a problem hiding this comment.
We discussed not defining trace-state as a list. Technically, this FORBIDS an implementation to split the header into multiple headers.
|
I support this PR. Had the same experience when implementing the current spec to conform this test-case. I had to add extra code to detect an empty header and reject a trace based on that. Allowing empty tracestate headers would make it easier. |
|
@yurishkuro I'll update the terminology. For the SHOULD - I think it's a reasonable request to make for the implementors to merge if possible. It will make less mistakes and implementation that fail to read additional headers |
Update test service to allow empty Tracestate headers. See w3c/trace-context#188.
Update test service to allow empty Tracestate headers. See w3c/trace-context#188.
Implementing
tracestatefor various languages proved it's very hard to conform to the emptytracestateheader test case. Code becoming too complex. Thus allowing empty value to simplify implementation.Also addresses #171