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

Allow multiple tracestate entries for one vendor #137

Closed
discostu105 opened this issue Jul 27, 2018 · 15 comments
Closed

Allow multiple tracestate entries for one vendor #137

discostu105 opened this issue Jul 27, 2018 · 15 comments
Milestone

Comments

@discostu105
Copy link
Member

discostu105 commented Jul 27, 2018

Use Case

Multi-tenant tracing.

Currenty TraceContext elegantly solves the problem of different tracing system that interact with each other. Via tracestate, "foreign" vendor information is transported throughout the trace. This is great, but a system traced only by one vendor can have a similiar problem. If services are monitored by different tenants/customers of that vendor, the trace breaks (assuming the vendor is relying on tracestate information).

Concrete example
A 3-tier trace where the first and the last tier are monitored by the same tenant (A), but the in-between tier is monitored by a different tenant (B), but both use the same APM vendor.

(A) -> (B) -> (A)

1. tracestate="vendor1=tenant:0xaaa;importantData:0xfff"
2. tracestate="vendor1=tenant:0xbbb;importantData:0xeee"
3. tracestate="vendor1=tenant:0xaaa;importantData:0xccc"  // trace for tenant A is broken

Since, with current design, a vendor can only use one key in tracestate, the trace would break.

Possible Solutions

  1. Vendor needs to encode multi-tenant support into a single tracestate entry.
1. tracestate="vendor1=(tenant:0xaaa;importantData:0xfff)"
2. tracestate="vendor1=(tenant:0xbbb;importantData:0xeee)|(tenant:0xaaa;importantData:0xfff)"
3. tracestate="vendor1=(tenant:0xaaa;importantData:0xfff)|(tenant:0xbbb;importantData:0xeee)"
  1. Vendor is allowed to have multipe tracestate enties with the same key.
1. tracestate="vendor1=tenant:0xaaa;importantData:0xfff"
2. tracestate="vendor1=tenant:0xbbb;importantData:0xeee, vendor1=tenant:0xaaa;importantData:0xfff"
3. tracestate="vendor1=tenant:0xaaa;importantData:0xfff, vendor1=tenant:0xbbb;importantData:0xeee"
  1. Vendor is allowed to encode dynamic info into the tracestate key.
1. tracestate="vendor1-0xaaa=importantData:0xfff"
2. tracestate="vendor1-0xbbb=importantData:0xeee, vendor1-0xaaa=importantData:0xfff"
3. tracestate="vendor1-0xaaa=importantData:0xfff, vendor1-0xbbb=importantData:0xeee"

Thoughts

  • Solution 1 would not need any change to the current tracestate spec. But it would require every vendor to solve a problem, that is pretty much elegantly solved already with tracestate (for multi-vendors).
  • Solution 2: The current spec says "only one entry per key". This would need to be changed. It might have implications on the data-type we use for the HTTP-header. It also bears the risk of implementations doing this wrong (Map instead of MultiMap).
  • Solution 3: Could be a legit solution. We'd need to define if keys can be arbitraty, or if there is still a defined format (<vendor>-<dynamicdata>) where <vendor> should still be a registered name (Provide vendor/open source product name register for tracestate #58).

There's been an issue by @SergeyKanzhelev , which I believe might overlap with this and maybe serves the same use-case: #117.

@codefromthecrypt
Copy link

codefromthecrypt commented Jul 27, 2018 via email

@codefromthecrypt
Copy link

codefromthecrypt commented Jul 27, 2018 via email

@discostu105
Copy link
Member Author

@adriancole do I understand that correctly, that your suggestion would be basically Solution 3, but without the requirement to have this in any defined way. So, any vendor can chose arbitrary keys?

IMHO there is no good way to enforce "registered" keys (or key-prefixes) anyway, so not sure if it should even go into the standard. However, I believe it would make sense to share a list a keys (or key-prefixes) to avoid potential key-clashes.

@codefromthecrypt
Copy link

codefromthecrypt commented Jul 27, 2018 via email

@codefromthecrypt
Copy link

codefromthecrypt commented Jul 27, 2018 via email

@yurishkuro
Copy link
Member

@discostu105 why does the fact that (B) is a different tenant require changes to the propagated context? Your example of "importantData" is very vague, it's hard to draw any conclusions or generalizations.

@discostu105
Copy link
Member Author

@yurishkuro sure, let me try to clarify.

At Dynatrace, we're currently propagating more information than what's currently possible via traceparent. Some examples of fields we propagate:

  • agentId, tagId (these combined could arguably be merged into spanId)
  • tenantId (identifies the tenant/customer)
  • clusterId (identifies the backend)
  • serverId (id of the server-node inside the cluster that is supposed to correlate this trace)
  • ...

A more detailed description is here: https://docs.google.com/document/d/1O40xdY0pLVQG4ChlhSsfU7m6HRCH1GJHNhLLD_Ko8tg/edit

The point is, that we have information that we need to propagate that does not fit into traceparent. In order to do that over multiple tenants/clusters, we would need any of the above solutions.

@yurishkuro
Copy link
Member

and all that information needs to be present in the tracestate in case the control returns to the first tenant?

@discostu105
Copy link
Member Author

yes. otherwise we cannot (efficiently) correlate the two tiers of tenant A.

The goal is that tenant A sees a connection between hope (1.) -> (3.). The second hop (tenant B) should be transparent. Of course it's clear from tracestate that a "foreign" tenant was in-between and we could employ logic to indicate that in our analysis, but it's important for us that the (1.)->(3.) connection is there.

@SergeyKanzhelev SergeyKanzhelev added this to the 3. FPWD milestone Aug 6, 2018
@discostu105
Copy link
Member Author

After discussion in Seattle:

  • To make parsing easier, we came up with the idea to put the vendor name before the "=" sign and prefix it with a "starting-character" ("@").
  • examples:
    • <dynamicdata>@vendor1=<value>
    • fw5-29a-3039@dt=1;FEC354CD6;1;2;727c
  • Searching for @dt= would be more robust for parsing (searching the string), compared to the search for dt-. (or is it?)
  • There are no objections against using tracestate like this.
  • @AloisReitbauer suggested it could make sense to add this as a possible usage example of tracestate, in case other vendors have that same problem to solve.
  • The exact format of the tracestate-key does not need to be specified exactly. If vendors want to interop, they need to exchange their key format anyway to be able to call each other for querying spans.

@codefromthecrypt
Copy link

I don't know if really we can deduce there are no objections due to the lack of participation. I think only a few parties are present at the workshop.. just keep this in mind.

Widening the character set and suggesting a pattern seems a bit before the fact. Personally, I don't like adding @ as it is a distraction.

What is hard for me to understand now, is this.. currently you have one header right? How is it that you can use x-dynatrace yet not be able to use one state entry? Can you please show how what you are asking to do works now (with normal headers) and how that can't be done in one trace state?

@codefromthecrypt
Copy link

Ex it would seem you would have exactly the same problem today, perhaps I missed you calling it out, but how are you able to have multiple tenants today without clobbering and somehow you can't apply this to tracestate? Or is the point that you would like to today or what? I think we should really focus the use case before a: widening characters sets, and much more than encouraging a specific practice. Ex this whole idea seems experimental in how it is described, which is fine, just not sure we should be documenting things that might thrash in practice.

@discostu105
Copy link
Member Author

Adrian, you are right. Dynatrace currently uses just one header (X-dynatrace), and multi-tenant tracing currently does not work for us (we re-start the trace). This is why we are seeking a solution to this problem. Tracestate is a natural fit to this problem. As discussed earlier, this putting dynamic information into the key is not in violation of the spec, so this shall be the solution to go.

Allowing the @ character to tracestate key names is of course discussible. At the workshop in Seattle it was the solution we jointly came up with. There was general interest in the use-case and the solution for it, although no other vendor attempted this solution yet. There was consensus that the use-case is valid enough that this solution shall be documented as a possible usage of tracestate.

I also agree with you that on github it's sometimes not very transparent who actually signed off & agreed with something (a PR like this). Even more so, when participants were locally on-site and just verbally agreed.

@codefromthecrypt
Copy link

codefromthecrypt commented Aug 8, 2018 via email

@SergeyKanzhelev
Copy link
Member

this is explained now. closing the issue

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants