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

Implement "Trace Identifiers" RFC #96

Merged
merged 1 commit into from
May 19, 2018
Merged

Conversation

cwe1ss
Copy link
Member

@cwe1ss cwe1ss commented May 1, 2018

This implements the new "Trace Identifiers RFC" by adding TraceId & SpanId members to ISpanContext.

The interesting thing to discuss here is the naming collision with existing tracer properties - e.g. MockTracer already had properties with the same names.

In this PR I changed the MockTracer properties to string but C# also has the possibility to "explicitly implement an interface" which would make it possible to keep the properties as long. This would look like this:

public sealed class MockSpanContext : ISpanContext
{
    // The tracer has its own TraceId property with whatever type it wants
    public long TraceId { get; }
    
    // Whenever someone works with the `ISpanContext` interface, 
    // he can still access the `string` version of TraceId.
    string ISpanContext.TraceId => TraceId.ToString(CultureInfo.InvariantCulture);
}

// Usage examples:

// Type is string because we have ISpanContext.
ISpanContext context = span.Context;
string traceId = context.TraceId;

// Type is long because we use the concrete type.
MockSpanContext context = (MockSpanContext)span.Context;
long traceId = context.TraceId;

Obviously, this is a bit confusing as it's quite easy to get the "wrong" type depending on what one is doing.

As MockTracer is used for unit tests where checking these properties is often necessary, I thought that changing the types to string makes this much less confusing.

For a real tracer, just explicitly implementing the interface and keeping the original types for its properties might be the best option though.

@cwe1ss cwe1ss added enhancement breaks-tracers A breaking change for tracers labels May 1, 2018
@cwe1ss cwe1ss requested review from tedsuo and yurishkuro May 1, 2018 07:31
Copy link
Member

@yurishkuro yurishkuro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 - nice to know that C# has this ability. I expect it to be a lot harder in the other languages.

@cwe1ss cwe1ss added this to the 0.12.0 milestone May 1, 2018
@cwe1ss
Copy link
Member Author

cwe1ss commented May 8, 2018

@opentracing/opentracing-c-maintainers feedback/approval please so that we can include this in the next release.

@cwe1ss
Copy link
Member Author

cwe1ss commented May 14, 2018

I'll merge this on/after Friday (May 18th) if there's no objections.

@carlosalberto
Copy link
Contributor

@cwe1ss This change looks great to me (definitely the explicit interface impl bit is a handy thing to have here).

@cwe1ss
Copy link
Member Author

cwe1ss commented May 19, 2018

I'll merge this now. I want to do an RC-release anyway so if there's any concerns, feel free to create a new issue!

@cwe1ss cwe1ss merged commit 492c78a into master May 19, 2018
@cwe1ss cwe1ss deleted the cweiss/TraceIdentifiers branch May 19, 2018 20:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaks-tracers A breaking change for tracers enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants