Reorganizes content to reflect decisions made since last workshop #72
Conversation
The following is an "all-in-one" change to capture the essence of changes described since the February workshop. Notably, this includes: * lowercase concatenation naming convention (ex `traceparent`) * applied description of the multiple trace states As the two headers are tightly coupled, this places them in the same document. This is done as one change as there is pressure for folks to do implementations and so far very few changes have actually taken place. By spiking a review to integrate feedback, we are more likely to have interoperable implementations without accidentally implementing old versions of the standard. Please take this practical concern into consideration when reviewing. Fixes #57
trace_context/HTTP_HEADER_FORMAT.md
Outdated
| Single header: | ||
|
|
||
| ``` | ||
| tracestate: parent_application_id = 123 |
There was a problem hiding this comment.
self-note: this example is crap
| This document provides rationale for the decisions made, mapping the | ||
| `Trace-Parent` and `Trace-State` fields to HTTP headers. | ||
|
|
||
| ## Lowercase Concatenated header names |
| - It is expected that the typical name will be a single word in latin and the value will be a | ||
| short string in latin or a derivative of an url. | ||
|
|
||
| ### Field name without value |
|
ps here's the notes this is based on https://docs.google.com/document/d/1YJ0oriH62Ixk3W37z31X0SeuXpgXpXBetiz8EWxG6NU/edit |
trace_context/HTTP_HEADER_FORMAT.md
Outdated
|
|
||
| ## Header value | ||
|
|
||
| `name1[=value1[;properties1]],name2[=value2[;properties2]]` |
There was a problem hiding this comment.
Is this meant to represent vendor-specific "golden copy"?
There was a problem hiding this comment.
yes. will adjust the example though I think we should remove any notion of nested encoding for now. I don't think we will want to mix atoms and molecules (ex atom being some random property and molecule being opaque trace state for amazon)
There was a problem hiding this comment.
I think =value1[;properties1] looks like an encode format to me, which I am not sure whether others agree. In another words, should value end with , and start with = be good enough?
There was a problem hiding this comment.
Agreed that this feels a bit too open to misinterpretation. Not sure what the best way to represent it here is, but ultimately readers should get the hint that this is vendorName1=opaqueValue1,vendorName2=opaqueValue2.
There was a problem hiding this comment.
ripped out the copy/pasted stuff from correlation context
There was a problem hiding this comment.
I think this looks great. I appreciate the move to lower case+concat to improve interop with MQ's.
It might be nice to see some additional examples with variations, for example multiple different vendors in tracestate. If we get a rich enough set of examples with edge cases, that would feed well into a compatibility test suite later.
trace_context/HTTP_HEADER_FORMAT.md
Outdated
| is only one trace graph and that is from the Congo service: | ||
|
|
||
| ``` | ||
| trace-parent: 00-4bf92f3577b34da6a3ce929d0e0e4736-00f067aa0ba902b7-01 |
There was a problem hiding this comment.
Remove - character in name to reflect new name (below too).
|
Had an offline discussion with @tylerbenson which was good as he wasn't at the workshop (except remote), which helped flesh out some things to address. There's certainly one of those things here, with this PR, which is priority. I think there's very high priority completely breaking the old spec based on the discussion with 30people over 2 days in feb. We need to do that so people stop implementing the old thing. OTOH, fleshing out everything including protocol in a single pull request will put it at risk. Here's some things I think from the chat with Tyler, I can work into this one.
|
trace_context/HTTP_HEADER_FORMAT.md
Outdated
|
|
||
| ## Header value | ||
|
|
||
| `name1[=value1[;properties1]],name2[=value2[;properties2]]` |
There was a problem hiding this comment.
I think =value1[;properties1] looks like an encode format to me, which I am not sure whether others agree. In another words, should value end with , and start with = be good enough?
trace_context/HTTP_HEADER_FORMAT.md
Outdated
|
|
||
| ## Relationship between the headers | ||
|
|
||
| The `tracestate` header is not an extension of the data in `traceparent`, |
There was a problem hiding this comment.
is not an extension is totally clear to me, but, I have to say, for new guys join this community, they will have no clue about where does this come from. The specification document should stay in the recent status. We may provide a CHANGELOG to describe the changes.
trace_context/HTTP_HEADER_FORMAT.md
Outdated
|
|
||
| ## Value format | ||
|
|
||
| Value starts after equal sign and ends with the special character `;`, separator `,` or end of string. Value represents a url encoded string and case sensitive. Spaces are allowed in the beginning and the end of the value. Value with spaces before and after MUST be considered identical to the trimmed value. |
There was a problem hiding this comment.
Does most people agree about this format of state.
There was a problem hiding this comment.
What's the purpose or benefit of including semicolon ; as a value-ending character? Can we (or should we) just ignore it and say that commas , and end-of-string are the only things that end a value?
I guess I'm assuming that "value" in this context means the entire opaque vendor-specific trace state, and since it's opaque they could use ; if they want to but by itself it has no special meaning and should therefore not be called out - is that the case or am I misunderstanding?
There was a problem hiding this comment.
It's also implicit in here that the value-ending characters (e.g. ,) are therefore reserved and should not be used in a vendor's opaque trace state value, but might be worth explicitly calling it out.
There was a problem hiding this comment.
done (the old formatting was unnecessary in opaque, but restricted characters are)
| For example, the following: | ||
| ``` | ||
| trace-parent: 00-4bf92f3577b34da6a3ce929d0e0e4736-00f067aa0ba902b7-01 | ||
| trace-state: yelp |
There was a problem hiding this comment.
Should demo about trace-state without value, but with other vendor/product value.
Such as:
trace-state: yelp,alpha=00-4bf92f3577b34da6a3ce929d0e0e4736-00f067aa0ba902b7-01
trace_context/README.md
Outdated
| * `Trace-State` maps all graphs the incoming parent is a part of in potentially vendor-specific formats. For example, if an ancestor of this request was in a different trace, there will be a separate entry for the last position it was in that trace graph. | ||
|
|
||
| Notably, the `Trace-State` field is unreliant on data in the `Trace-Parent`, | ||
| except if the direct upstream is fully described by `Trace-Parent`. |
There was a problem hiding this comment.
Maybe I'm missing something here, but how could a user tell whether the "direct upstream is fully described by Trace-Parent"? Specially in the case of having more than one trace graph involved.
Also, in the case of multiple trace graphs is there any rule regarding which of the identifiers should stay in the traceparent header? Are implementors allowed to "move" a different set of identifiers between the parent and state headers? e.g. traceparent has yelp-generated identifiers but as soon as it crosses to another vendor, they might want to propagate yelp's identifiers in tracestate and have their own in traceparent.
There was a problem hiding this comment.
I've removed the ability to split across headers (as this was copy/paste from correlation context) and opened an issue in case folks want to revisit later #80
|
Maybe I'm missing something here, but how could a user tell whether the
"direct upstream is fully described by Trace-Parent"? Specially in the
case of having more than one trace graph involved.
we are only allowing one entry for "traceparent", so the special case is
where the sender of a message is fully described by that (generic impl).
This doesn't mean transitively all ancestors are in the traceparent (others
are in tracestate).
Also, in the case of multiple trace graphs is there any rule regarding
which of the identifiers should stay in the traceparent header? Are
implementors allowed to "move" a different set of identifiers between the
parent and state headers? e.g. traceparent has yelp-generated identifiers
but as soon as it crosses to another vendor, they might want to propagate
yelp's identifiers in tracestate and have their own in traceparent.
only "yelp" (or a "yelp" clone) can resume trace state named "yelp". So if
the direct upstream isn't you (because the tag it corresponds with isn't
yours or something you are compatible with), then you restart a trace.
It could be perhaps helpful to make the special-cased section on generic
provider (no data except traceparent) in a completely different section far
away from all of this. Reason is that when there is no special casing, it
is simple. The most recent tracestate entry is the caller. If you are
compatible with that entry you can continue that trace. If you aren't you
insert a new entry for yourself (or replace yours if you are later in the
tracestate list)
… |
trace_context/HTTP_HEADER_FORMAT.md
Outdated
| `name1[=value1[;properties1]],name2[=value2[;properties2]]` | ||
|
|
||
| **Limits:** | ||
| Maximum length of a combined header MUST be less than 512 bytes. |
There was a problem hiding this comment.
Might be a deferred/todo thing, but at some point we might want to define recommended behavior for when you can't add your vendor's state to the header because it would put it over 512 bytes. Find the biggest offender and drop it on the floor? Find the oldest one and drop it (probably not possible to determine oldest one)? Throw everything on the floor and start fresh with just your vendor state?
There was a problem hiding this comment.
This will be part of the processing model and needs to be defined explicitly.
There was a problem hiding this comment.
theres already a TODO, which preceded this. lifecycle isn't well defined yet and too much to do in one PR, but good note
trace_context/HTTP_HEADER_FORMAT.md
Outdated
|
|
||
| Value starts after equal sign and ends with the special character `;`, separator `,` or end of string. Value represents a url encoded string and case sensitive. Spaces are allowed in the beginning and the end of the value. Value with spaces before and after MUST be considered identical to the trimmed value. | ||
|
|
||
| ## Properties |
There was a problem hiding this comment.
Based on other comments and my own personal opinion, I'd favor chopping this entire Properties section and just call out that the vendor value is opaque and vendors can do whatever they want as long as they don't use value-ending characters (e.g. ,) or otherwise violate the RFC 7230 spec.
I'd also be perfectly happy with @adriancole's suggestion that maybe the vendor value should be explicitly base64 rather than free-for-all. I might even prefer that - it means some extra work when parsing/extracting your trace graph, but it could seriously reduce confusion and misinterpretation when humans visually look at a tracestate header and try to understand it without reading this specification.
There was a problem hiding this comment.
And that confusion/misinterpretation is inevitable when key=val pairs are embedded everywhere in a large header value.
|
Honestly, I find it weird to describe the header format in plain English. We should use EBNF syntax to describe it. As an example look at the grammar for the HTTP Cookie header ... delimiter = %x09 / %x20-2F / %x3B-40 / %x5B-60 / %x7B-7E day-of-month = 1*2DIGIT ( non-digit *OCTET ) |
trace_context/HTTP_HEADER_FORMAT.md
Outdated
| ZSBzaG9ydCB2ZWhlbWVuY2Ugb2YgYW55IGNhcm5hbCBwbGVhc3VyZS4= | ||
| ``` | ||
|
|
||
| There is one exception to the "gold copy" rule, which is when the incoming |
There was a problem hiding this comment.
I agree with @adriancole on moving this generic case section elsewhere. I think there is some good potential value in the option, but I'm not sure the benefit outweighs the cost at the moment. The costs as I see it:
- It adds to the cognitive load required to understand the spec, and is not required for successful execution of an impl (i.e. if nobody does this then the spec still works).
- In order for this to work an impl needs to recognize when the incoming generic case is not compatible with their tracing system, and when they see an incompatibility they must do something special or else the trace graph of the vendor using the generic case is busted. The "something special" is copying the
traceparentvalue into the generic vendor's state. e.g. in this case my tracing system would have to settracestate: mystuff=someOpaqueValue,yelp=00-4bf92f3577b34da6a3ce929d0e0e4736-00f067aa0ba902b7-01(possibly base64-ing it if we decide that's required for the opaque vendor payload values). If I fail to do that copy, then the next time a yelp system sees this trace context they will not be able to resume correctly - their trace graph will be broken and probably unfixable. - The above issue is tricky to explain and potentially easy to goof up. It increases the complexity of an impl and goes against the general ethos we seemed to agree on in the workshop that you shouldn't mess with another vendor's gold copy. And when someone does goof up it means breaking other vendors' trace graphs.
For these reasons I'd recommend tabling this feature/option for now, and focus on the bare minimum to allow the spec to reach its goals. The less moving parts and the simpler it is, the higher likelihood of success IMO. Like I said I do think there's value here and it's worth revisiting, I just don't think it needs to be here right now. Unless we think it needs to be now or never, in which case it's probably worth discussing further.
trace_context/README.md
Outdated
|
|
||
| * `Trace-Parent` describes the position of the incoming request in its trace graph in a portable, fixed-length format. Its design focuses on fast parsing. | ||
|
|
||
| * `Trace-State` maps all graphs the incoming parent is a part of in potentially vendor-specific formats. For example, if an ancestor of this request was in a different trace, there will be a separate entry for the last position it was in that trace graph. |
There was a problem hiding this comment.
I'm having trouble parsing the first sentence here. I think it could benefit from a rewording. I'd suggest something but I'm not sure exactly what it's trying to say.
|
working in feedback now. Thanks folks |
|
by the way I am not going to endlessly work things in. I am going to work in as much as I can. we can decide whether to merge things or not, then raise a PR. blocking on perfection such as nailing EBNF is not something I'll be doing in this PR |
|
ok I updated based on the feedback, with two followups for a future work. I'm kindly asking to find way to merge this and only put blocking comments as again.. we are in a position of holding up everyone. Here are the followups
|
The following is an "all-in-one" change to capture the essence of changes
described since the February workshop. Notably, this includes:
traceparent)As the two headers are tightly coupled, this places them in the same document.
This is done as one change as there is pressure for folks to do implementations
and so far very few changes have actually taken place. By spiking a review to
integrate feedback, we are more likely to have interoperable implementations
without accidentally implementing old versions of the standard. Please take this
practical concern into consideration when reviewing.
Fixes #57