-
Notifications
You must be signed in to change notification settings - Fork 87
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
Comments
I played around with this and found that if I modify Trace.cs as shown below, the incoming 128-bit 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. |
@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!) |
hi. I haven't been active on this project because I can barely compile
csharp. I was pulled to another issue due to it being cross posted and
sadly it declined from there. in other words I wasnt intentionally ignoring
your issue.. I haven't been strictly attentive to the repo. personally I
help on many many repos in or outside our org when asked and usually the
help is received without drama. in other words I am happy to advise just i
cant code effectively for one reason i am near clueless in csharp and two
my internet is busted so I can only thumb advise till fixed. the fact that
you offer help is awesome as that basically makes progress possible. in
other words thank you for the offer.
all that said I will read the technical stuff and try to get back to you
either busting thumbs or after my caffeine level drops to where I can
starbucks again ;)
Meanwhile, and not as a punt, but personally I am probably not the only
person who can answer this question. we have a team and sometimes people
always ask me directly. that is fine but fragile. for future reference we
have a gitter channel openzipkin/zipkin where others who similarly dont
actively watch this repo or know csharp, but do know zipkin, live.
anyway I will reply in a bit. thanks
…On Thu, Feb 7, 2019, 1:05 PM Joel Potischman ***@***.*** wrote:
@adriancole <https://github.com/adriancole> I am very sorry to bother
you. I saw #230 <#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 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!)
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#226 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAD616YswxopOs2ROAt9TgKMcV6EeQjiks5vK7QqgaJpZM4aALjr>
.
|
as far as I can tell your code looks like it is correct. I dont know the
nature of Guid but the end goal is to parse and generate 128 bit 32 char
lowerhex trace id strings.
(there is b3-propagation repo.. main thing is high bits are to the left as
you mention)
I think if you raise a pull request and cover both generating and parsing
this data (ideally also proving it is sent through process if that is
somehow unlikely to work) I can do a concept review even not knowing
csharp. I have reviewed code like this in many languages I dont know for
better or for worse ;)
…On Wed, Jan 16, 2019, 1:51 AM Joel Potischman ***@***.*** wrote:
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 61CorrelationId = NumberUtils.LongToGuid(traceId);
// line 77CorrelationId = NumberUtils.LongToGuid(CurrentSpan.TraceId);
*AFTER*
//line 61CorrelationId = Guid.Parse(CurrentSpan.TraceIdHigh.ToString("X") + CurrentSpan.TraceId.ToString("X"));
//line 77CorrelationId = 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.
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#226 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAD6164iamGCGilDVns_13bUAGHnvx0Iks5vDhUTgaJpZM4aALjr>
.
|
@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! |
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:
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 to89ABCDEF
.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.
The text was updated successfully, but these errors were encountered: