Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use IETF Dictionary for tracestate #197

Closed
discostu105 opened this issue Oct 25, 2018 · 11 comments
Closed

Use IETF Dictionary for tracestate #197

discostu105 opened this issue Oct 25, 2018 · 11 comments
Labels
Decision This label is used to summarize decision that need to be made (open) and have been made (closed)
Milestone

Comments

@discostu105
Copy link
Member

discostu105 commented Oct 25, 2018

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 (httpwg/http-extensions#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: #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

  • item-value definitions are different (in the kind of characters they allow)
  • key and value size restrictions in IETF are larger (or non-existent). -> we could make the size restrictions RECOMMENDATIONS instead of spec rules
  • IETF defines more datatypes: IMHO it is fine to allow all those datatypes. There are no reasons against it. It might make frameworks that support tracestate more complicated (more API's), but that might not be a problem at all.

Pros

  • It's (going to be) a standard format, that we don't need to re-invent
  • There'll be parser-implementations that can be re-used

Cons

  • still has whitespaces (OWS) (-> parsing logic/perf?).
    • only around commas, not around equal. makes searching/(parsing a little easier

Open questions

  • What about the multiple-headers? Does dictionary still allow these? is order preserved with multiple headers? Is "ordered" property still given, if headers are split into multiples? @mnot
    • Given the order is preserved (see https://tools.ietf.org/html/rfc7230, section 3.2.2), we might be fine.
    • We might end up with empty entries at concatenation. we might have to be able to deal with that (e.g. nr="abc", , dt="cdf")

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

@discostu105 discostu105 added Decision This label is used to summarize decision that need to be made (open) and have been made (closed) trace-context labels Oct 25, 2018
@discostu105 discostu105 added this to the 4. CR milestone Oct 25, 2018
@yurishkuro
Copy link
Member

yurishkuro commented Oct 25, 2018

Personally, big -1 for multiple types and quotes:

  • all existing implementations of the spec become invalid
  • parsing becomes a lot more complicated
  • there is no justification in this proposal
    • why non-string values are needed
    • what the concrete use cases for that are
    • cost-benefit analysis of significantly complicating the parsing rules to support those use cases

@SergeyKanzhelev
Copy link
Member

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
Copy link
Member Author

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
Copy link
Contributor

mtwo commented Oct 26, 2018

@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:

  • A standard type that has been designed by experts who have lived through header definitions and thought through corner cases that we'll have missed. Its ordered nature also meets our performance requirements.
  • A smoother approval process
  • Less cynicism from future implementors, who would question why we invented our own dictionary type
  • Theoretical support from standard parsing libraries (though I question how valuable this actually is beyond validation scenarios)

For these benefits, we pay the following price:

  • Breaking existing experimental clients
  • Two extra characters per dictionary entry, however the dictionary has a maximum size of five items

To me, this seems like a good choice

@yurishkuro
Copy link
Member

yurishkuro commented Oct 26, 2018

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
Copy link
Contributor

@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:

  • Tracing Tool Providers
  • Cloud Providers
  • Middleware ISV Providers (e.g. messaging busses)
  • Network Appliance providers (e.g. firewalls, ...)

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
Copy link
Member

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
Copy link
Contributor

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
Copy link
Member Author

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).

  • We're already too far in the w3c process to make such a breaking change
  • IETF dictionary timeline does not look like it's being finished as soon as we need it

@mnot
Copy link
Member

mnot commented Feb 12, 2019

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
Copy link
Member Author

@mnot ok, thanks for clarifying :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Decision This label is used to summarize decision that need to be made (open) and have been made (closed)
Projects
None yet
Development

No branches or pull requests

6 participants