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

Remove "hexadecimal" restriction #16

Closed
yurishkuro opened this issue Sep 22, 2017 · 22 comments
Closed

Remove "hexadecimal" restriction #16

yurishkuro opened this issue Sep 22, 2017 · 22 comments
Milestone

Comments

@yurishkuro
Copy link
Member

This has been discussed in the original PR (starting from #1 (comment)) but got lost/ignored. In the current form the spec excludes tracing systems that may be using differently formatted strings for trace and/or span IDs.

@bogdandrutu
Copy link
Contributor

Couple of things:

  • I would say base16 not "hexadecimal". "hexadecimal" means you encode a number.
  • You need to respect HTTP restrictions so you have to encode the "string" or "byte array" or however you call it. So we have to define what is that encoding because otherwise you put a restriction on the traceId to be HTTP compatible.

@yurishkuro
Copy link
Member Author

because otherwise you put a restriction on the traceId to be HTTP compatible.

well, if we're talking about HTTP header spec, isn't that implied?

I would say base16 not "hexadecimal". "hexadecimal" means you encode a number.

https://www.google.com/search?q=define+hexadecimal says "relating to or using a system of numerical notation that has 16 rather than 10 as its base."

The point is, the current spec exclude tracing systems that might choose a different encoding or length of the trace ID. Adrian had an argument about storage implication, which is valid in the sense that yes there are implications, but it's not impossible to resolve. In the best case the two systems use "compatible" trace-ids and can use them directly. In the worse case the receiving instrumentation will record the incoming trace-id as a correlation id.

@codefromthecrypt
Copy link

codefromthecrypt commented Sep 23, 2017 via email

@wu-sheng
Copy link
Contributor

+1 about removing the base16 and length things. Skywalking is using three longs for ID(trace segment id and trace id). So length changes for every generated id. If we follow the spec, we must cost more CPU and memory for the ID. This is not good.

Right now, skywalking can only consider to add a new HEAD for this spec if some people want this interop feature. Put two heads, one for skywalking, one for TraceContext Spec. It can work, but it is the best solution. :)

But still, I hope the spec can make the skywalking supporting easier.

@wu-sheng
Copy link
Contributor

@adriancole Right now, skywalking is considering put the ids(traceId and spanId) from outer tracer into tags of the EntrySpan.

@codefromthecrypt
Copy link

codefromthecrypt commented Sep 23, 2017 via email

@codefromthecrypt
Copy link

codefromthecrypt commented Sep 23, 2017 via email

@yurishkuro
Copy link
Member Author

Hi Adrian,

What format does jaeger require that is not hexidecimal? What would happen if it were not? How would you handle it in jaeger?

I am not saying that general strings are required or even supported by Jaeger. Jaeger is using 128bit byte array as a trace ID, and cannot support arbitrary string IDs without substantial changes to the backend and client libs. The way I would handle incoming string ID is by recording it as a correlation ID tag.

But it's not about Jaeger, it's about inclusiveness of the spec. Maybe we should start with the goals of this spec, what exactly it is trying to achieve. There are different possible interop modes.

@codefromthecrypt
Copy link

codefromthecrypt commented Sep 23, 2017 via email

@yurishkuro
Copy link
Member Author

can we define what the charter is? it's not stated anywhere in the repository, hence the confusion.

@codefromthecrypt
Copy link

codefromthecrypt commented Sep 23, 2017 via email

@yurishkuro
Copy link
Member Author

yurishkuro commented Sep 23, 2017

Let's discuss the charter in #17

@codefromthecrypt
Copy link

codefromthecrypt commented Sep 23, 2017 via email

@wu-sheng
Copy link
Contributor

Hi, sheng. do you need to remove both constraints or just one (length)?

Skywalking id is long1.long2.long3. The dot is not part of base16 and long is not the certain length literally(After encoding, sure, it can be). So if the both constraints removed, skywalking will be no need to add new head, just need to separate the current sw3-head into two parts. This is my best choice.

And as you known, skywalking has a way to resolve this: support this by using a new head for interop and do not change the sw3-head. In that way, cost of encoding and size of http package will change, right? Maybe not big, but for a very high throughputs app, like some 10K tps application in Chinese e-commerce and telecom systems (large population -> large system...), everything changed, and I have to treat all perf related risks very carefully. Btw, these users have such high throughputs, but can't accept sampling... So you can image what is the situation skywalking facing......

@codefromthecrypt
Copy link

codefromthecrypt commented Sep 23, 2017 via email

@wu-sheng
Copy link
Contributor

What would you propagate in order to continue that trace?

Besides the context spec, which your guys used, I can introduce two use cases of many tracing system do to propagate the context.

First thing first, the reason of context includes more fieldsthan yours, is not about continue trace, they're about analysis, e.g the application relationship, the service relationship. And you can see if we run the analysis after all trace segments(spans) finished, it's clearly no need for these extra fields, but the analysis latency is very long, and cost more resources in read/write or memory at the same time.

And most of tracing systems use UUID or timestamp-trusted-traceid(skywalking is a kine of implementation).

So this is user case <1>, we propagate parent application code(id), parent service(operation) name etc.

And many tracing system implementation, they use the same span.id style, I called it EagleEye span-id style. EagleEye is an alibaba OSS system, they shared their implementations, so many people use it. The id is like these: 1.0, 1.1, 1.1.0, you can understand easily, 1.1.0-span is the first child of 1.1-span.

That is user case <2>.

@adriancole So, you can see, in order to fit this spec, they will do a lot of works. Most of them are about encoding and length...

@codefromthecrypt
Copy link

codefromthecrypt commented Sep 23, 2017 via email

@wu-sheng
Copy link
Contributor

IMO, I think for a correlation id, included trace-id and span-id of course, skywalking saved these two keys(only care max length, not use a separated key, and ascii encoding), generate a new trace. In fact even for sw3-head, we generate a new one too. The difference between these two is,

  1. for sw3, generate a trace-ref;
  2. for other system, generate correlation(tags) values.

User can query trace by correlation trace-id through skywalkingUI. And the can see the correlation values if this trace has correlation ids.

And for more, based on this spec, I hope all participated tracing systems, can provide a standard query condition, e.g. http:/xxx:xx/zipkin/trace?tid=ttttt&spanId=sssss. So I submit an issue about vendor id. After have that, the tracing system can config the mapping from vendor to url(e.g. http:/xxx:xx/zipkin/trace for zipkin) ref #14 .

This can really help users to take benefits from tracing system interop. I think that matters.

@codefromthecrypt
Copy link

codefromthecrypt commented Sep 23, 2017 via email

@yurishkuro
Copy link
Member Author

I want to clarify what this issue was about. One aspect of it was about variable-length IDs, which is currently discussed in @bhs's PR #19. The other aspect was about the encoding. For example, suppose some tracing system generates trace-ids in the form {serviceName}-{randomBytes} (@wu-sheng mentioned something like that, only with numbers). We could support it if we allowed arbitrary strings, but there are two problems

  1. we'd still need to talk about some encoding since we might want to reserve - as a trace/span id separator
  2. such trace-id scheme would hardly work if the system that uses it was on the receiving side, where it might get an opaque hexadecimal string

Does that mean that the only acceptable un-encoded representation of a trace-id is opaque byte array? I.e. no system receiving Trace-Context header should attempt to interpret the parts of that byte array since it has no guarantee it's a value compatible with its internal semantics (like two longs in Skywalking).

@bogdandrutu
Copy link
Contributor

@yurishkuro I think you are talking in the same issue about completely different problems. I am not saying that they not need to be discussed but the initial issue was about removing "hexadecimal" restriction.

@mtwo mtwo added this to the Release 0.1.0 milestone Oct 21, 2017
@yurishkuro
Copy link
Member Author

Nobody seems to object to the existing fix-length hex encoding, so closing this.

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

5 participants