UpdatedInitalPR

[dawallin]

I have updated and exposed a mutable SpanContext in the Span together with Get/SetBaggage methods. I removed the out parameter on the Tracer.Extract and instead returns an ExtractResult, because out parameters does not work well with async programming in .Net.

I also updated documentation in the OpenTracing project.

The code still contains a BasicTracer that could be extracted to its own repository.

Any comments and suggestions are highly appreciated.

[cwe1ss]

Hi Daniel, it's unfortunate that we now have two separate versions - I didn't know you were still working on it and I thought you would get a notification anyway if you decided to work on it again.

We should try to join forces asap! IMO it would be best if someone ( @bensigelman , @yurishkuro ?) decides which PR is better suited and merge it. Afterwards we can do further changes as separate PRs on a common code base.

I would prefer my PR for the following reasons:

• it supports netstandard and runs on all .NET platforms
• ISpan interface:
  • this one has Finish and FinishWithOptions - this isn't required and untypical because C# has overloads
  • Furthermore, I don't think we need FinishOptions. Java doesn't have it and the only important property is the timestamp. In C# there's not really an advantage in constructing this object vs just calling Log(..) etc before calling finish. (why create an object if you can easily call the methods??)
  • SetTag open vs closed - Java has closed types, the rest is open. I'd prefer open for compatibility reasons.
  • Log vs. LogEvent - I don't think the LogData class is necessary because again, C# has overloads. creating a separate object is unnecessary IMO.
  • DateTime usage is wrong for sure. Although DateTimeOffset might not be good as well - maybe we need "long" (ticks) - but how would one know if he should put unix epochs or windows ticks in?

ISpanContext:

• forcing IReadOnlyDictionary might be troublesome - it's up to the trace implementation to decide what to do with it
• no get/set methods as defined in the spec

ITracer:

• I don't see the advantage of having StartSpanOptions and a builder concept. these are exclusive IMO in such a basic API.
• Extract: operationName, ExtractResult are not in the spec

The handling of formats/carriers must be discussed separately - I'm still not sure what the best thing to do is there.

[dawallin]

I agree that this merge conflict is unfortunate and that we should quickly select which version to continue from. For me it is fine to continue with your PR and then discuss the differences with mine and move my stuff into yours when appropriate.

Perfect that you upgraded for .Net Core. It was on my road-map but I have no experience and haven't had the time to played around with .Net Core. Couldn't even run your code in my version of Visual Studio but it forces me to upgrade :). Also a big improvement that you added the psake scripts.

Good that you fixed the HttpClient into HttpHeaders and also added the AspNetCode middleware.

The reason for an option object instead of method parameters I guess would be if you have many parameters. For Finish and Log I agree that the use of overloads is a better alternative.

Having an opened SetTag is also probably just fine, I first thought that just string would be enough. I don't really see the use of non-string tags, but maybe there is.

Changing to UTC is clearly a better alternative than local time. What would be the advantage of ticks ?

The ReadOnlyDictionary was used to force an immutable implementation. The use of IEnumerable<KeyValuePair<string,string>> is probably ok, you almost get the same result.

During the summer we had a long discussion about immutable ISpanContext. We decided that Get and Set baggage would be on the ISpan and ISpanContext would only have a GetAll method.

For the StartSpanOptions and the SpanBuilder I was inspired by the Java implementation (but removed the abstract and introduced it as an extension method instead). I agree that the API is very simple but I think it makes the creation of a new span even more fluent. Instead of

var options = new StartSpanOptions()
    .FollowsFrom(rootSpan)
    .SetStartTimestamp(new DateTime(2016, 8, 21, 14, 0, 0, DateTimeKind.Utc));
var subSpan2 = tracer.StartSpan("sub-operation", options)
    .SetTagHttpUrl("http://example.com/api");

you can do

var subSpan2 = tracer.BuildSpan("sub-operation")
    .FollowsFrom(rootSpan)
    .SetStartTimestamp(new DateTime(2016, 8, 21, 14, 0, 0, DateTimeKind.Utc))
    .StartSpan()
    .SetTagHttpUrl("http://example.com/api");

Since it's just extensions both version would work fine.

Agree that we can discuss the format carriers separately later. My biggest problem with this is the tight coupling between them. The carrier is already bound to a specific format and therefore the format parameter feels unnecessary. The extension methods makes the call easier but if you change carrier to one with a different format you also need to change the method call. This will otherwise result in runtime errors. I tried to solve this with the generic IInject and IExtract interfaces. You said it uglify the API for the end user, I'm not sure I agree as long as the type can be automatically determined (which is the case at least in my last PR). The user does not need to care.

About the formatting I think the key restriction has been lifted away from the TextMapFormat but if I understand correct should only be used in the HttpHeaderFormat.

The ExtractResult could also be discussed later. I copied a pattern from the Couchbase API where your calls return a result object with result, successFlag and maybe an exception. You can then decide what to do, or what you like to do if something went wrong. This would protect the end-user because he/she never have to catch an exception, opentracing will therefore never fail your application which I think is important.

[cwe1ss]

closed, because we merged #2