improvements for PR #1

[cwe1ss]

Hi guys!
I came across this project yesterday and really like the idea, so I took Daniel's PR and tried to continue with it. That's what I've done:

General:

• The library now targets netstandard1.1, which means it runs on almost all .NET platforms (full .NET v4.5+, .NET Core, Mono, Xamarin, Windows Phone, Universal Windows Platform).
• I changed the project structure to better align with current repositories from Microsoft (e.g. ASP.NET Core) (src/test folder, .project.json, ...).
• psake based build script which runs the tests and creates the nuget packages.
• There's no documentation in code yet - I would add that if you want to proceed with it.

API:

• I tried to get as close to the specification as possible. I renamed stuff (e.g. "message" -> "eventName", ...)
• I removed the generics. Implementations will want to have typed versions of IScopeContext but IMO having the generics uglifies the API for end users and casting in the implementations is easy.
• I changed DateTime usages to DateTimeOffset and used UTC everywhere. DateTime.Now returns the local machine timezone and can't be used for such a library. DataTimeOffset has in implicit cast from DateTime so it also accepts them and handles timezones correctly.
• ISpan
  • removed GetTracer() - not in the spec and not necessary IMO
  • SetTag now accepts object as the value. There's no base class for numeric types in C# and having separate overloads for each type would break all implementations whenever a new type should be added. I think it's better to leave this open and let the implementation decide whether the type is OK or not. An alternative would be to limit it to struct types (a separate overload for string would still be required though).
  • Features that only extend the interface but don't require different implementations are moved into c# extension methods. This includes SetBaggageItem (which is a shortcut for Context.SetBaggageItem() and typed methods for all built in tags (e.g. SetTagComponent)
• ISpanContext is now a separate interface
• ITracer no longer has TryJoin - instead it has Extract as defined in the spec.
  • Extension methods for common use cases of StartSpan are in TracerExtensions.cs
• Propagation (Carriers/Formats):
  • IInjectCarrier and IExtractCarrier are now only marker interfaces and the different implementations are wrapper-objects around the carrier data structure (e.g. text-map, http-headers, byte array). Implementers have to decide based on this type how to inject/extract.
  • Every format defines its own extension methods for easier use (user doesn't have to manually wrap the carrier object)
• There's now a No-op tracer called NullTracer - Null* is more common in the .NET world than Noop*

OpenTracing.Carrier.HttpClient:

• This project is no longer required. The logic has been integrated into the OpenTracing package (Propagation\HttpHeadersCarrier.cs). I also changed the logic to no longer use the HttpClient as the carrier but instead the HttpHeaders dictionary (which is part of HttpRequestMessage and HttpResponseMessage). Using HttpClient.DefaultRequestHeaders was wrong because HttpClient instances are often shared between requests. Therefore, the headers must be set for every individual request.

OpenTracing.BasicTracer / OpenTracing.BasicTracer.Tests:

• I adjusted everything to the new API and made other changes as well - this is still pretty much work in progress.

Usage:

// get tracer (would be through dependency injection in real apps)
ITracer tracer = GetTracer();

// create root span
var rootSpan = tracer.StartSpan("get_account")
    .SetBaggageItem("user-id", "1234")
    .SetTag("key", "value")
    .SetTagComponent("AspNetCore"); // intellisense for known tags

// create HTTP api request
var request = new HttpRequestMessage(HttpMethod.Post, "/some_api");
request.Content = new StringContent("{ \"userId\": 1234 }");

// Inject - typed extension methods for known carriers
tracer.InjectHttpHeaders(rootSpan, request.Headers);

// call api
var response = await new HttpClient().SendAsync(request);

// Extract new context from response (this time with the raw method)
var spanContext = tracer.Extract(Formats.HttpHeaders, new HttpHeadersCarrier(response.Headers));

// create new span with reference
var subSpan1 = tracer.StartSpan("sub-operation", SpanReference.ChildOf(spanContext))
    .SetTag("custom-key", "custom-value");

// create another follows_from span with StartOptions
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");

// finish operations
subSpan1.Finish();
subSpan2.Finish(new FinishSpanOptions()
    .SetFinishTimestamp(new DateTime(2016, 8, 21, 14, 0, 5, DateTimeKind.Utc)));
rootSpan.Finish();

[bhs]

I guess there's no EMPTY_SET like in Java?

[cwe1ss]

There's Enumerable.Empty<Type>() for read-only lists (which returns a cached object), but there's no equivalent for dictionaries (maps) because they are mutable.

[bhs]

EOF for my comments... Basically looks really good overall, thanks again again.

[dawallin]

@bensigelman Is there anything from the comments above that we need to fix for this PR to be accepted?

[bhs]

@dawallin I don't feel strongly about whether it happens before or after merging this PR, but I do think we should remove API surface area around SpanBuilder and/or StartSpanOptions before encouraging people to create dependencies on this code.

On that note, here are pointers to how the startSpan calling code looks in other languages:

Java: https://github.com/opentracing/opentracing-java/blob/master/opentracing-api/src/main/java/io/opentracing/Tracer.java#L33

Go: https://github.com/opentracing/opentracing-go/blob/master/tracer.go#L30

Python: https://github.com/opentracing/opentracing-python/blob/master/opentracing/tracer.py#L58

Javascript: https://github.com/opentracing/opentracing-javascript/blob/master/src/tracer.js#L33

It's important that the calling code has an easy way to adjust zero or more of the startSpan params without specifying all of them, and in that sense I think the SpanBuilder might actually be the part of the API we'd want to keep.

Getting back to your question: I'd say "let's merge this", but please file issues about ^^^ as well as the fate of the helper/example implementations and so on.

It would be good to see if @yurishkuro has any blocking concerns, too.

[yurishkuro]

It would be good to see if @yurishkuro has any blocking concerns, too.

Nope, since this isn't final.

[cwe1ss]

I added SetOperationName(string operationName) to ISpan. I will create separate issues for the other topics!

[yurishkuro]

is byte array the right carrier for binary? I assume byte array has a fixed length, so especially for the injector how does the instrumentation know how long the tracing context in binary encoding will be? I don't know the native IO types in C#, but I would assume some sort of a byte buffer or i/o stream to be a better carrier than byte array.

[cwe1ss]

I'll look this up. Probably not.

[cwe1ss]

As of e9c8ae8, the format/carrier handling is now almost equal to the Java version.

• Format is typed and there is a static accessor class called Formats which has Formats.TextMap, Formats.HttpHeaders, etc.
  • The Format class has a Name property which is the actual factor for equality. I really don't like the Java version which only compares the actual object address.
• IInjectCarrier, IExtractCarrier is no longer required (Formats.Binary accepts a regular byte[] )
• I still have all the extension methods. I still really like them because IMO they make the API a lot easier to use for users.

I'm still not sure whether this is better or worse than our previous version 😄 but I guess it's more important to be equal to Java and solve any possible issues together when they arise.

One thing I'm worried about is when we have a case where a format should accept multiple incompatible carriers. E.g. now I use ITextMap for Formats.HttpHeaders - but as I've said, in .NET, http headers are typically represented as multi-value maps. We can now no longer define that .NET based trace implementations should be able to handle both versions (which would be better for performance most probably). We would have to introduce a new format instead which would be confusing for users.

Would be great if someone could merge this now. 😄 You could also give me contributor rights to this repository if you want - I would not do any non-trivial commits without a PR/your approval.

[dawallin]

I still have some problems with the format/carrier problem, but I will address that once this PR is accepted.
Basically I think format and carrier are totally connected and therefore you only need one parameter, the other one is just over specification. What the generics do here is just forcing you that they are correct at compile time. I have a proposal for a different solution in my PR #3, I will try to move that concept to this branch once this is merged.

[dawallin]

We have now added some issues and made some changes according to the major comments. @bensigelman Is there anything more before you can accepting this PR ?

[bhs]

@dawallin nope! I will just give you both write privs in this repo, you certainly have more C# knowledge than I.

[bhs]

(done)

[cwe1ss]

awesome - thanks!