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

Bump Microsoft.Extensions.Logging to net8 #4920

Merged
merged 13 commits into from
Oct 6, 2023

Conversation

reyang
Copy link
Member

@reyang reyang commented Oct 5, 2023

Related to #3205.

This is essentially the same thing as #4550.

@codecov
Copy link

codecov bot commented Oct 5, 2023

Codecov Report

Merging #4920 (842ca30) into main (d76542e) will decrease coverage by 0.16%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #4920      +/-   ##
==========================================
- Coverage   83.44%   83.29%   -0.16%     
==========================================
  Files         295      295              
  Lines       12324    12324              
==========================================
- Hits        10284    10265      -19     
- Misses       2040     2059      +19     
Flag Coverage Δ
unittests 83.29% <ø> (-0.16%) ⬇️

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

see 7 files with indirect coverage changes

@reyang reyang marked this pull request as ready for review October 5, 2023 03:56
@reyang reyang requested a review from a team as a code owner October 5, 2023 03:56
Copy link
Contributor

@Kielek Kielek left a comment

Choose a reason for hiding this comment

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

@reyang, this kind of PRs are killing possibility of using Automatic Instrumentation without including it in build pipeline on .NET. .NET Framework is still pretty safe (after we make a release with stable reference to the newly released library version).

2) Each minor version bump is normally security hotfixes or critical bug fixes.

Based on https://www.nuget.org/packages/Microsoft.Extensions.Logging/#versions-body-tab there is no security issue in any 3.1.x release. The same is for 6.0.0 and 7.0.0.
So we have only critical bug fixes from 3.1.x.

As I remember the package is part of the .NET 6+ and even if your application is referencing 3.1.0 it will be implicitly bumped to the version included in .NET.

Based on this, it should be safe to update dependency to 6.0.0 (I assume that it includes all critical fixes).

IMO EventId auto-generation is not good enough reason to update now, We can consider doing it when .NET6 reach EOL: November 12, 2024. It can be potentially done for release synced with .NET9.

FYI: @open-telemetry/dotnet-instrumentation-maintainers

docs/Directory.Packages.props Show resolved Hide resolved
Directory.Packages.props Outdated Show resolved Hide resolved
Copy link

@noahfalk noahfalk left a comment

Choose a reason for hiding this comment

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

I wasn't sure how some of the nullability and Http reference changes related to M.E.Logging, but no qualms with them.

@reyang
Copy link
Member Author

reyang commented Oct 5, 2023

@reyang, this kind of PRs are killing possibility of using Automatic Instrumentation without including it in build pipeline on .NET. .NET Framework is still pretty safe (after we make a release with stable reference to the newly released library version).

@Kielek would you provide some context here? For example, we're already doing the same thing for System.Diagnostics.DiagnosticSource

<!--
Typically, the latest stable version of System.Diagnostics.DiagnosticSource should be used here because:
1) Each major version bump will have some new OpenTelemetry API capabilities (e.g. .NET 6 introduced Meter
API, .NET 7 added UpDownCounter, .NET 8 is adding Meter/Instrument level attributes support, .NET 9 might
add Advice/Hint API) that the OpenTelemetry components rely on.
2) Each minor version bump is normally security hotfixes or critical bug fixes.
3) The .NET runtime team provides extra backward compatibility guarantee to System.Diagnostics.DiagnosticSource
even during major version bumps, so compatibility is not a concern here.
-->
<PackageVersion Include="System.Diagnostics.DiagnosticSource" Version="7.0.2" />

How does auto-instrumention solve the System.Diagnostics.DiagnosticSource version challenge today, can the same approach be applied to Microsoft.Extensions.Logging? Is there something that folks from .NET runtime can help?

Co-authored-by: Noah Falk <noahfalk@users.noreply.github.com>
@cijothomas
Copy link
Member

Fixes #3205.

This is essentially the same thing as #4550.

The PR description can be updated to exclude Fixes #3205, as even with this PR, SDK won't require Ms.Ext.Logging 8, so #3205 is still open.

@reyang reyang changed the title Bump Microsoft.Extensions.Logging and dependencies to net8 Bump Microsoft.Extensions.Logging to net8 Oct 6, 2023
@utpilla utpilla merged commit f01db2b into open-telemetry:main Oct 6, 2023
35 checks passed
@reyang reyang deleted the reyang/ilogger-version branch January 12, 2024 20:53
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.

None yet

9 participants