C# client library

Question

C# client library

Closed this issue 6 years ago · 50 comments

Placeholder issue to create a C# / dotNET client library.

E.g. https://github.com/gideonkorir/jaeger4net by @gideonkorir

Answer 1 · 2017-12-02T17:42:23.000Z

@yurishkuro I haven't given the library some love as we decided to use zipkin given thrift issues but I can finish the remaining integration (honestly that's calling into the jaeger agent) any help welcome

Answer 2 · 2018-02-21T20:46:36.000Z

@yurishkuro I've got a library we are working on that supports Jaeger. Still in early development, but coming along well: https://github.com/Chatham/LetsTrace

Answer 3 · 2018-02-21T21:22:09.000Z

@grounded042 great! Are you basing it on the work already done by @gideonkorir ?

Answer 4 · 2018-02-21T21:22:46.000Z

we decided to use zipkin given thrift issues

@gideonkorir could you elaborate on the thrift issues, and how they are addressed in Zipkin (which also uses thrift, typically)?

Answer 5 · 2018-02-22T13:11:20.000Z

@yurishkuro I took inspiration from the work @gideonkorir had done as well as the Java and Go Jaeger clients. Not fully featured yet as it's pretty basic - but we're working on sampling right now and making sure it comes up to speed with other clients.

Also, I think I might have run into the same issues that @gideonkorir did with Thrift. The main problem is finding a NuGet package with the official Thrift code in it. What I did to get around that for the time being was to pull Thrift into my repo which might not be the best idea as it's not the standard way of doing things for .Net.

Answer 6 · 2018-02-28T13:12:49.000Z

We broke out the Jaeger portion into https://github.com/Chatham/LetsTrace.Jaeger. It's a separate package that you pull into your project along with LetsTrace in order to work with Jaeger. We're working on documentation for it now along with getting it ready for production usage.

Answer 7 · 2018-03-03T03:33:24.000Z

I just finished a complete port of the Java code to .NET Core 2.0. I just want to cleanup a bit before putting it on GitHub. Also ported jaeger-client-java to C# and opentracing-grpc :)

Answer 8 · 2018-03-05T09:55:40.000Z

@yurishkuro I ran into the following problems with Thrift:

There is no official thrift package that supports .net core (Support for net core was important for us).
I needed a custom version of the thrift compiler (remember this)
At the time, I had sporadic failures in the cross tests but I didn't know how to troubleshoot them.

I have not revisited the issue ever since the switch to Zipkin was made plus I had a baby which made it near impossible to work on my projects.

Answer 9 · 2018-03-05T10:08:53.000Z

I am really interested in a client for .NET core 2.x

@Falco20019 do you have a repo with that porting?

@yurishkuro will the final version be a repo is the jaegertracing org mainained by the original author?

Answer 10 · 2018-03-05T12:41:43.000Z

@luigiberrettini LetsTrace uses .Net Standard 2.0 so that would work for you.

Answer 11 · 2018-03-05T14:45:43.000Z

@gideonkorir The latest Thrift compiler has .NET Standard 2.0 support and this is what LetsTrace and my Port are using. Since I built it from the latest IDL, your pull request is also in there.

@luigiberrettini Depending on what you need, LetsTrace seems to be a lightweight port. If you need the full jaeger client, I should have my library cleaned up the next 1-2 days.

Answer 12 · 2018-03-05T15:19:40.000Z

@luigiberrettini we would like to have an official Jaeger client under jaegertracing org.

Answer 13 · 2018-03-05T15:44:39.000Z

@Falco20019 yup, still working through some things, but if your port does what we need might drop it for yours. I'm interested to see how you solved the Thrift issues. Did you have to pull in and build the Thrift project for netcore?

Answer 14 · 2018-03-05T16:30:14.000Z

@grounded042 yes, also had to build and include the thrift project since they still don’t offer a NuGET package...

Answer 15 · 2018-03-05T17:31:48.000Z

Maybe you could open an issue on the thrift project to ask them publish a NuGet package

Answer 16 · 2018-03-06T01:10:55.000Z

I just did an initial push. Only sampler right now is ConstSampler (for testing) and Metrics are still missing sampling and baggage stats. All senders and all reporters are otherwise implemented. You can have a look at https://github.com/Falco20019/jaeger-client-csharp/tree/dev

@luigiberrettini The thrift repository has issues disabled... Only pull requests are enabled and I can't create a pull request just for discussion.

Answer 17 · 2018-03-06T02:32:12.000Z

@Falco20019 I'm looking forward to reviewing.

One question - what it the purpose of jaeger-client-csharp/JaegerClientCsharp/Thrift/? Did you decide to vendor Thrift code?

Answer 18 · 2018-03-06T04:01:27.000Z

@yurishkuro Right now there is no other way to use thrift in .NET Core than to include the library in your project. No NuGET package available and also no option in the official repository to request one since issues are disabled. I will look into making the complete namespace internal to avoid collisions with others. Or into loading it into a NuGET package myself, but then there would be just another unmaintained package...

Tests will also be added before merging into master. Right now my test suite consists of an application that is using jaeger with grpc-opentracing (which I will also push soon) and the all-in-one docker container as endpoint.

Also got all but two samplers completed. I stumbled upon an inconsistency between thrift and the Java implementation: jaegertracing/jaeger-idl#43

Answer 19 · 2018-03-06T12:55:03.000Z

@Falco20019 you might be interested in what we do in LetsTrace.Jaeger. Since Nuget packages like to have only one dll in them we ended up bin packing Thrift and Jaeger.Thrift into the LetsTrace.Jaeger dll. We also internalized both of them so there are no collisions.

Answer 20 · 2018-03-06T12:59:04.000Z

Also, @Falco20019 any plan for unit tests in there?

Answer 21 · 2018-03-06T15:05:25.000Z

@grounded042 I have seen that you used ILRepack to bundle it. This solves the collision issue, but I still don’t like it as solution because if someone wants to use thrift, they would have to include the Lib a second time themselves. That’s why I am looking to get it packaged into a separate NuGET package.

Unit tests will come once I finished porting, so maybe today or tomorrow. Before moving it into master, I will have Thrift either repacked like you do (using a submodule) or make a NuGET package myself if I can’t reach Apache. Hope to get everything done today or tomorrow.

Answer 22 · 2018-03-06T15:36:44.000Z

It might be better to combine forces on this stuff. Seems like we have 3 different repos and time might be better spent all working on one.

Obviously I like the way LetsTrace is going and we're building out something that more so works with multiple tracers, but we were planning on making LetsTrace.Jaeger a fully featured Jaeger client.

However, if @Falco20019 is in this for the long run for Jaeger I say let's get some issues on that so multiple people can work on and possibly speed things along.

@yurishkuro what all is needed to become an official client? And after becoming an official client who takes control of the backlog and making sure it's up to speed properly? Wanting to make sure we set the csharp client up for long term stability and usage.

Answer 23 · 2018-03-06T23:05:14.000Z

@grounded042 I only had a peak at https://github.com/Chatham/LetsTrace.Jaeger and not at https://github.com/Chatham/LetsTrace so I totally missed that you also already have most of the stuff ported. I totally mistook that you only focused on the communication part, why I called it lightweight before. Sorry for that! I will have a look on what's missing and will see if it's possible to combine our stuff and add what you are missing. :)

Answer 24 · 2018-03-06T23:07:21.000Z

what all is needed to become an official client

@grounded042 if you and @Falco20019 agree to join forces and converge on a single library, that would be the ticket to make it official. The only other lib we know of was https://github.com/gideonkorir/jaeger4net by @gideonkorir, but he said it's no longer being worked on.

And after becoming an official client who takes control of the backlog and making sure it's up to speed properly

Jaeger is a community project, so nobody can give a guarantee of longevity or continued support, the best way is to actually get more users of the library who might be willing to take on small tasks in the future. I myself have some C# experience, so can do reviews and maybe minor bug fixes.

Answer 25 · 2018-03-06T23:09:41.000Z

Also, we will need to use Apache 2.0 license (I see that LetsTrace is MIT). This is a requirement from CNCF.

Answer 26 · 2018-03-06T23:19:03.000Z

Had a quick look at LetsTrace and I think I will fork it and bring the missing parts in as 1+ pull requests :) Looks like you were mostly done. Could have spared me a day if I had looked into it more careful yesterday instead of porting everything myself, but hey, my fault :) Just by the name, it looked like it had nothing to to with OpenTracing and was misleaded, that it's something totally different that "just has Jaeger support" in it.

Answer 27 · 2018-03-07T06:54:07.000Z

@grounded042 I'm just doing some porting and testing at https://github.com/Falco20019/LetsTrace/tree/improvements and will break it down into smaller pull requests if you want to discuss the changes more granularily before merging. I also started making some changes to LetsTrace.Jaeger to use the existing THttpClientTransport from Thrift instead of using HttpClient, since that one has more error handling already implemented. Also the naming is inconsistent to regular async APIs which should always have the "Async" suffix in the method name and add an CancellationToken parameter. You also used new CancellationToken instead of the default CancellationToken.None. I will also add the UDP transfer for LetsTrace.Jaeger.

Answer 28 · 2018-03-07T13:23:39.000Z

Great progress here!

@yurishkuro I will talk to the people at my company - our policy has always been MIT license, but I will explain the case and see if we can move it to Apache.

@Falco20019 PRs welcome! My C# async knowledge / experience is not much, so thanks for making sure that stuff is how it should be.

Answer 29 · 2018-03-07T13:24:58.000Z

Also, if we need to change up the language for LetsTrace to make it more apparent that it handles OpenTracing we can do that. I know the LetsTrace.Jaeger doesn't have a README yet and that would probably help make things a little clearer.

Answer 30 · 2018-03-07T13:42:34.000Z

@Falco20019 I looked over the improvements branch you have, and that's looking great! The once question I have has to do with what @yurishkuro mentioned up the thread.

we would like to have an official Jaeger client under jaegertracing org.

If we contribute and work together on LetsTrace and LetsTrace.Jaeger, is LetsTrace.Jaeger good enough to be the official client since it's kind of like a plugin to LetsTrace and not a standalone client? If what is wanted is a standalone client then I think we might be better served moving stuff from LetsTrace into your jaeger-client-csharp repo.

Answer 31 · 2018-03-07T14:53:34.000Z

@grounded042 Right no there is no jaegertracing/jaeger-client-csharp repository. So once we have LetsTrace on a good Level, @yurishkuro can create one and we do a PR to merge it in there :) helps keeping the history intakt.

And the official client for Java also has plugins for Zipkin and some other stuff. But I like the modular type that you don’t have to include the Jaeger Module if you only want Zipkin. The only thing we cant do here is to have a Trace being built without specifying a Reporter manually since LetsTrace itself has no default implementation. I am just still thinking how to get the metrics built in the easiest. Since right now, the user would have to set the same metrics instance to the sampler, the reporter and the tracer manually on creating each of them. So I think supplying Factories for Reporter and Sampler to Tracer.Builder would be the best for having an easy usage.

Answer 32 · 2018-03-07T15:18:49.000Z

@Falco20019 if the overall API of the client is OpenTracing, there is no problem with including a Zipkin reporter even if it all goes to jaegertracing/jaeger-client-csharp - we do have zipkin reporters in other Jaeger clients as well.

Regarding the repo migration, there are three ways we can do it:

move the whole repository under jaegertracing org and then rename it to jaeger-client-csharp. This preserves the full history, not just commits, but also PRs, issues, stars, subscribers, etc. Also the original link https://github.com/Chatham/LetsTrace will be automatically redirected by GitHub.
push or fork LetsTrace git repo to jaegertracing/jaeger-client-csharp - this only preserves the commit history, and kind of leaves LetsTrace repo in a limbo state
only do a single initial commit to jaegertracing/jaeger-client-csharp - this won't even preserve the history of commits.

I would recommend Option 1, this is what has been done with most other Jaeger clients that originally were started under uber org. I will need to be made admin of LetsTrace repo to execute the move.

@grounded042 regarding MIT/Apache license, strictly speaking if we went with say Option 3 above, then your company wouldn't need to give permission since the code is already open sourced under MIT and can be freely cloned/modified. But it is indeed better if they agree to replace the license headers in-place in all the files to those of ALv2, before we execute the repo move.

Answer 33 · 2018-03-07T15:32:19.000Z

@yurishkuro I talked to the guys at my company and they're totally fine with Apache, so I'll go ahead and switch that over today.

Answer 34 · 2018-03-07T15:36:25.000Z

@Falco20019 I see what you're saying about the modular stuff. Could we create some sort of helper in LetsTrace.Jaeger that automatically gives you a tracer setup with Jaeger defaults?

Answer 35 · 2018-03-07T15:50:52.000Z

@yurishkuro Didn’t know that option 1 exists :) So totallly prefer that one since @grounded042 is fine with APL2

@grounded042 Yeah, could be a helper there. I‘m looking at different ideas like with Logging in Core and so on. But I think just a static method could also be enough. Hope to get some more stuff done today

Answer 36 · 2018-03-07T15:58:43.000Z

@Falco20019 perfect - let me know if there is anything you need me to do. Once you finish the improvements / merging your code in we should probably start making some issues on LetsTrace and LetsTrace.Jaeger.

Answer 37 · 2018-03-07T20:53:12.000Z

@Falco20019 the Thrift GitHub repo is a mirror of the official one.

Issues can be submitted on Jira:
https://issues.apache.org/jira/projects/THRIFT

Answer 38 · 2018-03-07T21:22:12.000Z

@luigiberrettini Thanks for the link. I just filed an issue here: https://issues.apache.org/jira/browse/THRIFT-4512

Answer 39 · 2018-03-08T03:48:22.000Z

@grounded042 I achieved a lot today. Got metrics and samplers implemented. Also integrated your changes from better_third_party_support. The RemoteControlledSampler uses ISamplingManager to completely hide the Thrift stuff and can also be implemented independently from Jaeger. Also added the UDP-Sender for Thrift. This makes ITransport (for RemoteReporter) and ISamplingManager (for RemoteControlledSampler) independant from IReporter and ISampler.

Getting a NoopTracer:

Tracer serverTracer = new Tracer.Builder("server")
.Build();

Getting a ConsoleTracer (right now using Console.Out, but I want to get ILogger in everywhere):

Tracer serverTracer = new Tracer.Builder("server")
.WithReporter(new ConsoleReporter())
.Build();

Getting a tracer with InMemoryMetrics, using the RemoteReporter with JaegerUDPTransport and an RemoteControllerSampler with HttpSamplingManager (both using default ports on localhost) (needs LetsTrace.Jaeger package):

Tracer serverTracer = new Tracer.Builder("server")
.WithTransport(new JaegerUDPTransport())
.WithSamplingManager(new HttpSamplingManager())
.WithMetricsFactory(InMemoryMetricsFactory.Instance)
.Build();

I adjusted the existing unit tests, but of course, there need to be a lot new ones implemented. Will to that before getting to merge anything. Same for adjustments to the README and adding a README for Letstrace.Jaeger.

Answer 40 · 2018-03-08T13:58:55.000Z

If you give me access to your forks I can start working on tests for LetsTrace.

Answer 41 · 2018-03-08T17:25:39.000Z

@grounded042 Sure thing, just added you as collaborator 👍

Answer 42 · 2018-03-12T18:58:01.000Z

Wondering if there is any help I can provide here. I have a client that has a need for Jaeger tracing in dotnet core 2.0 projects and this seems to be the best option.

Answer 43 · 2018-03-12T22:32:57.000Z

@mweinand there are still some unit tests missing for example. Features should be mostly on par with Java on my fork in the improvement branch. You could also do a review of the code if you want or work on documentation (xmldoc).

Answer 44 · 2018-03-13T13:13:29.000Z

@grounded042 and I have created some tracking issues for the open points in LetsTrace and LetsTrace.Jaeger that we need to get completed before getting it approved as official client:
jaegertracing/jaeger-client-csharp#5
https://github.com/Chatham/LetsTrace.Jaeger/issues/1

Answer 45 · 2018-03-24T09:44:39.000Z

Will the project still be called "LetsTrace" or will it be renamed to e.g. "Jaeger"? Seems a bit confusing to have a jaeger-client-csharp project that contains something called "LetsTrace" (which sounds completely tracer-independent).

Answer 46 · 2018-03-26T12:35:16.000Z

Personally, I would prefer to rename LetsTrace to Jaeger.Core and LetsTracer.Jaeger to Jaeger.Transport.Thrift. Then LetsTrace.Zipkin could be Jaeger.Transport.Zipkin. This would be in accordance to the already existing Jaeger.Thrift which is generated from jaeger-idl and would make it easier to see that Thrift transport consists of 3 projects and Zipkin is just the transport.

I also would like to see the following NuGET packages:

Jaeger.Core
Only containing project Jaeger.Core
Jaeger.Thrift
Containing projects Thrift, Jaeger.Thrift and Jaeger.Transport.Thrift
Depending on NuGET Jaeger.Core
Jaeger.Zipkin
Containing project Jaeger.Transport.Zipkin
Depending on NuGET Jaeger.Core
Jaeger
Depending on NuGET Jaeger.Thrift as an alias and easy way to keep up2date in case we change something

This would allow users to use Jaeger.Core + Jaeger.Zipkin if they only want to use the Zipkin transport without having to include Thrift at all, or just Jaeger + Jaeger.Zipkin. And the default user can just use Jaeger which allows as to remove Thrift runtime from NuGET Jaeger.Thrift and replace it by a dependency at a later point once there is an official release.

If you want to spare NuGET Jaeger.Thrift we can also just make that NuGET Jaeger (similar to who it's handled in Java).

/CC @yurishkuro @grounded042

Answer 47 · 2018-03-26T12:46:35.000Z

I think that NuGet packages should have the same name as the underlying assembly.

Having a Jaeger.Zipkin package which contains a Jaeger.Transport.Zipkin.dll is confusing. The package should therefore be called Jaeger.Transport.Zipkin IMO.

(Having only "Jaeger.Zipkin" is confusing anyway as this just names two different products without telling how they are related to each other.)

Answer 48 · 2018-03-26T13:00:45.000Z

This would reflect how it's also done in the jave repository: jaeger-zipking and jaeger-thrift.

This naming scheme is also used by GRPC where GRPC is an alias for GRPC.Core. Jaeger.Thrift contains more than just one DLL compared to Jaeger.Zipkin. Those two would just be easier ways to include the wanted technology with all it's dependencies.

Answer 49 · 2018-03-28T09:07:52.000Z

As there is a plan to rename the projects, I created a separate issue for this in the repo and I copied the comments from here to that new issue. This should ensure that this issue doesn't go off-topic.

Please file any new comments regarding the naming here: jaegertracing/jaeger-client-csharp#34

Answer 50 · 2018-03-30T15:36:57.000Z

Official client created https://github.com/jaegertracing/jaeger-client-csharp