Skip to content
This repository has been archived by the owner on Apr 28, 2022. It is now read-only.

Should this project have parity with jaeger-client-java? #29

Closed
cwe1ss opened this issue Mar 27, 2018 · 13 comments
Closed

Should this project have parity with jaeger-client-java? #29

cwe1ss opened this issue Mar 27, 2018 · 13 comments
Milestone

Comments

@cwe1ss
Copy link
Collaborator

cwe1ss commented Mar 27, 2018

If the plan is to make this the official C# Jaeger client, should this project be as close as possible to jaeger-client-java? I noticed that there's quite a few things missing or done differently right now. E.g. SpanContext is not immutable, metrics are implemented differently, there's ILetsTrace* interfaces here, etc.

This obviously makes porting features a little bit harder. For instance, I was trying to port the missing "baggage restrictions" files but this requires a few changes due to the non-immutability of SpanContext. Also, the tests can't be ported easily either.

I understand the desire to have your own implementation, so this would be fine by me. However, if you want to make it a Java-port, I could donate some time and bring this library on par with Java. I should have a few full days of availability until next week. This would obviously result in breaking changes though.

I could do the following things:

  • Merge the LetsTrace.Jaeger PR
  • Rename everything to Jaeger.* if you want (see C# client library jaeger#578 (comment) )
  • Remove the ILetsTrace* interfaces as they don't exist in Java
  • Make SpanContext immutable
  • Add missing features
  • Bring existing features on par with jaeger-client-java where useful.

Let me know if this would help.

@grounded042
Copy link
Collaborator

Thanks for the offer @cwe1ss. Your help is definitely appreciated. When we first started on this project we did not have any idea of becoming the official C# client for Jaeger - however, things change and it looks like we are moving that way (which I think is a good thing).

We took a good bit of inspiration from the Java client in the beginning as well as the Go client, but we didn't do a straight port as being a Jaeger client was not the goal at the time. Internally we are working on adding LetsTrace to our stack, but the whole reason we open sourced it was because we thought the community could benefit from what we had and we knew we could benefit from what the community had to offer. I am still young in my C# experience and I'm sure there are better ways out there to approach different aspects in this library - all of that to say that we welcome community feedback and input on how to move this library forward and to better serve the community.

Personally I think that what you mentioned above is a good idea. I'd like to get some input from @Falco20019 and @yurishkuro on this before we move forward.

Also I'd like to note that my last day at work is April 6th before I take off on a 6 month hike where I won't really have access to do any coding. I know @kevindaviscfc and @PhillipChaffee will be the main contributors from Chatham to this project, but I'd also like to get more of the community involved as I don't want this project to just be a project of Chatham. I think it is in the best interest of the project to bring on other contributors - especially once this is moved to Jaeger org.

@cwe1ss
Copy link
Collaborator Author

cwe1ss commented Mar 27, 2018

Also I'd like to note that my last day at work is April 6th before I take off on a 6 month hike where I won't really have access to do any coding.

I've been on a few big holidays as well - enjoy it and stay away from computers as far as you can! 😄 Where do you go?

Making it a near-clone of Java has the big advantage that we can do this very quickly, so we all would get a first fully-featured OpenTracing-compatible tracer ASAP.

Combining this tracer with the instrumentation-library OpenTracing.Contrib.NetCore should give us a pretty good starting point for OpenTracing compatible tracing in .NET (Core). As soon as we have that, we can create some blog posts and tutorials which hopefully helps OpenTracing to gain more traction in the .NET community.

@yurishkuro
Copy link
Member

Ramping up on features is a good idea. But I would suggest first focusing on wrapping up the migration to the jaeger org.

@grounded042
Copy link
Collaborator

@cwe1ss I'll be hiking the Pacific Crest Trail which goes from Mexico to Canada through the United States. I've never taken a big break like this, but I'm looking forward to it.

@cwe1ss
Copy link
Collaborator Author

cwe1ss commented Mar 27, 2018

@grounded042 Sounds awesome!!! I hope you have good shoes! 😅

What's the ETA for moving the project to the Jaeger org? Anything else missing now that #10 has been merged?

@grounded042
Copy link
Collaborator

I'd like to get the build passing. See #30. I'm trying to tackle that. Once we get that done I think we should be good to move.

Does anyone know how package name change on NuGet are handled? Should we push up one version of the package with build warnings on things like Tracer that notify people this is the last version of LetsTrace and that it will be under a different name on NuGet soon?

@cwe1ss
Copy link
Collaborator Author

cwe1ss commented Mar 27, 2018

Publishing a new version (from the last release tag) with a notice in the package description and [Obsolete] on something that everyone has to use would definitely be the most user-friendly version.

An easier (but not so friendly) option would be to just unlist all packages and hope that people will find the new packages by themselves. I guess that most people who are brave enough to use a 0.0.* product will look it up on github anyway. Given that there's < 200 downloads, this might be an acceptable solution if you don't want to put in the effort of creating a new release.

@Falco20019
Copy link
Collaborator

Falco20019 commented Mar 27, 2018

I'm fine with getting it closer to the java version. A lot of parts that I ported have their roots in the java implementation. I also have some stuff from my own implementation that I started before jumping on the LetsTrace train (jaeger-client-csharp). I'm sure there are already some parts of code that can be copied into the LetsTrace code base to fill some holes or get closer to the java implementation. But I thought it's easier to first get LetsTrace closer to completetion and then fix the portability issues. So any help is welcomed from my side!

@grounded042 I wish you a nice trip! Thanks for all the work so far! Do you plan to get the move to jaeger org done before you leave? Otherwise, are @kevindaviscfc and @PhillipChaffee setup as collaborators with full permissions so that they can get the move done?

@grounded042
Copy link
Collaborator

Thanks @Falco20019! Also thank you for all the work you've done too. Great to see things come together on this. I would like to get it moved before I leave and if for some reason that doesn't happen then @kevindaviscfc and @PhillipChaffee will be able to handle things for you.

@cwe1ss
Copy link
Collaborator Author

cwe1ss commented Apr 12, 2018

I've been trying to get parity by doing PRs for each module (e.g. #54 ) but this is quite annoying as it often affects many other modules as well.

For that reason, I've been working on a (very) BIG port. I have a WIP (it doesn't yet compile and some stuff is still missing) in the cweiss/JavaParity branch and I will probably get it done tomorrow.

This would bring feature/code parity with Java (except for a few things) but it obviously breaks some stuff and the PR will be very hard to review. (I e.g. also rearranged methods to be aligned with Java etc)

I still think it would be worth doing as this makes porting any future changes much easier. We can then rely on the code being almost the same. This has also been quite nice in opentracing-java vs. opentracing-csharp.

Also, doing separate PRs will take MUCH longer.

Your thoughts?

@Falco20019
Copy link
Collaborator

I‘m fine with a bigger PR. Depending on when you get it done, I would have a look. I will also have a look at your branch tomorrow to get a first glimpse since I have to leave 5pm tomorrow for a birthday and since the weekend is pretty stuffed.

@Falco20019
Copy link
Collaborator

Falco20019 commented Apr 20, 2018

Maybe this is the better place for the parity discussion. There are some open discussion points for after #55 is merged. This is just to have everything on one starting point. For some points it's better to start of a new issue later on.

  • How close do we want to stay to the Java code base:
    • Technically identical but making use of C# features?
    • Making use of partial classes for better structuring or keeping it in one file? (see here)
    • Have only Builder ctors instead of a mixed environment? Tests can also use the builder. (see here)
    • Do we want to keep TraceId and SpanId separate? (see here)
    • Do we want to rename Jaeger.Core to just Jaeger? (see here)
  • Do we want to keep the dependency on Thrift in the Jaeger.Core? (see here)
    • Java wants to get away from it
  • Shift from ISender to ITransports and IEncoders? (see here)

@Falco20019 Falco20019 added this to the v0.1.0 milestone May 29, 2018
@Falco20019
Copy link
Collaborator

I will create separate issues for the remaining discussions and will afterwards close this issue since we decided to get closer to Java but still take the freedom to use C# features when it makes sense.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants