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.

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

[DOC] Update Jaeger tutorial to use OTLP Exporter #3940

Merged
merged 7 commits into from
Nov 30, 2022

Conversation

hgaol
Copy link
Contributor

@hgaol hgaol commented Nov 24, 2022

For #3925 , Update Jaeger tutorial to use OTLP Exporter.

Changes

Please provide a brief description of the changes here.

  • Updated the README.md for Jaeger tutorial
  • Updated Program.cs for Jaeger tutorial.
  • Added Jaeger tutorial example project into OT solution.

For significant contributions please make sure you have completed the following items:

  • Appropriate CHANGELOG.md updated for non-trivial changes
  • Design discussion issue #
  • Changes in public API reviewed

@hgaol hgaol requested a review from a team November 24, 2022 06:56
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Nov 24, 2022

CLA Signed

The committers listed above are authorized under a signed CLA.

  • ✅ login: hgaol / name: Han Gao (6b43138)

Copy link
Member

@reyang reyang left a comment

Choose a reason for hiding this comment

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

Looks solid 👍

@hgaol
Copy link
Contributor Author

hgaol commented Nov 28, 2022

Hi @reyang and @Kielek , thanks for your review and approve this PR. Since there're some errors in CI about HttpClient not found for net472/net48, I've submitted a new commit and updated the .csproj file like this. Don't know if it's a good practice or I can just revert the changes about adding the getting-started-jeager project to the solution.

failed CI action: https://github.com/open-telemetry/opentelemetry-dotnet/actions/runs/3538390359

@codecov
Copy link

codecov bot commented Nov 28, 2022

Codecov Report

Merging #3940 (4bc5d74) into main (e376aef) will increase coverage by 0.06%.
The diff coverage is n/a.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #3940      +/-   ##
==========================================
+ Coverage   85.18%   85.25%   +0.06%     
==========================================
  Files         287      287              
  Lines       11026    11026              
==========================================
+ Hits         9393     9400       +7     
+ Misses       1633     1626       -7     
Impacted Files Coverage Δ
...ZPages/Implementation/ZPagesExporterEventSource.cs 56.25% <0.00%> (-6.25%) ⬇️
...tpListener/Internal/PrometheusCollectionManager.cs 73.62% <0.00%> (-2.20%) ⬇️
src/OpenTelemetry/BatchExportProcessor.cs 84.11% <0.00%> (+1.86%) ⬆️
...nTelemetry/Internal/OpenTelemetrySdkEventSource.cs 82.53% <0.00%> (+2.38%) ⬆️
...ter.ZPages/Implementation/ZPagesActivityTracker.cs 100.00% <0.00%> (+2.85%) ⬆️
...lemetry/Internal/SelfDiagnosticsConfigRefresher.cs 89.42% <0.00%> (+2.88%) ⬆️
...emetry.Api/Internal/OpenTelemetryApiEventSource.cs 82.35% <0.00%> (+2.94%) ⬆️

@hgaol
Copy link
Contributor Author

hgaol commented Nov 29, 2022

rebased all main commits. I've tested locally and think/hope there's no CI error..

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Nov 29, 2022

CLA Signed

The committers listed above are authorized under a signed CLA.

@reyang
Copy link
Member

reyang commented Nov 30, 2022

@open-telemetry/dotnet-maintainers this looks good to merge.

<ProjectReference Include="$(RepoRoot)\src\OpenTelemetry.Instrumentation.Http\OpenTelemetry.Instrumentation.Http.csproj" />
</ItemGroup>
<ItemGroup>
<PackageReference Include="System.Net.Http" Version="4.3.4" Condition="$(TargetFramework) == 'net472' or $(TargetFramework) == 'net48'" />
Copy link
Member

Choose a reason for hiding this comment

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

@alanwest @utpilla This change piqued my interest. We don't specify targets explicitly. When I build this project locally I get output for net7.0, net6.0, net462, net47, net471, net472, & net48. It is interesting that net462, net47, & net471 don't also need this reference 🤔 But it raises a question: Should we specify targets on these docs projects?

Copy link
Member

Choose a reason for hiding this comment

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

@CodeBlanch CodeBlanch merged commit 2604607 into open-telemetry:main Nov 30, 2022
@hgaol hgaol deleted the 3925 branch December 1, 2022 14:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants