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

0.11 Release plan #55

Closed cwe1ss closed 6 years ago

cwe1ss commented 6 years ago

This is a meta issue to track/discuss what should be done in the next release. The tasks reflect my proposal, any feedback/help is more than welcome!

Parity with opentracing-api from Java

The main thing that's missing right now is the support for scopes. This will only add new types/members but it will still be a breaking change (for implementations) since it adds members to an existing interface (ITracer).

Apart from that, the C# version currently has some other differences. We want to be as close to the Java version as possible so we will only keep differences that make sense for C#/.NET. We will do this with #50 which basically replaces the whole codebase and is based on the current master of opentracing-java.

This PR will introduce the following changes to the current code:

Changing these things obviously results in a ton of breaking changes but I guess the impact is still minimal and the benefit of being on par with Java outweighs it.

We will keep the following differences. Please comment if you want to remove any of these differences as well!

Tasks:

Enhancements / Additions

We don't yet have an equivalent for Java's additional packages "util", "mock" and "noop". We will implement all of these in one OpenTracing.Utils package to reduce the number of packages.

Tasks:

Making sure stuff works

We only have a few unit tests as most of the code is interfaces. However we definitely need to port the tests from Java's util-package and we need to write our own for our AsyncLocal implementation.

We should also confirm that the API can actually be used by using it somewhere. I don't know if the basictracer-packages are still used in other languages. If so, we should update basictracer-csharp. Otherwise I have some personal prototypes. Other projects that would be willing to test stuff would be appreciated.

Tasks:

Infrastructure

Schedule

I've got some time next week, so I'd like to have PRs and discussions for the core API as far as possible by the end of next week. I can also create the utils-project with implementations for GlobalTracer and scopes. So depending on your feedback and the timeframe of other people who want to contribute, we might want to target a release between 02/15 and 02/28?!

cwe1ss commented 6 years ago

Added an "Infrastructure"-block.

cwe1ss commented 6 years ago

I rewrote the issue. It's now a proper plan as it says what will be changed and what will be kept.

This still reflects my proposal so please add your comments if you have any feedback!!!

ndrwrbgs commented 6 years ago

LGTM. I can start working on some of the boxes after #50 is in. [nit] SpanContextCorruptedException doesn't exist

I'd also add details regarding semantics.md, unless java has those implemented in it's Utils project in which it's covered (I just know my local copy I introduced methods for them).

cwe1ss commented 6 years ago

I'd also add details regarding semantics.md, unless java has those implemented in it's Utils project in which it's covered (I just know my local copy I introduced methods for them).

What do you mean by that?

ndrwrbgs commented 6 years ago

Semantic extensions on top of the specification

https://github.com/opentracing/specification/blob/master/semantic_conventions.md

E.g.

    public static class KnownTagNames
    {
        public const string Component = "component";

        public const string Error = "error";

        public static class Peer
        {
            public const string Port = "peer.port";
            public const string Address = "peer.address";
            public const string Hostname = "peer.hostname";
            public const string Ipv4 = "peer.ipv4";
            public const string Ipv6 = "peer.ipv6";
            public const string Service = "peer.service";
        }

        public static class Db
        {
            public const string Instance = "db.instance";
            public const string Statement = "db.statement";
            public const string Type = "db.type";
            public const string User = "db.user";
        }

        public static class Http
        {
            public const string Method = "http.method";
            public const string Url = "http.url";
            public const string StatusCode = "http.status_code";
        }

        public static class MessageBus
        {
            public const string Destination = "message_bus.destination";
        }

        public static class Span
        {
            public const string Kind = "span.kind";
        }

        public static class Sampling
        {
            public const string Priority = "sampling.priority";
        }

and structs like this for reporting those values easily

    public struct PeerData
    {
        public string Address { get; }
        public string HostName { get; }
        public string Ipv4 { get; }
        public string Ipv6 { get; }
        public int? Port { get; }
        public string Service { get; }

        public PeerData(string address, string hostName, string ipv4, string ipv6, int? port, string service)
        {
            this.Address = address;
            this.HostName = hostName;
            this.Ipv4 = ipv4;
            this.Ipv6 = ipv6;
            this.Port = port;
            this.Service = service;
        }
    }
carlosalberto commented 6 years ago

Other projects that would be willing to test stuff would be appreciated.

For the Span propagation part, we extracted and created a set of examples for trying out the approaches. All these examples are under the java-examples directory under opentracing-java, and I think we could take that approach, for a start (whenever we get to the point of implementing the IScopeManager ;) )

carlosalberto commented 6 years ago

We don't yet have an equivalent for Java's additional packages "util", "mock" and "noop". We will implement all of these in one OpenTracing.Utils package to reduce the number of packages.

I'm wondering about the motivation for this. The advantage over having different assemblies/packages, is that one of keeping resemblance between C# and Java (after all, they are kinda siblings). Guessing it's because of how packages are usually organized in the Nugget gallery?

Besides that, what I'm also curious about is the namespace organization - I think keeping similar namespaces to the Java packages would be good: i.e. OpenTracing.Mock, OpenTracing.NullTracer (or Noop), and OpenTracing.Util. Maybe that's altogether the plan already, but wanted to double check ;)

(I understand that usually C#/.Net has moved into having a few namespaces with lots of members, as opposed to Java and its multiple, sometimes deep nested packages with a few classes each, but in this case I keeping a similar structure with the Java API would be a good thing).

cwe1ss commented 6 years ago

@ndrwrbgs Semantic extensions on top of the specification

As it doesn't exist in Java (or not that I know of) I'd say we look into this after this release and discuss it with the Java folks. Ok?

cwe1ss commented 6 years ago

@carlosalberto: I'm wondering about the motivation for this. The advantage over having different assemblies/packages, is that one of keeping resemblance between C# and Java (after all, they are kinda siblings). Guessing it's because of how packages are usually organized in the Nugget gallery?

I'm not a fan of multiple packages if they don't have a clear architectural reason and I do not really see the reason to e.g. have different packages for Noop and Util as they will probably always be used together (Util has the scope stuff and GlobalTracer; and GlobalTracer depends on Noop). It's a lot easier for developers to discover new types within one package/assembly than it is to realize that one needs another package/assembly.

As none of these types (mock, util, noop) currently contain dependencies outside of the .NET standard library, I personally would even go as far as to include all of it as regular subfolders in the main OpenTracing assembly. Sure, the "OpenTracing" package is meant to be "the" abstraction library between instrumentation and implementation and a change to a util-class that is only necessary for instrumentation should not affect the implementation side but this would mean that packages would have different version numbers (e.g. "OpenTracing.Util" would be released more often than "OpenTracing") but that only leads to a version hell and makes it really hard for users/developers to know which packages work with each other. (e.g. OpenTracing.Util v0.14.0 depends on OpenTracing v0.11.0 and OpenTracing.Util v0.15.0 depends on OpenTracing v0.12.0, ...)

Anyway, as I've said, the proposal reflects my opinion (one has to start somewhere) and I'm more than happy to discuss this and to change the proposal!

So I see three possibilities:

I would be OK with whatever the majority decides but given the fact that there's still only <= 5 active people here, I'd say that it's not really useful to decide based on votes/opinions. So I'd also be ok to just follow the "do whatever Java does" approach until the user base here is big enough to be able to better decide on whether or not a deviation from Java is actually useful/necessary. Therefore we could just go with separate assemblies!?

Thoughts?

carlosalberto commented 6 years ago

Hey Christian!

Thanks for jumping in :)

Disadvantage: Harder to use; version hell if they are released separately

Yeah, that needs some care - and that's why, even in the Java ecosystem, where each package has it's own artifact in Maven, they are all in the same repository, to prevent them from being released separately (while keeping their dependencies 'isolated', specially between the main api and noop/mock/util).

After a second look, I'd suggest a separated package that contains all the extra stuff (as you originally proposed), but without the Opentracing.Util. prefix for noop/mock (as that will create some de-alignment between Java and C# APIS, even if only mentally ;) ) Maybe for that to happen, this second package would need to be called something other than Util (Extra? Support?)

Else, as you also suggested, follow the Java path and deviate eventually as we go... (but still not sure, of course)

I would be OK with whatever the majority decides but given the fact that there's still only <= 5 active people here...

One of the things about being at the very beginning of a project :) Nevertheless, it's good the C# land has started to get some traction, little by little (finally ;) )

gideonkorir commented 6 years ago

@cwe1ss my vote is for 2 separate packages so long as they have the same version number (released simultaneously). As an implementer I only take a dependency on OpenTracing package but as a client/user I can decide to take a dependency on OpenTracing.Extras if I choose to.

@carlosalberto I agree with you on the util prefix plus the word util is confusing, is it a utility for using OpenTracing? something like Extras makes it known that it was built on top of the OpenTracing spec

If possible, I'd have the GlobalTracer in the OpenTracing namespace but still in the Extras package. This removes the need to keep typing in 2 usings (I know CTRL + . helps alleviate some of the pain here but still)

cwe1ss commented 6 years ago

I do not really like the names Extra(s), Support. I've never seen a popular package with a similar name on NuGet. Microsoft is using the term "Extensions" for their new "Microsoft.Extensions.*" packages which might sound better (at least to me)?!

Another thing to consider is that there will be OpenTracing.Contrib.* packages so having a OpenTracing.[Extras|Support|Extensions] package and e.g. a OpenTracing.Contrib package might also be confusing as one wouldn't know what contains what.

Since we don't have a proper name and since the namespaces should be the same as in Java, I now think that having a mixed package might be the worst option for now.

I would still prefer to have everything in one "OpenTracing" package as all of the code only uses BCL types and as that's also what other opentracing packages do (e.g. Go, PHP). This would also keep the same namespaces as the modules (Noop, Mock) would just be subfolders.

Thoughts? I hope I'll be available for the upcoming first cross-language working group meeting next week. If so, I'll discuss this there as well.

cwe1ss commented 6 years ago

BTW, the current release already has a Null/Noop tracer and it is part of the one „OpenTracing“ assembly.

gideonkorir commented 6 years ago

@cwe1ss just search Extras on nuget.org should return a few packages (Autofac has a lot of those).

Given that we only use BCL types and that we will always have to release in lock step, I change my mind - single Assembly makes most sense.

cwe1ss commented 6 years ago

I took a closer look at our exception types as they don't exist in Java. I created PR #64 with more details and updated this issue description. The PR would remove these types. Please post any comments in the PR.

carlosalberto commented 6 years ago

Since we don't have a proper name and since the namespaces should be the same as in Java, I now think that having a mixed package might be the worst option for now.

I agree on that, at least for now. I think one package could work just fine, as upon further inspection, it seems packages in Nuget do not get so 'split' as compared to Maven/Java.

grounded042 commented 6 years ago

I've been tracking this project for a little while as the company I work for is looking to add tracing into our system. We're currently building out a proof of concept using version 0.10 of this lib. I'd be happy to help work towards a releasable 0.11. In the top-most post there are some work items - is there a specific one that isn't being worked on that would be good for me to start on? Or is there another area I could help?

cwe1ss commented 6 years ago

Everything has been merged so we are pretty close to a release IMO. I compared the code with Java once again and created a summary of everything that is different here (I hope I didn't miss anything):

General:

Core API:

Util:

Noop:

Mock:

I'll give people a few more days to provide feedback. If there's no objections, I'll release this beginning of next week!

In the meantime, I'm working on my ASP.NET Core integration to make sure we have a working example at the time of release.

/cc @bhs @tedsuo @yurishkuro

MikeGoldsmith commented 6 years ago

Looks good to me. Good work @cwe1ss @ndrwrbgs @Aaronontheweb

Aaronontheweb commented 6 years ago

One nitpick:

We use DateTimeOffset instead of long ticks

This is one change I'm not a fan of mostly because the native DateTime and DateTimeOffset are expensive and error-prone to serialize natively. For best results, you usually end up serializing them into their ticks representations anyway. I'd prefer that to be the native exposed format.

cwe1ss commented 6 years ago

@Aaronontheweb How would one get the ticks?

Aaronontheweb commented 6 years ago

https://github.com/akkadotnet/akka.net/blob/3f924fda8d6ca55c5af90a710d23be8ce36d5c7a/src/core/Akka/Util/MonotonicClock.cs

Seed the initial ticks value with DateTimeOffset.Now (instead of setting it to 0 like we do above) and then just add the delta provided by the Stopwatch running in the background.

edit: Or DateTimeOffset.UtcNow

Aaronontheweb commented 6 years ago

Like I said though, it's a nitpick.

cwe1ss commented 6 years ago

It's important to note that the overloads with a timestamp are mostly for replaying existing spans (e.g. from a log file) where people have a specific timestamp in the past (vs the current timestamp) so they have to parse/get the DateTime from somewhere anyway. So in this case it's all about usability.

Regular instrumentation will call the overloads without timestamps so tracers could optimize that path by using whatever mechanism they want to get the current timestamp.

cwe1ss commented 6 years ago

PS: Note that Stopwatch runs out of sync a little bit, that's why e.g. .NET corefx' Activity type synchronizes it every 2 hours. In .NET Core, DateTime uses high precision out of the box so they're no longer using Stopwatch there.

grounded042 commented 6 years ago

I've got a working example over at https://github.com/Chatham/LetsTrace. There is an opentracing_11 branch which has the latest code from here integrated in. The library as a whole is still new and needs some good feedback, but I've tested it locally using the example.

carlosalberto commented 6 years ago

@cwe1ss I started porting (locally) the Java examples (from opentracing-java/opentracing-examples), and I expect to at least have a pair available prior to the release. Should be creating a PR over the next days ;)

cwe1ss commented 6 years ago

@carlosalberto that would be awesome!

cwe1ss commented 6 years ago

I've just released v0.11.0. I thought about creating a RC first but I don't think there's much point in it for this release as there's very few users anyway. We'll obviously change this in the upcoming releases.

Release notes: https://github.com/opentracing/opentracing-csharp/releases/tag/0.11.0

Please create a new issues with any problems/feedback.

Thanks everyone!

bhs commented 6 years ago

(@cwe1ss: congrats!!)