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

Trace.CreateFromId ignores TraceIdHigh? #226

Open
jpotisch opened this issue Jan 15, 2019 · 5 comments
Open

Trace.CreateFromId ignores TraceIdHigh? #226

jpotisch opened this issue Jan 15, 2019 · 5 comments

Comments

@jpotisch
Copy link

jpotisch commented Jan 15, 2019

I apologize if I am just being dumb here but we were working on implementing Zipkin on our distributed platform and are finding that the 128-bit trace ID from a java app is being passed correctly to our .net app via the X-B3-TraceId header, but Zipkin4Net seems to only use the lower 64 bits as its own trace ID. This of course breaks our trace into two parts.

I looked at the source and I see that TraceId is used but TraceIdHigh is not:

private Trace(ITraceContext spanState)
        {
            CurrentSpan = spanState;
            CorrelationId = NumberUtils.LongToGuid(CurrentSpan.TraceId);
        }

I'm not familiar enough with Zipkin to attempt a PR on this but if I'm right (a big if!), that would explain what we observe, e.g. Zipkin4Net extracts 128-bit X-B3-TraceId value 0123456789ABCDEF but discards the first 64 bits and writes its own traces to 89ABCDEF.

If we're doing something wrong, of course please close this ticket and sorry for wasting your time, but any guidance on how to do it right would be much appreciated.

Thanks.

@jpotisch
Copy link
Author

I played around with this and found that if I modify Trace.cs as shown below, the incoming 128-bit X-B3-TraceId header gets correctly mapped to the trace CorrelationId:

BEFORE

// line 61
CorrelationId = NumberUtils.LongToGuid(traceId);

// line 77
CorrelationId = NumberUtils.LongToGuid(CurrentSpan.TraceId);

AFTER

//line 61
CorrelationId = Guid.Parse(CurrentSpan.TraceIdHigh.ToString("X") + CurrentSpan.TraceId.ToString("X"));

//line 77
CorrelationId = Guid.Parse(CurrentSpan.TraceIdHigh.ToString("X") + CurrentSpan.TraceId.ToString("X"));

Note that this is just very simple code to show the issue. I'm not suggesting this is the actual way to make the change! But it does seem to confirm my belief that the library does not correctly propagate 128-bit traces today.

@jpotisch
Copy link
Author

jpotisch commented Feb 7, 2019

@adriancole I am very sorry to bother you. I saw #230 so I know you're trying to solve the responsiveness issue on this project, and I know you don't owe me a thing. I am just hoping you might be able to at least point me in the right direction on this issue? If I've found a real bug and am able to come up with a fix and tests, I'm happy to submit a PR, but I need at least a little guidance at this extremely early stage of my understanding of the zipkin4net internals.

We are using Zipkin on our distributed platform (React client, two Java and one .NET backend services) and we are so close to having a single trace represent a request's entire lifecycle, but the .NET app's losing the high half of the incoming trace ID is keeping that goal tantalizingly out of reach, so I am highly motivated to volunteer to help come up with a fix.

So again, I apologize, and again, I'm happy to contribute a fix if this is in fact a bug and someone can give me a map to get started. (And again, if there is no bug and we're just doing something wrong, sorry!)

@codefromthecrypt
Copy link
Member

codefromthecrypt commented Feb 7, 2019 via email

@codefromthecrypt
Copy link
Member

codefromthecrypt commented Feb 7, 2019 via email

@jpotisch
Copy link
Author

jpotisch commented Feb 9, 2019

@adriancole Thank you so much. I really didn't know who to reach out to and saw your post so figured you were worth a shot. I already discovered how to break my quick proof-of-concept code, so I will see if I can get a little further along and then reach out to the gitter channel to see if I can make this PR-worthy.

And FYI guid is just a 128-bit data structure used a lot in Windows so it's a perfect fit for trace IDs.

Thanks again!

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

2 participants