w3c / trace-context

Trace Context
https://w3c.github.io/trace-context/
Other
470 stars 76 forks source link

Use IETF Dictionary for tracestate #197

Closed discostu105 closed 5 years ago

discostu105 commented 6 years ago

When discussing tracestate datastructures in WG, we agreed on the proposal to use IETF structured dictionary instead of list (@AloisReitbauer, @mtwo, @bogdandrutu, @victorNewRelic, @mwear, @danielkhan, @nikmd23, @tedsuo, @discostu105).

Here is the latest version: https://tools.ietf.org/html/draft-ietf-httpbis-header-structure-08#section-3.1

It turns out this version of structured headers just added the "ordered" property for dictionaries (https://github.com/httpwg/http-extensions/issues/659) recently.

Since we need ordered property, this now turns out to be an attractive candidate to use for tracestate.

It has definitions for different types for the item-value (e.g. string, binary, int, ...).

example:

  Example-DictHeader: en="Applepie", da=*w4ZibGV0w6ZydGU=*

Strings are quoted, which is a breaking change to parsers compared to the current spec! However, it's nice to have a well-defined rules for non-string datatypes (e.g. base64 case, which New Relic is going to use).

Another change compared to the current spec is that the item-key would not allow the @ symbol (PR: https://github.com/w3c/distributed-tracing/pull/153). In the IETF spec, the key only allows:

sh-identifier = lcalpha *( lcalpha / DIGIT / "_" / "-"/ "*" / "/" )
lcalpha       = %x61-7A ; a-z

Getting rid of the '@' definition can have a discussion. From dynatrace perspective it's ok from the current perspective, since we will add id's into our key anyway, we will have no problem of uniquely finding that key with a string-search. Curious if that is also going to be ok for @SergeyKanzhelev , @erabug .

Changes to current spec

Pros

Cons

Open questions

We'd appreciate feedback/discussion on this move from participants who are not present in Lyon.

yurishkuro commented 6 years ago

Personally, big -1 for multiple types and quotes:

SergeyKanzhelev commented 6 years ago

tracestate is for the opaque values. It is bad for protocols that needs to compress data, but making values typed doesn't make anything better as everything will be a string now and will require quotes or special character to indicate "binary".

Current spec is written in assumption that vendors will put all vendor-specific fields inside the single vendor tracestate value. And only few tracestate keys will be shared amongst vendors. This proposal changes that assumption as it will only be helpful for the case if every field defined in a separate tracestate key-value pair.

Looking on existing implementations I feel that most vendors will prefer to manipulate a single tracestate pair. This gives consistency and bigger independence from other implementations. So this change is not justified by the need of vendors.

I'd suggest to keep spec as it is now.

P.S. There is a chance that when everybody will start respecting traceparent and more collaborative scenarios will start lighting up we will need a change like this for better compression and interoperability. But I don't see it's happenning very soon.

discostu105 commented 6 years ago

I hear your points about multiple datatypes. We don't need them, since every vendor just needs to understand her own entry and can encode it in any way she wants.

The argument was more about using a well-defined standard format, vs. defining our own custom format. In the former, you'd be able to re-use parsers, in the latter you'd always have to write a special one.

That being said, IETF standard is not even release, so there are no parsers for it as of today. Also, the spec might still be unstable.

mtwo commented 6 years ago

@yurishkuro To my knowledge, the only existing implementations are prototypes by New Relic, Instana, and Dynatrace, and a version available in OpenCensus. Given this, changing the tracestate spec at this point doesn't seem to present huge challenges for implementors, but perhaps there are other implementations that we're unaware of?

@SergeyKanzhelev @yurishkuro The biggest motivation for switching to the IETF structured format is that it's a internet standard that well supports our requirements. With a custom format, we've been advised that the first question that we'll be asked during approval with groups like the W3C and IETF will be "why don't you use the same list / dictionary as everyone else?", and we won't have a good answer beyond not wanting to break existing experimental clients.

The way I see it, we gain the following:

For these benefits, we pay the following price:

To me, this seems like a good choice

yurishkuro commented 6 years ago

approval with groups like the W3C and IETF

This is something that always bothered me in this project. As far as I am concerned, we are the standards body. We agree on a spec that works for us, the actual practitioners who write the actual code and worry about it's performance and maintenance. Shouldn't the goal be a spec that is useful for our domain, easy to implement, and performant, rather than worrying about explaining it to some third party who have no stake in the game? [end of rhetorical rant]

I'm also not convinced by the arguments about relying on another standard. We don't invent a Turing-complete language when all we need is to pass a hex string. That dictionary standard is a massive overkill for our use case, difficult to implement, and as you say has no reference libs to even show how to implement it.

I understand the value of reuse, but not in the form of importing 100k LOC library to use a single function. If the dictionary spec provides a useful foundation, then let's find a way to explicitly restrict it to something that makes sense.

And yes, there are other prototype implementations.

AloisReitbauer commented 6 years ago

@yurishkuro

The team is aware that this change will break implementations. However, during our last F2F meetings and teleconferences we always pointed out that this part of the spec was not yet stable. We had tracers implement it to prove the concept, knowing that implementations might change.

For a standard, it is important to support all stakeholders. In the case of trace context these are:

All of these stakeholders need to parse HTTP headers so they need to support structured header parsing anyways. Trace context would be a special case that will require all of them to add dedicated support for parsing this additional header. This will slow down adoption and contradicts our goal of fast industry adoption.

We also discussed providing a reference implementation for parsing the header. So implementors will have access to an open source implementation for parsing that will do this efficiently; I am also confident this will not be 100k lines of code.

It is always hard to capture 5 days of discussions in a GitHub issue. That's why we encourage implementors to join the F2F meetings. We will have a discussion of the resulting PR in our next teleconference before merging. I recommend you join this call

yurishkuro commented 6 years ago

I have stated my objections. I strongly recommend implementing and profiling things first before deciding on standardising something.

Separately, f2f meetings are perfectly fine for discussions. Voting in the open source projects should be carried out publicly. And people have justified expectations to have the decisions or recommendations reached via f2f to be properly documented. I asked above - what are the must-have use cases that require trace context to use a dictionary with types values?

AloisReitbauer commented 6 years ago

Just to be clear. Using a dictionary and value types does not require all value types to be supported for a specific header. We could also choose to support only string and binary.

discostu105 commented 5 years ago

We discussed in todays call to not adpot IETF dictionary format at this point for the following reasons (in addition to all the points that have been brought up in the comments).

mnot commented 5 years ago

Thanks @discostu105. Just to point #2 - it's about to go into Working Group Last Call (a ~two week process), after which it'll go into IETF Last Call (~2 more), then be approved.

discostu105 commented 5 years ago

@mnot ok, thanks for clarifying :)