Giter Club home page Giter Club logo

jaeger-client-csharp's People

Contributors

albertteoh avatar bocharovf avatar bojanpantovic1989 avatar cwe1ss avatar enesunal avatar falco20019 avatar fangchd avatar fcampana-fastcode avatar grounded042 avatar jamendozag avatar jnsso avatar js8080 avatar kevindaviscfc avatar marusyk avatar mihai-unity avatar phxnsharp avatar pikbot avatar regeldso avatar rwkarg avatar sosiska avatar xdarsie avatar yurishkuro avatar

Stargazers

 avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar

Watchers

 avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar

jaeger-client-csharp's Issues

Shift from ISender to ITransports and IEncoders

The follow-up discussion from here.

The open questions are:

  • Do we want to shift from ISender to ITransports and IEncoders

This is definetly a meta topic, but I think it still would be good to have a local look at it first. Makes aggregating points for and against in the main issue easier.

Break LetsTrace into multiple packages

We currently have multiple projects in LetsTrace. The NuGet way is to have different packages instead of building everything into one. I think this would also benefit us in keeping things separated.

We have the following packages:

  • LetsTrace
  • Thrift
  • Jeager.Thrift

We could break these up and some namespaces inside of LetsTrace into the following:

  • LetsTrace - the main, core implementation. No specific tracing system implementations.
  • LetsTrace.Jaeger - LetsTrace pieces needed to work with Jaeger
    • Jaeger.Thrift would most likely be bin-packed in here
    • Thrift would most likely be bin-packed in here
  • LetTrace.Zipkin - LetsTrace pieces needed to work with Zipkin
    • We might be able to use the Thrift implementation here

Move to Jaeger Org

Hey @yurishkuro we've been chatting at Chatham about getting tracing implemented and we've got a couple of questions about LetsTrace once we move it over to the Jaeger Org. We had talked before about ownership over the backlog (jaegertracing/jaeger#578 (comment)).

We're still wondering about the backlog and contributors - we have a skilled C# team that is going to be using the library internally and who is willing to contribute, speed along, and maintain the repo. It seems like there are not very many C# developers in the Jaeger Org at this point and we're concerned that LetsTrace won't get the love it needs once moved. Will we be considered maintainers once the move occurs? Or will we have to be forking and submitting PRs to be reviewed by someone in the org?

Rename projects to Jaeger.*

I'll move the discussion regarding the new project names into this new issue to not let the original issue (jaegertracing/jaeger#578 ) go off-topic.

From @cwe1ss:

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).


From @Falco20019:

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


From @cweiss:

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.)


From @Falco20019:

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.

Add DCO

Model after main jaeger repo

Infinite loop in Jaeger.Util.Utils.UniqueId under concurrent use

Requirement - what kind of business use case are you trying to solve?

Infinite loop in Jaegetr.Util.Utils.UniqueId during concurrent Span building.

Problem - what in Jaeger blocks you from solving the requirement?

Access to an internal static System.Random instance is not synchronized to prevent concurrent access

Proposal - what do you suggest to solve the problem or improve the existing situation?

Add locking around the specific call to System.Random. See PR #91

Any open questions to address

Spans not getting reported to UI

I am trying to write a hello world to evaluate Jaeger for C# Client API

And defining the Tracer like this :

public Tracer Init(string serviceName, ILoggerFactory loggerFactory)
        {       
            var senderConfiguration = new Configuration.SenderConfiguration(loggerFactory).WithAgentHost("localhost").WithAgentPort(6831);
            var reporterConfiguration = new Configuration.ReporterConfiguration(loggerFactory)
                .WithSender(senderConfiguration)
                .WithLogSpans(true);
            var samplerConfiguration = new Configuration.SamplerConfiguration(loggerFactory)
                .WithType(ConstSampler.Type)
                .WithParam(1);
            return (Jaeger.Tracer)new Configuration(serviceName, loggerFactory)
                .WithSampler(samplerConfiguration)
                .WithReporter(reporterConfiguration)
                .GetTracer();                
        }

But I see the spans getting reported to logs but nothing in UI running on http://localhost:16686/search

On reboot it says

Initialized Tracer(ServiceName=xyz, Version=CSharp-0.2.0.0, Reporter=CompositeReporter(Reporters=RemoteReporter(Sender=UdpSender(UdpTransport=ThriftUdpClientTransport(Client=10.34.153.126:6831))), LoggingReporter(Logger=Microsoft.Extensions.Logging.Logger`1[Jaeger.Reporters.LoggingReporter])), Sampler=ConstSampler(True), IPv4=0, Tags=[jaeger.version, CSharp-0.2.0.0], [hostname, LUSC02WG0EAHTDH], ZipkinSharedRpcSpan=False, ExpandExceptionLogs=FalseUseTraceId128Bit=False)

Any help is greatly appreciated.

Thanks.

Required field Process is not set

Hello,

after trying the latest version (0.1.0), I get an error from Jaeger server:
Unable to process request body: Required field Process is not set\n.
Could you help me?

My configuration:

            var senderConfiguration = new Configuration.SenderConfiguration(loggerFactory)
                .WithEndpoint($"http://{jaegerConfig.ServerIp}:{jaegerConfig.ServerPort}/api/traces");

            var reporterConfiguration = new Configuration.ReporterConfiguration(loggerFactory)
                .WithSender(senderConfiguration);

            var samplerConfiguration = new Configuration.SamplerConfiguration(loggerFactory)
                .WithType("const");

            Tracer = new Configuration(serviceInfo.ServiceId, loggerFactory)
                .WithReporter(reporterConfiguration)
                .WithSampler(samplerConfiguration)
                .GetTracer();

Usage:

           var builder = _tracer.Tracer
                .BuildSpan("HomeControler request")
                .WithTag("Server", Tags.SpanKindServer)
                .WithTag("Client", Tags.SpanKindClient)
                .WithTag(Tags.DbInstance.Key, "")
                .WithTag(Tags.DbType.Key, "sql");

            using (var scope = builder.StartActive(true))
            {
                var span = scope.Span;

                ViewBag.Title = "My Sample";
                span.SetOperationName("Home page");
                span.SetTag("smth", "special value");

                using (var scope2 = _tracer.Tracer.BuildSpan("Home page 2").StartActive(true))
                {
                    var span2 = scope2.Span;
                    span2.Log("Before scope 2");

                    Task.Delay(2000).Wait();

                    span2.Log("After scope 2");
                }

                span.SetTag("smth2", "special value 2");
            }

BTW, a lot of errors were swallowed, so I had to debug it locally to figure out the real issue.
For example:
RemoteReporter, line 135 (catch (SenderException ex)) only report through _metrics.ReporterFailure, but not to _logger, so when I forgot http:// in an url, it didn't do anything.
THttpClientTransport, line 180 (msg.EnsureSuccessStatusCode();) it throws exception, but doesn't return message from server.
If you help me with this issue, I can provide PR for logging correction.

Use partial classes

The follow-up discussion from here.

The open questions are:

  • Do we want to have stuff more seperated or stay closer to Java for easier maintaining

Restructuring the code base has no effect on the API, so it is a purely internal choice.

Support Third Party Propagation

In order to more fully support third party tracing systems we need to have something along the lines of a propagation registry like the Java client has.

Bring in the improvements branch

@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.

Correlating traces between csharp and java clients

Im trying to set up a demo to demonstrate tracing across various languages supported by opentracing.

I am running into an issue when trying to get a trace between csharp and java.

I think i have tracked it down to the fact that the traceid generated by the chsarp client is too long.

It seems that the traceid is 32 characters long in the generated header but all the traceids generated by both java and python are only 16 character.

csharp example
e25d900a5c462be7cafa992548dc6d0d

java example
cafa992548dc6d0d

I havent been quite able to get to the bottom of why this might be and what the issue is with the longer id on the java end as it seems to cause an issue and make the java client create a new traceid and breaks the cross process tracing.

I think i have tracked the cause down to the tostring method on TraceID here https://github.com/jaegertracing/jaeger-client-csharp/blob/master/src/Jaeger/TraceId.cs#L38

But im not sure if this is right or wrong.

Separation of TraceId and SpanId

The follow-up discussion from here.

The open questions are:

  • Do we want to keep TraceId and SpanId separate?
  • Do we want to stay close to Java in this case?

I like the separation more than the Java implementation. As of now, jaeger-client-java only supports 64bit TraceIds. We also default to 64bit and are also completely compatable but still keep the flexibility.

CI choice

I noticed this repo uses AppVeyor for CI - is this just a preference or a convention for .Net projects (e.g. travis.org is not an option)?

Propagation format is seemingly only configurable through environment variables

Requirement - what kind of business use case are you trying to solve?

Integration with Istio, by propagating zipkin headers.

Problem - what in Jaeger blocks you from solving the requirement?

It is not clear how to configure the propagation to use the zipkin format.
The only approach I've found is to use environment variables.
Since I'm interesting in configuring this through C# code, I'm currently using an approach like this:

...
Environment.SetEnvironmentVariable("JAEGER_PROPAGATION", "b3");
Configuration config = Configuration.FromEnv(loggerFactory);
ITracer tracer = config.GetTracer();
...

Proposal - what do you suggest to solve the problem or improve the existing situation?

Provide a way to configure the propagation, directly in C#.
If there already is, I haven't been able to figure it out, and would appreciate some documentation or examples that address this. πŸ˜„

Only Builders, no Constructors

The follow-up discussion from here.

The open questions are:

  • Do we want to only have Builders and make all ctors consistently private
  • Do we want to stay close to Java in this case?

I assume this to become a meta topic, since it would be best to stay close to Java in this case, since it affects the API. So it should be changed everywhere or nowhere.

When is 0.2.0 expected to be released?

Requirement - what kind of business use case are you trying to solve?

Use Jaeger version without unbounded CPU utilization bug (see PR #91)

Problem - what in Jaeger blocks you from solving the requirement?

The currently published 0.1.1 package does not contain the above bug fix

Proposal - what do you suggest to solve the problem or improve the existing situation?

Provide guidance on when to expect 0.2.0 or some other package release that contains the fix in the above PR.

Any open questions to address


Dependency on Thrift

The follow-up discussion from here.

The open questions are:

  • Do we want to keep our dependency on Thrift?

Java thinks about modularization and using Configuration and service load pattern to only load what's needed. We would have to analyze how to do this in C#. I assume it would be similar to how Microsoft.Extensions.Logging and ASP.NET Core are abstracting everything into packages and enabling it though adding a middleware/provider and calling an extension method on Configuration presumably.

v0.0.10 release plan

Now that #55 and #59 are merged, we should do a release soon.

My main open question is whether we should wait for the upcoming v0.12.0 release of OpenTracing? I'd like to release that one by the end of next week.

Anything else that should be included?

Consider using "OpenTracing.Contrib.NetCore" in examples

What do you think about using OpenTracing.Contrib.NetCore in the examples project? It does not require people to add a custom middleware and it has more features. This would give people better guidance on what to use in ASP.NET Core projects.

Your examples could then focus on how the LetsTrace tracer is initialized/configured. As it implements OpenTracing, usage in instrumentation-code isn't tracer-specific anyway.

Add size checking in transport

Currently we do not check the buffer byte size in the Jaeger.Transport.Thrift.Transport.JaegerThriftTransport class. This can lead to the exception Cannot flush closed transport. Message too long. I have seen the exception when using the JaegerUdpTransport, but not the JaegerHttpTransport In other Jaeger client implementations there are checks to make sure exceptions like that do not happen: https://github.com/jaegertracing/jaeger-client-java/blob/master/jaeger-core/src/main/java/com/uber/jaeger/senders/ThriftSender.java

We should add a check. Also of note is that TMemoryBufferClientTransport is missing a reset method which could complicate things as we need that.

Create NuGet organization and assign permissions

@yurishkuro We currently can't deploy because the NuGet credentials in our appveyor.yml file are wrong. Could you please do the following things - this will ensure that we have a similar setup like the OpenTracing NuGet account where multiple people have access to the NuGet packages:

  • Login on nuget.org with your "javajumbo" account
  • Add a new organization and name it e.g. "Jaeger", "JaegerTracing", ... (whatever you like and is available).
  • Add this new organization as an "owner" to all "Jaeger"-packages. You can do this on this page: https://www.nuget.org/account/Packages
  • Add administrators to the organization. My NuGet account name would be "cwe1ss". Not sure what @Falco20019 's account name is. (I can always add him later once I'm an admin)

As you've already made me an admin on the "jaegertracing"-AppVeyor account, I'll then do everything else (I'll create the new NuGet API key and add it to AppVeyor and I'll update the appveyor.yml file)

Admin-rights for @Falco20019

Unfortunately, I'm currently really busy at work so I have very limited time for contributions :(

I therefore like to give @Falco20019 admin-rights for this repo. This allows him to e.g. squash a stuck PR (see #91) He already has write-access.

any concerns?

/cc @yurishkuro

ChildOf reference doesn't work

Hello,

thank you for the library. I continue to integrate Jaeger into our system, but I struggle with setting a reference to a parent span.
We are using Linkerd as a service mesh which sends us open tracing coordinates via HTTP header.

It’s base64 encoded and formatted as: spanId:8 parentId:8 traceId:8 flags:8' (big-endian).
link

I managed to parse that coordinates and create parent context with method SpanContext.ContextFromString(decodedString) and reference it by span.AddReference(References.ChildOf, parentCtx).
But after sending it through HTTP sender (whether it was sent), I'm not able to find any of my trace logs in Jaeger GUI (without that reference, it is visible at least under own service category).

Are there any logs which could help me to locate the issue?
Do you see any obvious problem with my code below?

More info:

Code:

        public ITracer Tracer { get; }

        public ISpanBuilder BuildSpan(string operation)
        {
            var span = Tracer?.BuildSpan(operation);
            if (span == null)
                return null;

            var coordinates = _correlation.GetTracingCoordinates();
            if (!string.IsNullOrWhiteSpace(coordinates))
            {
                var parentCtx = GetParentContextFrom(coordinates);
                if (parentCtx != null)
                    span.AddReference(References.ChildOf, parentCtx);
            }       
            return span;
        }

        private static SpanContext GetParentContextFrom(string coordinates)
        {
            try
            {
                var decoded = Convert.FromBase64String(coordinates);
                var splitted = Split(decoded, 8).Select(ByteArrayToString).ToArray();
                var decodedString = string.Join(":", splitted[2], splitted[1], splitted[0], splitted[3]);
                var ctx = SpanContext.ContextFromString(decodedString);
                return ctx;
            }
            catch
            {
                return null;
            }
        }

Trace id: 7d04838dcc710f8f
Coordinates: UmfYme1Ztni5uqIlE9LJin0Eg43McQ+PAAAAAAAAAAY=

Runtime values:
image

Jaeger gui:
image

Logs with reference:
Nothing

Logs without reference:

info: Jaeger.Configuration[0]
      Initialized Tracer(ServiceName=DataAccessSample, Version=CSharp-0.1.1.0, Reporter=RemoteReporter(Sender=HttpSender), Sampler=ConstSampler(True), IPv4=-1062683071, Tags=[jaeger.version, CSharp-0.1.1.0], [hostname, DESKTOP-7OEFQ0F], [ip, 192.168.190.65], ZipkinSharedRpcSpan=False, ExpandExceptionLogs=False)

Trouble getting traces into Jaeger

First: Love this project! Hopefully we can use it to trace all of our messages in our EEA.

My Problem: I've played around with the C# API a bit in the last days but I've trouble getting the traces into my Jaeger instance. Maybe someone here can help me and provide an useful code snippet.

What I did:
Start Jaeger in a stand-alone Docker container with
docker run -d --name jaeger --rm -p6831:6831/udp -p5775:5775/udp -p16686:16686 jaegertracing/all-in-one:latest

Now, I wanted to simple get just one test trace into the running Jaeger container, for which I wrote the following C# code:

using System;
using System.Threading;
using Jaeger.Core;
using Jaeger.Core.Metrics;
using Jaeger.Core.Reporters;
using Jaeger.Core.Samplers;
using Jaeger.Transport.Thrift.Samplers;
using Jaeger.Transport.Thrift.Transport;
using Microsoft.Extensions.Logging;
using Microsoft.Extensions.Logging.Console;
using OpenTracing.Tag;
using OpenTracing.Util;

namespace OpenTracingEval
{
    class Program
    {
        static void Main(string[] args)
        {
            var loggerFactory = new LoggerFactory();
            loggerFactory.AddProvider(new ConsoleLoggerProvider((text, logLevel) => logLevel >= LogLevel.Trace, true));
            var serviceName = "initExampleService";
            var sender = new JaegerUdpTransport("localhost", 6831); // Tried also 5775
            var reporter = new LoggingReporter(loggerFactory);
            var sampler = new ConstSampler(true);
            var tracer = new Tracer.Builder(serviceName)
                .WithLoggerFactory(loggerFactory)
                .WithReporter(reporter)
                .WithSampler(sampler)
                .WithTransport(sender)
                .Build();

            // Tried also this code
            //var tracer = new Tracer.Builder("server")
            //    .WithTransport(new JaegerUdpTransport("localhost", 5775))
            //    .WithSamplingManager(new HttpSamplingManager())
            //    .WithMetricsFactory(InMemoryMetricsFactory.Instance)
            //    .Build();


            GlobalTracer.Register(tracer);

            using (var scope = tracer.BuildSpan("someWork").StartActive(true))
            {
                try
                {
                    for (var i = 0; i < 5; i++)
                    {
                        Console.WriteLine("Doing some heavy stuff.");
                        Thread.Sleep(1);
                    }
                }
                catch (Exception e)
                {
                    Tags.Error.Set(scope.Span, true);
                }
            }   
            tracer.Dispose();
            Console.ReadKey();
        }
    }
}

I get the expected output in the console, but no trace is added in my Jaeger instance.

Doing some heavy stuff.
Doing some heavy stuff.
Doing some heavy stuff.
Doing some heavy stuff.
Doing some heavy stuff.
info: Jaeger.Core.Reporters.LoggingReporter[0]
      Reporting span:
 {
        "Context": {
          "TraceId": {
            "High": 15815064987969972333,
            "Low": 9610257056116139028,
            "IsValid": true
          },
          "SpanId": {},
          "ParentId": {},
          "Flags": 1,
          "IsSampled": true
        },
        "FinishTimestampUtc": "2018-04-20T13:22:01.0115301Z",
        "Logs": [],
        "OperationName": "someWork",
        "References": [],
        "StartTimestampUtc": "2018-04-20T13:22:00.9998658Z",
        "Tags": {
          "sampler.type": "const",
          "sampler.param": true
        }
      }

I've also checked if there is anything send to the Jaeger container at all on the specified UDP port and it seems like the program doesn't even send a message.

My project.csproj if it helps:

<Project Sdk="Microsoft.NET.Sdk">

  <PropertyGroup>
    <OutputType>Exe</OutputType>
    <TargetFramework>netcoreapp2.0</TargetFramework>
  </PropertyGroup>

  <ItemGroup>
    <PackageReference Include="Jaeger" Version="0.0.9" />
    <PackageReference Include="Microsoft.Extensions.Logging.Console" Version="2.0.1" />
    <PackageReference Include="OpenTracing" Version="0.11.0" />
  </ItemGroup>

</Project>

Any help is welcome!

Http Thrift requests without Content-Type headers are rejected

Requirement - what kind of business use case are you trying to solve?

Report Spans via Http Thrift requests (port 14268)

Problem - what in Jaeger blocks you from solving the requirement?

Recent versions of the Jaeger backend reject Http Thrift requests that do not contain a Content-Type: application/x-thrift header.
An Http 400 response is returned with the following message:
Cannot parse content type: mime: no media type

Proposal - what do you suggest to solve the problem or improve the existing situation?

Include the expected header in Http Thrift requests

Any open questions to address

Add Sub-millisecond timing precision

Requirement - what kind of business use case are you trying to solve?

Accurately timing spans at sub-millisecond resolution.

Problem - what in Jaeger blocks you from solving the requirement?

DateTimeExtensions.ToUnixTimeMicroseconds() first computes integer milliseconds, then scales to microseconds. This unnecessarily removes sub-millisecond precision from all microsecond measurement.

Proposal - what do you suggest to solve the problem or improve the existing situation?

Update DateTimeExtensions.ToUnixTimeMicroseconds to the following:

        private const long TicksPerMicrosecond = TimeSpan.TicksPerMillisecond / 1000; 
        private const long UnixEpochMicroseconds = UnixEpochTicks / TicksPerMicrosecond;

public static long ToUnixTimeMicroseconds(this DateTime utcTimestamp)
        {
            // Truncate sub-millisecond precision before offsetting by the Unix Epoch to avoid
            // the last digit being off by one for dates that result in negative Unix times
            long microseconds = utcTimestamp.Ticks / TicksPerMicrosecond;
            return microseconds - UnixEpochMicroseconds;
        }

Any open questions to address

No known open questions.
There is just one test JaegerThriftSpanConverterTests.TestConvertSpan() that assumes the implementation is Milliseconds * 1000 which implies that the loss of precision is a requirement to be tested for which I'm assuming is not actually the case.

transport configuration for web api sample

As far as I can see in the libary code, there aren't any "defaults" as to where to send traces (looking at the jaeger trhift transport class for instance). Also I cannot find any explicit definition of the required transport params in the example code, which leads me to believe that the example simply won't push traces anywhere. Is this right? It would be awesome with more explicit documentation around configuring the transport. It would also be good if debug-level stdout logging actually included endpoint info, such as sent trace xyz to endpoint:port using protocol <proto>

Thrift library has dependencies on Microsoft.AspNetCore.* and Microsoft.Extensions.* libraries

It seems like Thrift support for .NET is a huge mess. There's the original csharp code base and there's also a newer netcore code base. The latest csharp release is very old and doesn't target .NET Standard and the netcore project doesn't have any official NuGet releases yet.

This project currently has a local copy of the netcore project and it IL-merges it but even this project should not be used IMO because it has a a lot of weird non-standard dependencies - especially Microsoft.AspNetCore. This means that anyone who wants to use LetsTrace/Jaeger in a regular console app would get the whole ASP.NET Core stack DLLs as well - and this is not acceptable IMO.

We therefore need a different solution IMO. I can think of the following:

  • As LetsTrace/Jaeger is currently targeting netstandard2.0 we might be able to just reference the official Thrift package. They introduced this compatibility feature recently AFAIK. However, I'm not sure if the very old version of the official Thrift package still works for us.
  • Vendor-in the csharp project instead of netcore and have it target netstandard2.0. Ideally, this should work with minimal effort as .NET Standard 2.0 is very compatible to the full .NET framework. We would still have to IL-merge it - or we could also just release it under a different name and save us the pain of IL-merging. (e.g. Jaeger.Thrift.ThriftLib).
  • Try one of the existing forks of Thrift on NuGet - e.g. https://www.nuget.org/packages/Apache.Thrift/

Your thoughts?

Jaeger and the `Microsoft.Extensions.*` stack (DI, Configuration, Logging)

ASP.NET Core services are a one of the main targets of this library so it should be as easy as possible to set up a tracer there. We should therefore support the following modules/libraries:

  • Microsoft.Extensions.Configuration: A developer should be able to configure Jaeger from environment variables, config files, etc. - The Microsoft.Extensions.Configuration library supports many different input sources.
  • Microsoft.Extensions.DependencyInjection: A developer should be able to call services.AddJaeger().
  • Microsoft.Extensions.Logging: We already have a dependency on that.

With our already existing dependency on Microsoft.Extensions.Logging we are currently in a weird limbo state: People who do not use the new Microsoft.Extensions.* stack (e.g. ASP.NET WebForms) inherit many Microsoft.Extensions.* dependencies and can't easily consume the logs; people who already use the new Microsoft.Extensions.* stack (ASP.NET Core) get proper logging support but they don't get DI & Configuration support.

So the first thing I would like to discuss is if we should build this into the library (by adding additional dependencies) or if we want to move all Microsoft.Extensions.* stuff into a separate library:

Scenario 1: Making the library Microsoft.Extensions.*-native

Adding additional dependencies would make it a Microsoft.Extensions-"native" library like e.g. Entity Framework Core, Identity Server. This would make it very easy to use Jaeger with ASP.NET Core (and any hosting framework that supports that stack) as they only have to add the main Jaeger package and call services.AddJaeger() and everything else will pretty much work automatically.

But this obviously also limits its usage possibilities as it makes it much harder to use it in other scenarios.

Scenario 2: Adding a new Jaeger.Microsoft.Extensions library

Thanks to the Builder types, we already have a decent initialization story, so it's already quite easy to use the library without dependencies on Microsoft.Extensions.*. One thing we don't yet have though (not even with #55 ) is Javas Configuration.java type that supports configuration via environment variables. I did not port this because I wanted to have this discussion first.

To make it easier to use the library in ASP.NET Core, we could add a Jaeger.Microsoft.Extensions library (other naming suggestions welcome!) and provide the support for DI & Configuration there. The drawback with this is that people have to know about this additional library.

If we go this way, we should also discuss whether or not we want to keep the Microsoft.Extensions.Logging dependency in the main library. Unfortunately, there's no "one global logging abstraction" in .NET so every alternative has drawbacks as well. Using the dependency-free LibLog doesn't support Microsoft.Extensions.Logging and using our own interfaces requires people to build adapters. My proposal would be to just change the dependency to Microsoft.Extensions.Logging.Abstractions for now as that doesn't bring in any other Microsoft.Extensions.* dependencies. (Libraries should only rely on the Microsoft.Extensions.*.Abstractions libraries anyway)

To further illustrate this scenario, I have some initial code for this in the cweiss/MsExtensions-branch (it's already based on #55).


I would favor scenario 2 with the separate library. This would ensure that the main Jaeger library can stay as close as possible to the Java code and that Jaeger can also be used properly outside of ASP.NET Core.

Your thoughts?

Add a build script for local execution and use it in AppVeyor

Would be nice to have a build.ps1 file which restores/builds/packs/tests locally. This would ensure we can submit PRs that don't fail on AppVeyor. AppVeyor should then also only call this script.

I'd be happy to send a PR with the script I'm using for several projects - e.g. in opentracing-csharp if you like it. It's a simple script that doesn't have any dependencies on e.g. psake, cake, ... but still delivers a nice output. It's also faster than the current AppVeyor script as it does restore/build/pack in one call.

Self referencing loop when using LoggingReporter

With the dropping of IJaegerCoreSpan in #55, the JsonIgnore attribute from Tracer was removed. This is needed to resolve a referencing loop in JsonConvert used by LoggingReporter. Right now, logging throws this exception:

Newtonsoft.Json.JsonSerializationException: 'Self referencing loop detected for property 'Span' with type 'Jaeger.Span'. Path 'Tracer.ScopeManager.Active'.'

This happens, when we are logging the active Span when finishing a scope from StartActive. We have two options to resolve this.

This can be easily fixed by adding the JsonIgnore attribute to the Tracer property in the Span implementation.

Reporting spans with JaegerHttpTransport fails with ObjectDisposedException

This seems to happen after a few sends or after a few seconds. I was using a batch size of 3.

I will probably have time to look at this in the next 1-3 days but if anyone wants to jump in, feel free to do so and just let me know in the comments.

fail: Jaeger.Core.Reporters.RemoteReporter[0]
      Unable to report span in RemoteReporter
Thrift.Transports.TTransportException: Couldn't connect to server: System.Net.Http.HttpRequestException: Error while copying content to a stream. ---> System.ObjectDisposedException: Cannot access a closed Stream.
   at System.IO.StreamHelpers.ValidateCopyToArgs(Stream source, Stream destination, Int32 bufferSize)
   at System.IO.MemoryStream.CopyToAsync(Stream destination, Int32 bufferSize, CancellationToken cancellationToken)
   at System.Net.Http.StreamToStreamCopy.CopyAsync(Stream source, Stream destination, Int32 bufferSize, Boolean disposeSource, CancellationToken cancellationToken)
--- End of stack trace from previous location where exception was thrown ---
   at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
   at System.Net.Http.HttpContent.<CopyToAsyncCore>d__44.MoveNext()
   --- End of inner exception stack trace ---
   at System.Net.Http.HttpContent.<CopyToAsyncCore>d__44.MoveNext()
--- End of stack trace from previous location where exception was thrown ---
   at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
   at System.Net.Http.WinHttpHandler.<InternalSendRequestBodyAsync>d__131.MoveNext()
--- End of stack trace from previous location where exception was thrown ---
   at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
   at System.Net.Http.WinHttpHandler.<StartRequest>d__105.MoveNext()
--- End of stack trace from previous location where exception was thrown ---
   at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
   at System.Runtime.CompilerServices.ConfiguredTaskAwaitable`1.ConfiguredTaskAwaiter.GetResult()
   at System.Net.Http.DiagnosticsHandler.<SendAsync>d__2.MoveNext()
--- End of stack trace from previous location where exception was thrown ---
   at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
   at System.Runtime.CompilerServices.ConfiguredTaskAwaitable`1.ConfiguredTaskAwaiter.GetResult()
   at System.Net.Http.HttpClient.<FinishSendAsyncBuffered>d__58.MoveNext()
--- End of stack trace from previous location where exception was thrown ---
   at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
   at Thrift.Transports.Client.THttpClientTransport.<FlushAsync>d__21.MoveNext() in C:\projects\jaeger-client-csharp\src\Thrift\netcore\Thrift\Transports\Client\THttpClientTransport.cs:line 178
   at Thrift.Transports.Client.THttpClientTransport.<FlushAsync>d__21.MoveNext() in C:\projects\jaeger-client-csharp\src\Thrift\netcore\Thrift\Transports\Client\THttpClientTransport.cs:line 201
--- End of stack trace from previous location where exception was thrown ---
   at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
   at Jaeger.Transport.Thrift.Transport.Sender.JaegerThriftHttpSender.<SendAsync>d__2.MoveNext() in C:\projects\jaeger-client-csharp\src\Jaeger.Transport.Thrift\Transport\Sender\JaegerThriftHTTPSender.cs:line 32
--- End of stack trace from previous location where exception was thrown ---
   at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
   at Jaeger.Transport.Thrift.Transport.Sender.JaegerSender.<FlushAsync>d__9.MoveNext() in C:\projects\jaeger-client-csharp\src\Jaeger.Transport.Thrift\Transport\Sender\JaegerSender.cs:line 57
--- End of stack trace from previous location where exception was thrown ---
   at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
   at Jaeger.Transport.Thrift.Transport.JaegerHttpTransport.<ProtocolAppendLogicAsync>d__8.MoveNext() in C:\projects\jaeger-client-csharp\src\Jaeger.Transport.Thrift\Transport\JaegerHTTPTransport.cs:line 67
--- End of stack trace from previous location where exception was thrown ---
   at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
   at Jaeger.Transport.Thrift.Transport.JaegerThriftTransport.<AppendAsync>d__5.MoveNext() in C:\projects\jaeger-client-csharp\src\Jaeger.Transport.Thrift\Transport\JaegerThriftTransport.cs:line 56
--- End of stack trace from previous location where exception was thrown ---
   at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
   at Jaeger.Core.Reporters.RemoteReporter.<Report>d__5.MoveNext() in C:\projects\jaeger-client-csharp\src\Jaeger.Core\Reporters\RemoteReporter.cs:line 46

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

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 jaegertracing/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.

Publish NuGet for new name

We need to get the package published under it's new name(s) in NuGet. I know Jaeger.Thrift and Jaeger.Thrift.VendoredThrift will require a transfer of ownership on NuGet. @yurishkuro my last day at work is tomorrow. Any chance we could get this setup before the end of day on 4/6/2018?

Recommend Projects

  • React photo React

    A declarative, efficient, and flexible JavaScript library for building user interfaces.

  • Vue.js photo Vue.js

    πŸ–– Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.

  • Typescript photo Typescript

    TypeScript is a superset of JavaScript that compiles to clean JavaScript output.

  • TensorFlow photo TensorFlow

    An Open Source Machine Learning Framework for Everyone

  • Django photo Django

    The Web framework for perfectionists with deadlines.

  • D3 photo D3

    Bring data to life with SVG, Canvas and HTML. πŸ“ŠπŸ“ˆπŸŽ‰

Recommend Topics

  • javascript

    JavaScript (JS) is a lightweight interpreted programming language with first-class functions.

  • web

    Some thing interesting about web. New door for the world.

  • server

    A server is a program made to process requests and deliver data to clients.

  • Machine learning

    Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.

  • Game

    Some thing interesting about game, make everyone happy.

Recommend Org

  • Facebook photo Facebook

    We are working to build community through open source technology. NB: members must have two-factor auth.

  • Microsoft photo Microsoft

    Open source projects and samples from Microsoft.

  • Google photo Google

    Google ❀️ Open Source for everyone.

  • D3 photo D3

    Data-Driven Documents codes.