Bring in the improvements branch

[grounded042]

@Falco20019 has an improvements branch that we have both been working on (mostly him :)).

It would be nice to get this merged in an incorporated soon. I'm creating this so we can track what all needs to be done before we merge.

[grounded042]

@Falco20019 do you know what all needs to be done before you're ready to merge?

[Falco20019]

I think we only need to get the tests done before merging the branch. Functionality should be mostly complete and on par with jaeger-client-java. A code review would also be nice, just to ensure maintainability. Right now, I'm filling in the missing tests based on coverage analysis. All of my other open tasks can be reparate pull requests that can be worked on afterwards and independently.

I also would love to have thrift excluded and in an official NuGET package, but that only affects LetsTrace.Jaeger.

Open tasks known by me:

• Complete test suite
• Extract some constants into Constants.cs
• Add Metrics support for baggage
• Make RemoteReporter async (have an implementation that I can reuse from my own project)
• Baggage restrictions in Tracer (can be postponed)
• Checking if NullReporter can be replaced by LoggingReporter(NullLoggerFactory.Instance)
• Performance tests

[grounded042]

I'm with you on the Thrift issue - we could publish the package ourself, but I really don't want to do that.

I'm working through tests - right now we are at 80% coverage and I'd like to get to at least 90%. As for a code review I'm going to have some other guys from Chatham take a look, but others are welcome.

We are starting to look at getting this into production, so we'll also be able to see how using it actually works.

[grounded042]

My goal is to get this merged in by the end of the week.

[Falco20019]

@mweinand and @yurishkuro offered help, so maybe they could look into doing a code review. I'm out of office for the rest of this week, so I only will try to get tests and constants done the next couple hours. I assume, the rest of the TODOs can be postponed since they haven't been part of LetsTrace before and are also not mandatory according to jaegertracing/jaeger#366

[grounded042]

I need to go through and change a lot of the TODOs to issues here on GitHub

[grounded042]

I'll look at getting things reviewed and merged by the end of the week and maybe when you come back we'll have a new package ready to go!

[Falco20019]

Sounds like a plan. I'm pretty happy with the improvements and how quickly we got them done so far. Thought it would take longer to implement :) Thanks for the support on the unit tests, greatly appreciate it.

[grounded042]

Yeah, no problem. Glad we could join forces on this project. Can't wait to see where it goes.

[Falco20019]

We are at 95% coverage now. PerOperationSampling needs some more love but I'm out of office now. Maybe someone could extend the RemoteControlledSamplerTest, which should cover nearly all of the uncover test surface. An TODO notice is left in the code.

[yurishkuro]

you guys rock!

[grounded042]

@Falco20019 sweet! I've actually been working on the PerOperationSampling tests today, so we're all good!

[grounded042]

seeing if we could replace NullReporter with LoggingReporter(NullLoggerFactory.Instance) was mentioned above, but I'm going to leave the NullReporter in there as Java also has one. I like explicitly defining a null reporter.

[grounded042]

We have tests done with 99% coverage right now. I'm going to have some guys here at Chatham do the review to merge it in. Once that is complete we'll working on making sure LetsTrace.Jaeger merges in the improvements branch. Once that is done we can merge the two together and get working on moving everything into the jaegertracing org.

[mweinand]

Nice work!

[grounded042]

This has been merged. I bumped the version and updated the README as well. Next step is to get the improvements branch merged into LetsTrace.Jaeger and then merge that repo with this. Before we execute the move to the Jaeger org we'll probably want to put some compiler warnings in for anyone using the lib under it's old name.