Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jaeger exporter implementation #132

Merged
merged 56 commits into from
Jul 24, 2019
Merged

Jaeger exporter implementation #132

Changes from 1 commit
Commits
Show all changes
56 commits
Select commit Hold shift + click to select a range
639cd23
Fix visual studio dotnet core build task
EGHornbostel Jul 3, 2019
6d37ed2
Added CNCF Jaeger exporter support
EGHornbostel Jul 3, 2019
8e45715
possible fix for win32 build issue.
EGHornbostel Jul 3, 2019
86db023
Fix windows build by adding Thrift source
EGHornbostel Jul 3, 2019
dd4492b
Fix yet more windows build issues.
EGHornbostel Jul 3, 2019
0d872b4
fix another style issue.
EGHornbostel Jul 3, 2019
6fd13b8
fix reference to System.Net.Http
EGHornbostel Jul 8, 2019
ac5561a
remove unnecessary conditional framework reference.
EGHornbostel Jul 8, 2019
7b82349
Update lib/Thrift/Transports/Client/TStreamClientTransport.cs
EGHornbostel Jul 8, 2019
5ec204c
Update lib/Thrift/Transports/Client/TStreamClientTransport.cs
EGHornbostel Jul 8, 2019
01f4283
Added the conditional for netstandard2.0
EGHornbostel Jul 8, 2019
41c8151
remove IDisposable TODO comments.
EGHornbostel Jul 8, 2019
82eddea
Update src/OpenTelemetry.Exporter.Jaeger/Implimentation/EmitBatchArgs.cs
EGHornbostel Jul 9, 2019
1098705
Add readme with thrift fork info.
EGHornbostel Jul 9, 2019
fcab9be
Merge branch 'master' into jaeger-exporter
EGHornbostel Jul 9, 2019
ba897aa
cleanup after merge.
EGHornbostel Jul 9, 2019
bbbcf47
WIP remove thrift code
EGHornbostel Jul 10, 2019
fa5fbd8
Use prepackaged Apache Thrift assembly
EGHornbostel Jul 11, 2019
9aaa138
wrap EmitBatch
EGHornbostel Jul 11, 2019
ac65edf
Revert "wrap EmitBatch "
EGHornbostel Jul 11, 2019
645d444
Revert "Use prepackaged Apache Thrift assembly"
EGHornbostel Jul 11, 2019
c368e2e
Revert "Use prepackaged Apache Thrift assembly"
EGHornbostel Jul 11, 2019
93c845e
Revert "WIP remove thrift code"
EGHornbostel Jul 10, 2019
89840a2
Add missing ConfigureAwait(false) to Thrift async calls.
EGHornbostel Jul 11, 2019
c22d3db
Added Thrift fork differences to README.md
EGHornbostel Jul 11, 2019
010dd08
WIP Jaeger Unit tests
EGHornbostel Jul 12, 2019
3a47d48
Merge remote-tracking branch 'upstream/master' into jaeger-exporter
EGHornbostel Jul 12, 2019
2d8401e
Fix build after merge.
EGHornbostel Jul 12, 2019
b831f6e
Merge branch 'master' into jaeger-exporter
SergeyKanzhelev Jul 12, 2019
b6e3720
WIP Jaeger Unit tests - Span conversion to Jaeger Thrift format
EGHornbostel Jul 12, 2019
3b06692
WIP Jaeger Unit tests - Span conversion to Jaeger Thrift format - cle…
EGHornbostel Jul 12, 2019
3188066
Merge remote-tracking branch 'upstream/master' into jaeger-exporter
EGHornbostel Jul 12, 2019
f56e7e4
Clean up merge issues
EGHornbostel Jul 12, 2019
916f223
WIP
EGHornbostel Jul 12, 2019
5f98892
WIP cleanup
EGHornbostel Jul 12, 2019
1cc3e83
more testing WIP
EGHornbostel Jul 13, 2019
aff702e
use pattern matching for attribute conversion
EGHornbostel Jul 15, 2019
e4f9b12
Merge remote-tracking branch 'upstream/master' into jaeger-exporter
EGHornbostel Jul 15, 2019
b4c5f5b
WIP cleanup after timestamp removal.
EGHornbostel Jul 16, 2019
6ded7ba
Merge remote-tracking branch 'upstream/master' into jaeger-exporter
EGHornbostel Jul 17, 2019
8620491
WIP
EGHornbostel Jul 20, 2019
b18f778
WIP UDP integration test
EGHornbostel Jul 20, 2019
604e747
Update src/OpenTelemetry.Exporter.Jaeger/Implimentation/Batch.cs
EGHornbostel Jul 20, 2019
d4bd960
remove solution-explorer
EGHornbostel Jul 20, 2019
c8a706a
add to gitignore
EGHornbostel Jul 20, 2019
011cd04
refactored List<T> to IEnumerable<T>
EGHornbostel Jul 20, 2019
4a4a592
Fix unit test bug
EGHornbostel Jul 20, 2019
0341811
Add integration test
EGHornbostel Jul 20, 2019
e2030e7
cleanup
EGHornbostel Jul 20, 2019
940fce7
fix unix epoch conversion
EGHornbostel Jul 21, 2019
7a98559
Update Thrift expected payload in unit test.
EGHornbostel Jul 21, 2019
9fa75eb
fix build issues
EGHornbostel Jul 21, 2019
eb1b880
fix typo
EGHornbostel Jul 23, 2019
4408d98
Merge remote-tracking branch 'upstream/master' into jaeger-exporter
EGHornbostel Jul 23, 2019
4d27578
fix solution
EGHornbostel Jul 23, 2019
c333fb6
Merge branch 'master' into jaeger-exporter
austinlparker Jul 24, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions lib/Thrift/Thrift.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,8 @@
<TargetFrameworks Condition="$(OS) != 'Windows_NT'">netstandard2.0</TargetFrameworks>
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@EGHornbostel EGHornbostel Jul 8, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

is not needed

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

</PropertyGroup>

<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" />
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please remove version, it is not needed

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

</ItemGroup>

</Project>