opentracing / opentracing-csharp

OpenTracing API for C# (.NET). 🛑 This library is DEPRECATED! https://github.com/opentracing/specification/issues/163
http://opentracing.io
Apache License 2.0
516 stars 73 forks source link

Implement "Trace Identifiers" RFC #96

Closed cwe1ss closed 6 years ago

cwe1ss commented 6 years ago

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 commented 6 years ago

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

cwe1ss commented 6 years ago

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

carlosalberto commented 6 years ago

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

cwe1ss commented 6 years ago

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!