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

Fix build errors #1488

Merged
merged 3 commits into from
Dec 12, 2023
Merged

Conversation

utpilla
Copy link
Contributor

@utpilla utpilla commented Dec 11, 2023

Changes

@utpilla utpilla requested a review from a team December 11, 2023 23:43
<Reference Include="System.Net.Http" Condition="'$(TargetFramework)' == '$(NetFrameworkMinimumSupportedVersion)'" />
</ItemGroup>
-->
<PackageReference Include="System.Net.Http" Version="4.3.4" 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 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 System.Net.Http assembly 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 System.Net.Http assembly.

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

Copy link

codecov bot commented Dec 11, 2023

Codecov Report

Merging #1488 (f7d5098) into main (71655ce) will increase coverage by 7.91%.
Report is 92 commits behind head on main.
The diff coverage is 90.62%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1488      +/-   ##
==========================================
+ Coverage   73.91%   81.83%   +7.91%     
==========================================
  Files         267      124     -143     
  Lines        9615     3451    -6164     
==========================================
- Hits         7107     2824    -4283     
+ Misses       2508      627    -1881     
Flag Coverage Δ
unittests-Solution 81.83% <90.62%> (?)

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

Files Coverage Δ
...Telemetry.Exporter.InfluxDB/InfluxDBEventSource.cs 60.00% <ø> (ø)
...ry.Exporter.InfluxDB/InfluxDBExporterExtensions.cs 100.00% <100.00%> (ø)
...metry.Exporter.InfluxDB/InfluxDBMetricsExporter.cs 84.61% <ø> (-3.85%) ⬇️
...Telemetry.Exporter.InfluxDB/PointDataExtensions.cs 87.50% <100.00%> (ø)
...ry.Exporter.InfluxDB/TelegrafPrometheusWriterV1.cs 100.00% <ø> (ø)
...ry.Exporter.InfluxDB/TelegrafPrometheusWriterV2.cs 100.00% <ø> (ø)
...stana/Implementation/InstanaExporterEventSource.cs 0.00% <ø> (ø)
...try.Exporter.Instana/Implementation/InstanaSpan.cs 100.00% <ø> (ø)
...orter.Instana/Implementation/InstanaSpanFactory.cs 100.00% <ø> (ø)
...er.Instana/Implementation/InstanaSpanSerializer.cs 93.79% <ø> (ø)
... and 44 more

... and 212 files with indirect coverage changes

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

I have the same fixes on #1487 but I can merge these in.

@utpilla utpilla merged commit 4640cba into open-telemetry:main Dec 12, 2023
29 checks passed
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.

2 participants