ABNF definition for tracestate #107

SergeyKanzhelev · 2018-04-13T18:16:57Z

yurishkuro · 2018-04-13T18:49:18Z

trace_context/HTTP_HEADER_FORMAT.md

-`vendorName1=opaqueValue1,vendorName2=opaqueValue2`
+`tracestate` is a Structured header in accordance to [Draft of Structured Headers for HTTP](http://httpwg.org/http-extensions/draft-ietf-httpbis-header-structure.html). Its value MUST be a dictionary ([Draft of Structured Headers for HTTP](http://httpwg.org/http-extensions/draft-ietf-httpbis-header-structure.html), Section 4.1).
+
+Maximum length of a combined header MUST be less than 512 bytes. If the maximum length of a combined header is more than 512 bytes it SHOULD be ignored.


how did this 512 number come up? Why 512? HTTP Cookies are 4kb

https://github.com/w3c/distributed-tracing/blob/master/trace_context/HTTP_HEADER_FORMAT_RATIONALE.md#size-limits

Idea was that the size should be small enough so it will fit into messaging systems metadata for most systems. Like queues and such. It was a main concern against using the correlation context

I suggest adding the reason to the spec.

Added comment into the field definition. Please take a look

That rationale is vague and distracting. Can you put a concrete place where 512 would be a limitation? even one or two is better than something that a lot of people find distractingly small.

Should the header be ignored or truncated?

Truncation can only be formalized with the ordering (issue #80). I intentionally make a language SHOULD to allow for advanced implementations.

Also see mention of it in #110. I mention that for privacy concerns you may whitelist keys. But this is highly discouraged

AloisReitbauer · 2018-04-17T17:41:21Z

trace_context/HTTP_HEADER_FORMAT.md

+Example: `vendorName1=opaqueValue1,vendorName2=opaqueValue2`
+
+The value a concatenation of trace graph key-value pairs. Only one entry per
+key is allowed because the entry represents that last position in the trace.


This is already defined by using a dictionary

you commenting to outdated text

I think you missed discussion in #81

nicmunroe · 2018-04-27T22:22:13Z

correlation_context/HTTP_HEADER_FORMAT.md

@@ -14,27 +14,15 @@ Multiple correlation context headers are allowed. Values can be combined in a si

 ## Header value

+`tracestate` is a Structured header in accordance to [Draft of Structured Headers for HTTP](http://httpwg.org/http-extensions/draft-ietf-httpbis-header-structure.html). Its value MUST be a Parameterised List ([Draft of Structured Headers for HTTP](http://httpwg.org/http-extensions/draft-ietf-httpbis-header-structure.html), Section 4.3).


Should this be Correlation-Context, not tracestate?

nicmunroe · 2018-04-27T22:33:09Z

trace_context/HTTP_HEADER_FORMAT.md

 ## Header name

 `tracestate`

 ## Header value

-`vendorName1=opaqueValue1,vendorName2=opaqueValue2`
+This section uses the Augmented Backus-Naur Form (ABNF) notation of [RFC5234](https://tools.ietf.org/html/rfc5234), including the DIGIT rule from that document. It also includes the OWS rule from [RFC7230](https://www.rfc-editor.org/info/rfc7230).


Might be good to add some quick in-line layman definitions for DIGIT and OWS - they're not particularly complicated and would save some people not steeped in specs (like me) from having to go digging around in some lengthy RFCs.

At the very least more focused links that get the reader closer to the thing you're pointing out, like the DIGIT rule in appendix B.1 for RFC 5234 and the OWS rule from RFC 7230 Section 3.2.3 (the RFC7230 link you currently have doesn't take you directly to the spec, let alone a section in the spec - I recommend this tools.ietf.org/html link instead even if you don't deep link into the relevant section).

nicmunroe · 2018-04-27T22:42:24Z

trace_context/HTTP_HEADER_FORMAT.md

+
+Note that identifiers can only contain lowercase letters.
+
+Valus is opaque string up to 256 characters printable ASCII [RFC0020](https://www.rfc-editor.org/info/rfc20) characters (i.e., the range 0x20 to 0x7E) except comma `,` and `=`. Note that this also excludes tabs, newlines, carriage returns, etc.


👍 for the layman-readable definitions immediately surrounding the RFC-style definitions. Nicely done. I think we could use one of these layman definitions for dictionary and dict-member above. It appears to be the only piece of the puzzle missing a layman definition.

nicmunroe · 2018-04-27T22:45:09Z

trace_context/HTTP_HEADER_FORMAT.md

+lcalpha    = %x61-7A ; a-z
+```
+
+Note that identifiers can only contain lowercase letters.


Recommend changing this line to:

Note that keys must begin with a lowercase letter, and can only contain lowercase letters a-z, digits 0-9, underscores _, dashes -, asterisks *, and forward slashes /.

nicmunroe · 2018-04-27T22:59:14Z

trace_context/HTTP_HEADER_FORMAT.md

 ## Header name

 `tracestate`

 ## Header value

-`vendorName1=opaqueValue1,vendorName2=opaqueValue2`
+This section uses the Augmented Backus-Naur Form (ABNF) notation of [RFC5234](https://tools.ietf.org/html/rfc5234), including the DIGIT rule from that document. It also includes the OWS rule from [RFC7230](https://www.rfc-editor.org/info/rfc7230).
+


I'd suggest a layman definition here before the dictionary RFC-style definition, maybe something like:

The tracestate header value is a dictionary as defined below. The dictionary is a series of dict-members separated by commas ,, and a dict-member is a key/value pair separated by an equals sign =. Spaces and horizontal tabs surrounding dict-members are ignored. There can be a maximum of 128 dict-members in a dictionary.

A simple example of a dictionary with two dict-members might look like: vendorname1=opaqueValue1,vendorname2=opaqueValue2

I know you have that example later, but it might be good to have it up front to give people some context as they're reading through the RFC-style definitions.

SergeyKanzhelev · 2018-04-27T23:01:46Z

@nicmunroe great review! I think I addressed all the feedback.

nicmunroe · 2018-04-27T23:08:45Z

@SergeyKanzhelev I had one last recommendation that I took too long to write out before you addressed my comments. :) I guess I should have done an actual review and submitted it all at once rather than individual comments. Sorry!

Overall I'm pretty happy with how this reads between the combination of layman definitions and RFC-style definitions that are still fairly simple.

I'm personally punting on the 512 bytes issue for now, but don't take my silence on that particular issue as consent necessarily - I'd like to see it addressed as mentioned by others. The specific 512 number does feel a little arbitrary without an example or two of systems that have a 512 limit.

… clean up examples

nicmunroe · 2018-05-02T22:30:15Z

Based on the discussion here: #81 (comment)

Seems like we could slightly adjust the definition of tracestate to something like: tracestate = #(dict-member), getting rid of the dictionary definition, and it solves the multiple-same-named-header issues and implies that ordering is important all at the same time? (Maybe rename dict-member to ts-member or something more relevant after getting rid of dictionary).

It's possible I'm misreading, and I have no idea if the #(...) syntax from RFC 7230 Section 3.2.2 is valid ABNF syntax, but if this works then the definition of tracestate is even more succinct, and we can add some explanatory text that links to https://tools.ietf.org/html/rfc7230#section-3.2.2 and explains that multiple-same-named tracestate headers implies a comma separated list where ordering matters.

nicmunroe · 2018-05-03T00:20:16Z

If we decide to go this route of explicitly supporting multi-same-named-headers to solve some of our definition and ordering issues we should consider adding some SHOULD language to encourage implementations to combine multi-tracestate-headers into a single concatenated header when doing further HTTP propagation. This would maybe help with some of the accidental breakage we might see in the wild, e.g. calling headers.get("tracestate") instead of headers.getAll("tracestate"). A good spec-implementing citizen could then concatenate into a single header in the hopes of things "just working" anyway even if downstream does the wrong thing.

If we want to explicitly not support multi-same-named headers (something that RFC 7230 Section 3.2.2 implies we can do) then we probably would need to pick a different delimiter than comma. (And then we'd still probably have to define what to do if you receive a multi-header anyway since it will undoubtedly happen, like with cookies - see the commentary in appendix A.2.3. of this doc.)

mnot · 2018-05-03T03:06:05Z

This shouldn't reference or mention Structured Headers if it's not going to use it.

nicmunroe · 2018-05-03T04:06:41Z

Agreed. If we end up not going with structured headers then this PR name should be changed (again).

SergeyKanzhelev · 2018-05-09T17:00:42Z

I'm not sure what was the conclusion during workshop. Since we have ordering in specification already I adjusted the ABNF definition to use list instead of dictionary. Also it seems like everybody OK with proposed format as oppose to use of structured header.

Can you please let me know if this was discussed and whether there are still concerns with this particular PR.

yurishkuro reviewed Apr 13, 2018

View reviewed changes

SergeyKanzhelev mentioned this pull request Apr 16, 2018

Explain/remove 512 limit of tracestate in spec #109

Closed

AloisReitbauer reviewed Apr 17, 2018

View reviewed changes

SergeyKanzhelev mentioned this pull request Apr 18, 2018

Describe headers using ABNF format #81

Closed

Sergey Kanzhelev added 7 commits April 27, 2018 14:18

structured header format for tracestate

b453f41

added limit justification

f6e100d

removed explanaition of the limit of 512 bytes

b274b58

moved to explicit ABNF description

a898439

fix reference

7570f2f

remove unnecessary note

2f4eabf

adjusted rationale

d1254cc

SergeyKanzhelev force-pushed the sergkanz/structuredHeaderForTraceState branch from 25ce321 to d1254cc Compare April 27, 2018 21:23

nicmunroe reviewed Apr 27, 2018

View reviewed changes

addressed nic's feedback

efac3c9

nicmunroe reviewed Apr 27, 2018

View reviewed changes

Sergey Kanzhelev added 2 commits April 27, 2018 16:12

added human-readable description before jumping into ABNF

2b7aab3

created a TODO in correlation context to either have custom format or…

74faa2a

… clean up examples

nicmunroe mentioned this pull request May 3, 2018

Clarify the HTTP header name choice for trace #119

Closed

SergeyKanzhelev changed the title ~~structured header format for tracestate~~ ABNF definition for tracestate May 3, 2018

Sergey Kanzhelev added 2 commits May 3, 2018 15:55

revert C-C changes to simplify PR

5eb613d

renamed dict to list for consistency with tracestate mutations

45e63bd

AloisReitbauer approved these changes May 16, 2018

View reviewed changes

AloisReitbauer merged commit e2ce54a into master May 16, 2018

bogdandrutu deleted the sergkanz/structuredHeaderForTraceState branch October 12, 2018 01:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ABNF definition for tracestate #107

ABNF definition for tracestate #107

SergeyKanzhelev commented Apr 13, 2018

yurishkuro Apr 13, 2018

SergeyKanzhelev Apr 13, 2018

yurishkuro Apr 13, 2018

SergeyKanzhelev Apr 13, 2018

codefromthecrypt Apr 13, 2018

AloisReitbauer Apr 17, 2018

SergeyKanzhelev Apr 17, 2018

AloisReitbauer Apr 17, 2018

SergeyKanzhelev Apr 17, 2018

SergeyKanzhelev Apr 17, 2018

nicmunroe Apr 27, 2018

nicmunroe Apr 27, 2018

nicmunroe Apr 27, 2018

nicmunroe Apr 27, 2018

nicmunroe Apr 27, 2018

SergeyKanzhelev commented Apr 27, 2018

nicmunroe commented Apr 27, 2018

nicmunroe commented May 2, 2018

nicmunroe commented May 3, 2018 •

edited

Loading

mnot commented May 3, 2018

nicmunroe commented May 3, 2018

SergeyKanzhelev commented May 9, 2018

		@@ -14,27 +14,15 @@ Multiple correlation context headers are allowed. Values can be combined in a si

		## Header value

		`tracestate` is a Structured header in accordance to [Draft of Structured Headers for HTTP](http://httpwg.org/http-extensions/draft-ietf-httpbis-header-structure.html). Its value MUST be a Parameterised List ([Draft of Structured Headers for HTTP](http://httpwg.org/http-extensions/draft-ietf-httpbis-header-structure.html), Section 4.3).


		Note that identifiers can only contain lowercase letters.

		Valus is opaque string up to 256 characters printable ASCII [RFC0020](https://www.rfc-editor.org/info/rfc20) characters (i.e., the range 0x20 to 0x7E) except comma `,` and `=`. Note that this also excludes tabs, newlines, carriage returns, etc.

ABNF definition for tracestate #107

ABNF definition for tracestate #107

Conversation

SergeyKanzhelev commented Apr 13, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SergeyKanzhelev commented Apr 27, 2018

nicmunroe commented Apr 27, 2018

nicmunroe commented May 2, 2018

nicmunroe commented May 3, 2018 • edited Loading

mnot commented May 3, 2018

nicmunroe commented May 3, 2018

SergeyKanzhelev commented May 9, 2018

nicmunroe commented May 3, 2018 •

edited

Loading