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

[Exporter.Geneva] Add net8.0 target for GenevaExporter #1463

Conversation

utpilla
Copy link
Contributor

@utpilla utpilla commented Dec 5, 2023

Changes

  • Add net8.0 target for GenevaExporter
  • Update OTel SDK version to 1.7.0-rc.1
  • Update public API files (this was missed in previous stable releases)
  • Update package version of Benchmark.NET
  • Appropriate CHANGELOG.md updated for non-trivial changes

@utpilla utpilla added the comp:exporter.geneva Things related to OpenTelemetry.Exporter.Geneva label Dec 5, 2023
@utpilla utpilla requested a review from a team December 5, 2023 21:43
Copy link

codecov bot commented Dec 5, 2023

Codecov Report

Merging #1463 (5dd931d) into main (71655ce) will increase coverage by 0.43%.
Report is 69 commits behind head on main.
The diff coverage is 70.26%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1463      +/-   ##
==========================================
+ Coverage   73.91%   74.35%   +0.43%     
==========================================
  Files         267      262       -5     
  Lines        9615     9806     +191     
==========================================
+ Hits         7107     7291     +184     
- Misses       2508     2515       +7     
Flag Coverage Δ
unittests-Exporter.Geneva 60.25% <32.14%> (?)
unittests-Exporter.OneCollector 89.72% <100.00%> (?)
unittests-Extensions 82.75% <100.00%> (?)
unittests-Instrumentation.AspNet 72.56% <94.44%> (?)
unittests-Instrumentation.EventCounters 75.92% <ø> (?)
unittests-Instrumentation.Owin 85.71% <100.00%> (?)
unittests-Instrumentation.Process 100.00% <ø> (?)
unittests-Instrumentation.Runtime 100.00% <ø> (?)
unittests-Instrumentation.StackExchangeRedis 71.37% <6.45%> (?)
unittests-Instrumentation.Wcf 78.33% <81.59%> (?)
unittests-PersistentStorage 58.80% <ø> (?)
unittests-ResourceDetectors.Azure 80.90% <60.00%> (?)
unittests-ResourceDetectors.ProcessRuntime 76.08% <76.08%> (?)
unittests-Solution 81.10% <81.15%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
...xporter.Geneva/Internal/ConnectionStringBuilder.cs 92.94% <100.00%> (ø)
...r.Geneva/Metrics/GenevaMetricExporterExtensions.cs 93.33% <ø> (ø)
...orter.Geneva/MsgPackExporter/MsgPackLogExporter.cs 95.10% <100.00%> (+1.76%) ⬆️
...ter.Geneva/MsgPackExporter/MsgPackTraceExporter.cs 94.52% <100.00%> (+2.10%) ⬆️
...ry.Exporter.InfluxDB/InfluxDBExporterExtensions.cs 100.00% <100.00%> (ø)
....Exporter.OneCollector/Internal/CallbackManager.cs 96.00% <100.00%> (+0.16%) ⬆️
...ector/Internal/Transports/HttpJsonPostTransport.cs 95.41% <ø> (ø)
...er.OneCollector/Internal/Transports/IHttpClient.cs 100.00% <ø> (ø)
...ctor/Logs/OneCollectorLogExportProcessorBuilder.cs 67.74% <ø> (ø)
...ions.Enrichment/Internal/TraceEnrichmentActions.cs 100.00% <100.00%> (ø)
... and 68 more

... and 35 files with indirect coverage changes

@@ -16,6 +16,7 @@

<ItemGroup>
<PackageReference Include="OpenTelemetry" Version="$(OTelSdkVersion)" />
<PackageReference Include="System.Runtime.InteropServices.RuntimeInformation" Version="4.0.0" Condition="'$(TargetFramework)' == 'net462'" />
Copy link
Contributor Author

Choose a reason for hiding this comment

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

With 1.7.0-rc.1 version of the SDK, this package now pulls in 8.0.0 version of Microsoft.Extensions.Logging.Configuration instead of 3.1.0 version that used to come with the previous dependency.

Microsoft.Extensions.Logging.Configuration has added a dedicated net462 target in 8.0.0 which does not have the RuntimeInformation library included. That's why we need an explicit package dependency for net462 targets now. With 3.1.0 version, there was no dedicated net462 target, and the package ended up using netstandard2.0 bits which includes the RuntimeInformation package.

Follow this discussion for more details: open-telemetry/opentelemetry-dotnet#5020 (comment)

Copy link
Member

@CodeBlanch CodeBlanch left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -117,7 +125,7 @@ public override unsafe int GetChars(byte* bytePtr, int byteCount, char* charPtr,
return byteCount;
}

Copy link
Member

Choose a reason for hiding this comment

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

nit: this line is not needed I guess?

@utpilla utpilla merged commit ec811d8 into open-telemetry:main Dec 6, 2023
96 of 97 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
comp:exporter.geneva Things related to OpenTelemetry.Exporter.Geneva
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants