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

Overriding Trace ID #1240

Closed
lupengamzn opened this issue Sep 8, 2020 · 5 comments
Closed

Overriding Trace ID #1240

lupengamzn opened this issue Sep 8, 2020 · 5 comments
Labels
question Further information is requested

Comments

@lupengamzn
Copy link
Contributor

Question

Describe your environment.

As AWS X-Ray adopts trace ID with timestamp, we need to override OT's original random trace ID when writing the propagator. The only exposed API allow us to do so is through SetParentId. However, when we apply this API to a root activity, its trace ID will be overridden but its parent ID will no longer be null, which will fail the sampling checker as a root activity in ActivitySourceAdapter when there is no sampler set in advance.

Before

RootActivity.ParentId = null

After call SetParentId(traceIdWithTimestamp, default, ActivityTraceFlags.None), root activity's trace ID will be overridden as traceIdWithTimestamp and will have a parent ID in the format below:

RootActivity.ParentId = "00-traceid with timestamp-0000000000000000-00"

What are you trying to achieve?

Even through the root activity described above has a ParentId, it indeed has no actual parent and should be categorized as other regular root activity that has a ParentId as null.

We're wondering if it's possible that you could take this case into consideration in rewriting the sampling checker when you're improving ActivitySourceAdapter as mentioned in #1210.

Similar as (or other alternative methods)

if (string.IsNullOrEmpty(activity.ParentId) || activity.ParentSpanId.ToHexString() == "0000000000000000") // added this case into consideration
{
       parentContext = default;
}
else if (activity.Parent != null)
{
       parentContext = activity.Parent.Context;
}
else
{
       parentContext = new ActivityContext(
       activity.TraceId,
       activity.ParentSpanId,
       activity.ActivityTraceFlags,
       activity.TraceStateString,
       isRemote: true);
}
@lupengamzn lupengamzn added the question Further information is requested label Sep 8, 2020
@lupengamzn
Copy link
Contributor Author

cc/ @anuraaga @srprash

@cijothomas
Copy link
Member

linking to similar discussion from runtime - dotnet/runtime#42786

@anuraaga
Copy link

anuraaga commented Oct 1, 2020

Is it even conceivable for OTel to continue to use the Activity API? That has not been developed based on our spec I think - it would be crazy if it was given how unstable our spec has been. Our spec is finally nearing stability but .NET has a different roadmap probably. It makes me wonder how we can manage this seeming disconnect in roadmaps. Really what we need is a story of .net migrating to OTel rather than the opposite maybe. Sorry this isn't a concrete idea but a thought given the confusion I see about the tying of the two projects.

@cijothomas
Copy link
Member

Is it even conceivable for OTel to continue to use the Activity API? That has not been developed based on our spec I think - it would be crazy if it was given how unstable our spec has been. Our spec is finally nearing stability but .NET has a different roadmap probably. It makes me wonder how we can manage this seeming disconnect in roadmaps. Really what we need is a story of .net migrating to OTel rather than the opposite maybe. Sorry this isn't a concrete idea but a thought given the confusion I see about the tying of the two projects.

Yes. This repo was initially written outside of Activity API. Then the maintainers here and .NET runtime team collaborated, to add more capabilities to Activity API, and decided to implement OTel on top of the Activity API. We can have more discussions on this - either in the SIG meetings, or here.

@cijothomas
Copy link
Member

dotnet/runtime#46704 TraceId generation can be overridden now, so closing this isssue.

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

No branches or pull requests

3 participants