Jaeger exporter implementation #132

EGHornbostel · 2019-07-03T00:26:38Z

This PR creates a Jaeger exporter which adds support for CNCF Jaeger.

EGHornbostel · 2019-07-03T03:22:26Z

Not sure what's going on. I can't seem to get this build to work on windows... fails for me even on the master branch.

EGHornbostel · 2019-07-03T04:58:13Z

OK... figured out why I couldn't compile under windows. Seems like SourceLink isn't happy on my windows machine.

Anyway, after removing that reference so I could compile, I found out why the dotNet (windows) build is failing. Despite what they advertise, the Thrift library I'm referencing doesn't really work with dotNet Framework 4.6... so I need to figure out a workaround.

EGHornbostel · 2019-07-03T20:38:48Z

I've mentioned this a little in the Gitter chat, but in order to make the Jaeger Exporter work with both .net Framework 4.6 and dotnet standard 2, I had to pull in the Thrift source and include it in a /lib folder so that I could conditionally add System.Net.Http when compiling under Windows. see: https://github.com/open-telemetry/opentelemetry-dotnet/blob/0d872b48ce599eab896a2ea01c912a6097cf8520/lib/Thrift/Thrift.csproj

Does anyone have any thoughts on possible alternative ways to make this work?

lmolkova · 2019-07-04T04:43:22Z

lib/Thrift/Thrift.csproj

+
+  <PropertyGroup>
+    <TargetFrameworks>net46;netstandard2.0</TargetFrameworks>
+    <TargetFrameworks Condition="$(OS) != 'Windows_NT'">netstandard2.0</TargetFrameworks>


Suggested change

<TargetFrameworks Condition="$(OS) != 'Windows_NT'">netstandard2.0</TargetFrameworks>

Nitpicking here but I believe keeping the current value of TargetFrameworks and appending if condition is true makes a bit more sense:

<PropertyGroup> <TargetFrameworks>netstandard2.0</TargetFrameworks> </PropertyGroup> <PropertyGroup Condition="'$(OS)' == 'Windows_NT'"> <TargetFrameworks>net46;$(TargetFrameworks)</TargetFrameworks> </PropertyGroup>

The reason being, when we add netstandard2.1, you just need to add to the top TargetFrameworks/add only once.

Although I'm not sure what's the reason to not compile for net46 on non Windows. ReferenceAssemblies can be used for that. Tests can run on Mono.

OK... figured out why I couldn't compile under windows. Seems like SourceLink isn't happy on my windows machine.

could you please share what is the problem? I don't have issues on my windows machine and I wonder if some prerequsites are missing from our contributor's guide.

I was getting this error:

The "Microsoft.Build.Tasks.Git.LocateRepository" task failed unexpectedly.

I can't give you specifics right now, since I just realized that I left my external drive that contains my windows image at home. I'll update this note with a more detailed error when I get home.

here dotnet/sourcelink#107 they tell "SourceLink requires .NET Core 2.1.300."

Do you have it installed on your machine? this version or later? Can you please check?

here dotnet/sourcelink#107 they tell "SourceLink requires .NET Core 2.1.300."

Do you have it installed on your machine? this version or later? Can you please check?

Hi, yeah, I actually checked that the first thing when I switched over to Windows.

I just finally figured it out. I didn't have Git installed on my Windows VM because I've been managing source control through my MacOS host. I didn't even catch this when tried to clone the repo, since I cloned it using Visual Studio 2019's "Clone or check out code" feature, and Visual Studio 2019 is able to clone without Git installed.

So, it may seem like a silly thing to add to the CONTRIBUTING.md, but we might want to add a line item to install the Git command line tools. Especially since Visual Studio 2019 can now interact with Git without the tools installed.

lmolkova · 2019-07-04T04:44:49Z

lib/Thrift/Thrift.csproj

+    <TargetFrameworks Condition="$(OS) != 'Windows_NT'">netstandard2.0</TargetFrameworks>
+  </PropertyGroup>
+
+  <ItemGroup Condition="$(OS) == 'Windows_NT'">


change it to

<ItemGroup Condition="'$(TargetFramework)' == 'net46'"> <Reference Include="System.Net.Http" /> </ItemGroup>

otherwise it bring .NET Core nuget package, while what we need is the lib that is part of installed .NET on 4.6

lmolkova · 2019-07-04T04:45:59Z

src/OpenTelemetry.Exporter.Jaeger/JaegerExporter.cs

+            this.isInitialized = false;
+        }
+
+        // TODO: override a finalizer only if Dispose(bool disposing) above has code to free unmanaged resources.


is it still WIP or TODOs can be cleaned up? :)

I can clean them up. I was thinking to leave it as WIP, but it's unnecessary. It's the standard dispose pattern that anyone should know what to do with.

lmolkova · 2019-07-04T04:52:13Z

've mentioned this a little in the Gitter chat, but in order to make the Jaeger Exporter work with both .net Framework 4.6 and dotnet standard 2, I had to pull in the Thrift source and include it in a /lib folder so that I could conditionally add System.Net.Http when compiling under Windows. see: https://github.com/open-telemetry/opentelemetry-dotnet/blob/0d872b48ce599eab896a2ea01c912a6097cf8520/lib/Thrift/Thrift.csproj

Does anyone have any thoughts on possible alternative ways to make this work?

I'm surprised that Thrift is not available on nuget packages. Is it correct that there is no 'official' source for this packages? Also, do I understand correctly that thrift doesn't change much? I.e. existing implementation won't require changes, correct?

I think in this case including source code and compiling it the only reasonable option.
Can you leave some comments saving where you took this code from? Commit id?
So if it breaks, at least we'll know where to check for possible fixes.

Also, there are not many tests written for exporters and it is going to hunt us down eventually. Could you please write some unit tests and maybe some integration test, so when we change things (in API e.g. ) we know we didn't break the exported.

lmolkova · 2019-07-04T04:53:22Z

OK... figured out why I couldn't compile under windows. Seems like SourceLink isn't happy on my windows machine.

could you please share what is the problem? I don't have issues on my windows machine and I wonder if some prerequsites are missing from our contributor's guide.

lmolkova

Thanks a lot for the contribution! I've left some comments - let's discuss.

bruno-garcia

Left a couple notes. I hope you find them helpful.

bruno-garcia · 2019-07-04T09:05:49Z

lib/Thrift/Thrift.csproj

+
+  <PropertyGroup>
+    <TargetFrameworks>net46;netstandard2.0</TargetFrameworks>
+    <TargetFrameworks Condition="$(OS) != 'Windows_NT'">netstandard2.0</TargetFrameworks>


Nitpicking here but I believe keeping the current value of TargetFrameworks and appending if condition is true makes a bit more sense:

<PropertyGroup> <TargetFrameworks>netstandard2.0</TargetFrameworks> </PropertyGroup> <PropertyGroup Condition="'$(OS)' == 'Windows_NT'"> <TargetFrameworks>net46;$(TargetFrameworks)</TargetFrameworks> </PropertyGroup>

The reason being, when we add netstandard2.1, you just need to add to the top TargetFrameworks/add only once.

Although I'm not sure what's the reason to not compile for net46 on non Windows. ReferenceAssemblies can be used for that. Tests can run on Mono.

lib/Thrift/Transports/Client/TStreamClientTransport.cs

bruno-garcia · 2019-07-04T09:09:06Z

lib/Thrift/Transports/Client/TStreamClientTransport.cs

+            return await InputStream.ReadAsync(buffer, offset, length, cancellationToken);
+        }
+
+        public override async Task WriteAsync(byte[] buffer, int offset, int length, CancellationToken cancellationToken)


Suggested change

public override async Task WriteAsync(byte[] buffer, int offset, int length, CancellationToken cancellationToken)

public override Task WriteAsync(byte[] buffer, int offset, int length, CancellationToken cancellationToken)

bruno-garcia · 2019-07-04T09:09:17Z

lib/Thrift/Transports/Client/TStreamClientTransport.cs

+                    "Cannot read from null inputstream");
+            }
+
+            await OutputStream.WriteAsync(buffer, offset, length, cancellationToken);


Suggested change

await OutputStream.WriteAsync(buffer, offset, length, cancellationToken);

return OutputStream.WriteAsync(buffer, offset, length, cancellationToken);

bruno-garcia · 2019-07-04T09:11:21Z

lib/Thrift/Transports/Server/TNamedPipeServerTransport.cs

+                }
+            }
+
+            public override void Close()


nit: Could be single liner with expression bodied member:

Suggested change

public override void Close()

public override void Close() => _stream?.Dispose();

bruno-garcia · 2019-07-04T09:12:35Z

lib/Thrift/Transports/Server/TServerFramedTransport.cs

+                }
+                catch (SocketException sx)
+                {
+                    throw new TTransportException("Could not accept on listening socket: " + sx.Message);


Does TTransportException take the inner exception as a second argument?

bruno-garcia · 2019-07-04T09:13:41Z

lib/Thrift/Transports/Server/TServerFramedTransport.cs

+            try
+            {
+                TFramedClientTransport tSocketTransport = null;
+                var tcpClient = await _server.AcceptTcpClientAsync();


I believe in libraries we should still be using ConfigureAwait(false) to save someone calling .Result and deadlocking.

@bruno-garcia done... did a massive regex search and replace to fix the missing ConfigureAwait(false) all over the Thrift code.

bruno-garcia · 2019-07-04T09:17:46Z

lib/Thrift/Transports/TTransportFactory.cs

+    // ReSharper disable once InconsistentNaming
+    public class TTransportFactory
+    {
+        public virtual TClientTransport GetTransport(TClientTransport trans)


All these methods with a single return or single assignment could be expression bodied member:

Suggested change

public virtual TClientTransport GetTransport(TClientTransport trans)

public virtual TClientTransport GetTransport(TClientTransport trans) => trans;

src/OpenTelemetry.Exporter.Jaeger/Implimentation/EmitBatchArgs.cs

lmolkova · 2019-07-08T17:58:21Z

lib/Thrift/Thrift.csproj

-  <ItemGroup Condition="$(OS) == 'Windows_NT'">
-    <PackageReference Include="System.Net.Http" Version="4.3.4" />
+  <ItemGroup Condition="'$(TargetFramework)' == 'net46'">
+    <Reference Include="System.Net.Http" Version="4.3.4" />


please remove version, it is not needed

<Reference Include="System.Net.Http" />

lmolkova · 2019-07-08T17:58:47Z

lib/Thrift/Thrift.csproj

@@ -5,8 +5,8 @@
    <TargetFrameworks Condition="$(OS) != 'Windows_NT'">netstandard2.0</TargetFrameworks>


<TargetFrameworks Condition="$(OS) != 'Windows_NT'">netstandard2.0</TargetFrameworks>

is not needed

Actually, after removing this and compiling, the code fails in .net standard 2.0. I double checked and this is the same pattern we're using in all the other projects in the solution. Should we be doing something else?

Correction: it doesn't fail in .net standard 2.0, it fails on any non-windows machine that cannot host .net Framework 4.5. The build command sees all the target frameworks and attempts to build them all. Since Framework 4.5 is not available, the framework 4.5 part of the build fails.

Sample output:

/usr/local/share/dotnet/sdk/2.2.300/Microsoft.Common.CurrentVersion.targets(1175,5): error MSB3644: The reference assemblies for framework ".NETFramework,Version=v4.6" were not found. To resolve this, install the SDK or Targeting Pack for this framework version or retarget your application to a version of the framework for which you have the SDK or Targeting Pack installed. Note that assemblies will be resolved from the Global Assembly Cache (GAC) and will be used in place of reference assemblies. Therefore your assembly may not be correctly targeted for the framework you intend. [/Users/eg.hornbostel/dev/opentelemetry-dotnet/lib/Thrift/Thrift.csproj] 14 Warning(s) 1 Error(s)

ok, i see, so you are building on Mac i guess and this protects your build. Sorry for misleading advice, please put it back.

EGHornbostel · 2019-07-08T18:02:55Z

I'm surprised that Thrift is not available on nuget packages. Is it correct that there is no 'official' source for this packages? Also, do I understand correctly that thrift doesn't change much? I.e. existing implementation won't require changes, correct?

Thrift is available as a NuGet package, but it doesn't appear to work correctly in Framework 4.6

I actually created this fork from another fork in Uber's own source code for Jaeger's client.

And you are correct. As far as I could tell, Thrift hasn't changed much in the last few years.

I think in this case including source code and compiling it the only reasonable option.
Can you leave some comments saving where you took this code from? Commit id?
So if it breaks, at least we'll know where to check for possible fixes.

I took the code from Jaeger's source:

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

commit ID

0794ea71cb6e58f7bf0f0ef2c0c8ceceb1d8b6d9

Path to their Thrift fork is
src/Thrift

Also, there are not many tests written for exporters and it is going to hunt us down eventually. Could you please write some unit tests and maybe some integration test, so when we change things (in API e.g. ) we know we didn't break the exported.

Agreed. I'll start working on unit tests.

Do we have any examples of good integration tests in this solution that I can use as a guide?

Co-Authored-By: Bruno Garcia <[email protected]>

EGHornbostel · 2019-07-08T19:25:42Z

@lmolkova I've switched the project from using the NuGet version of System.Net.Http over to the direct Framework reference. However, I'm now getting a new build issue on windows.

It appears that Thrift needs access to HttpClientHandler.ClientCertificates This doesn't exist in .net Framework 4.6, but only in Framework 4.7 and above (see https://docs.microsoft.com/en-us/dotnet/api/system.net.http.httpclienthandler.clientcertificates?view=netframework-4.8). The nuget package seems to be the only thing that works without code changes.

If we would like to consider a code change, it would require vastly rewriting this Thrift class to remove the dependency with HttpClientHandler and rewrite it to use HttpWebRequest instead.

Thoughts?

Co-Authored-By: Bruno Garcia <[email protected]>

EGHornbostel · 2019-07-20T22:56:17Z

@lmolkova I found some bare-bones integration tests with the UDP transport in the Jaeger client. I ported it over. I also created my own test for whether or not the Thrift payload is encoded consistently given the same exact data. Is this enough?

EGHornbostel · 2019-07-20T22:57:06Z

@bruno-garcia I've made changes based on your input. Thanks very much for your help. You've been most helpful to a guy who's been away from the C# game for 5 years.

megakid · 2019-07-23T11:25:25Z

src/OpenTelemetry.Exporter.Jaeger/Implimentation/Batch.cs

+// limitations under the License.
+// </copyright>
+
+namespace OpenTelemetry.Exporter.Jaeger.Implimentation


Namespace Typo: Implimentation -> Implementation

austinlparker · 2019-07-23T16:51:30Z

This looks OK overall, could you fix the merge conflict on the sln? I'll hold off on more merges to keep things stable.

EGHornbostel · 2019-07-23T22:27:47Z

This looks OK overall, could you fix the merge conflict on the sln? I'll hold off on more merges to keep things stable.

.sln merge conflicts are the best.

austinlparker · 2019-07-24T02:09:42Z

@lmolkova wanted to make sure you were cool with merging this since you requested changes

…ting to a client or duplex streaming service fails (open-telemetry#132) Any exceptions during a write to a clientstreaming or duplexstreaming RPC are not tracked. They are bubbling up to the caller and we are losing the opportunity to record the correct status code and properly stop the activity. Before this change, the newly added tests would fail with a status code of (1) cancelled which was getting set during Dispose instead of proactively at the time of the failure.

EGHornbostel added 2 commits July 2, 2019 17:15

Fix visual studio dotnet core build task

639cd23

Added CNCF Jaeger exporter support

6d37ed2

open-telemetry#77

EGHornbostel requested review from austinlparker, discostu105, lmolkova and SergeyKanzhelev as code owners July 3, 2019 00:26

possible fix for win32 build issue.

8e45715

EGHornbostel added 3 commits July 3, 2019 09:13

Fix windows build by adding Thrift source

86db023

Fix yet more windows build issues.

dd4492b

fix another style issue.

0d872b4

lmolkova reviewed Jul 4, 2019

View reviewed changes

lmolkova requested changes Jul 4, 2019

View reviewed changes

bruno-garcia suggested changes Jul 4, 2019

View reviewed changes

fix reference to System.Net.Http

6fd13b8

lmolkova reviewed Jul 8, 2019

View reviewed changes

EGHornbostel and others added 4 commits July 8, 2019 11:05

remove unnecessary conditional framework reference.

ac5561a

Update lib/Thrift/Transports/Client/TStreamClientTransport.cs

7b82349

Co-Authored-By: Bruno Garcia <[email protected]>

Update lib/Thrift/Transports/Client/TStreamClientTransport.cs

5ec204c

Co-Authored-By: Bruno Garcia <[email protected]>

Added the conditional for netstandard2.0

01f4283

remove IDisposable TODO comments.

41c8151

EGHornbostel and others added 11 commits July 15, 2019 18:04

WIP cleanup after timestamp removal.

b4c5f5b

Merge remote-tracking branch 'upstream/master' into jaeger-exporter

6ded7ba

WIP

8620491

WIP UDP integration test

b18f778

Update src/OpenTelemetry.Exporter.Jaeger/Implimentation/Batch.cs

604e747

Co-Authored-By: Bruno Garcia <[email protected]>

remove solution-explorer

d4bd960

add to gitignore

c8a706a

refactored List<T> to IEnumerable<T>

011cd04

Fix unit test bug

4a4a592

Add integration test

0341811

cleanup

e2030e7

EGHornbostel added 3 commits July 20, 2019 19:49

fix unix epoch conversion

940fce7

Update Thrift expected payload in unit test.

7a98559

fix build issues

9fa75eb

megakid reviewed Jul 23, 2019

View reviewed changes

EGHornbostel added 3 commits July 23, 2019 15:31

fix typo

eb1b880

Merge remote-tracking branch 'upstream/master' into jaeger-exporter

4408d98

fix solution

4d27578

austinlparker approved these changes Jul 24, 2019

View reviewed changes

lmolkova approved these changes Jul 24, 2019

View reviewed changes

Merge branch 'master' into jaeger-exporter

c333fb6

austinlparker merged commit 8e1d8fa into open-telemetry:master Jul 24, 2019

bruno-garcia mentioned this pull request Jul 24, 2019

Support for CNCF Jaeger #77

Closed

EGHornbostel deleted the jaeger-exporter branch July 24, 2019 20:31

	public override async Task WriteAsync(byte[] buffer, int offset, int length, CancellationToken cancellationToken)
	public override Task WriteAsync(byte[] buffer, int offset, int length, CancellationToken cancellationToken)

	await OutputStream.WriteAsync(buffer, offset, length, cancellationToken);
	return OutputStream.WriteAsync(buffer, offset, length, cancellationToken);

	public override void Close()
	public override void Close() => _stream?.Dispose();

	public virtual TClientTransport GetTransport(TClientTransport trans)
	public virtual TClientTransport GetTransport(TClientTransport trans) => trans;

		@@ -5,8 +5,8 @@
		<TargetFrameworks Condition="$(OS) != 'Windows_NT'">netstandard2.0</TargetFrameworks>

Jaeger exporter implementation #132

Jaeger exporter implementation #132

Conversation

EGHornbostel commented Jul 3, 2019

EGHornbostel commented Jul 3, 2019

EGHornbostel commented Jul 3, 2019 • edited Loading

EGHornbostel commented Jul 3, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

EGHornbostel Jul 8, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lmolkova commented Jul 4, 2019

lmolkova commented Jul 4, 2019

lmolkova left a comment

Choose a reason for hiding this comment

bruno-garcia left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

EGHornbostel commented Jul 8, 2019 • edited Loading

EGHornbostel commented Jul 8, 2019 • edited Loading

EGHornbostel commented Jul 20, 2019

EGHornbostel commented Jul 20, 2019

Choose a reason for hiding this comment

austinlparker commented Jul 23, 2019

EGHornbostel commented Jul 23, 2019

austinlparker commented Jul 24, 2019

EGHornbostel commented Jul 3, 2019 •

edited

Loading

EGHornbostel commented Jul 3, 2019 •

edited

Loading

EGHornbostel Jul 8, 2019 •

edited

Loading

EGHornbostel commented Jul 8, 2019 •

edited

Loading

EGHornbostel commented Jul 8, 2019 •

edited

Loading