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

Non-core packages: Remove netstandard targets. Replace with net6.0/net462 as applicable. #3697

Closed

Conversation

alanwest
Copy link
Member

In the previous SIG meeting I had discussed drafting an announcement regarding our stance for supporting .NET Standard.

In drafting that announcement, I believe this PR may help us clarify our current stance.

.NET Standard targets have already been removed from ASP.NET Core and HttpClient instrumentation in #3567 and
#3664, respectively.

This PR poses the question whether we should apply this stance to all our our non-core "unstable" packages. My opinion is that it would be odd for us to claim support for .NET Standard for some but not all of our instrumentation and extensions.

The question of whether we should do anything with our core packages remains a separate conversation. That said, I think it's possible that users will find it odd that the API/SDK continue support .NET Standard - thereby allowing them to use them in .NET 5/.NET Core 3.1 apps - but they are prevented from using the latest instrumentation.

Thoughts?

@alanwest alanwest requested a review from a team September 26, 2022 23:03
@codecov
Copy link

codecov bot commented Sep 26, 2022

Codecov Report

Merging #3697 (64527a1) into main (8800b2f) will increase coverage by 0.21%.
The diff coverage is 100.00%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #3697      +/-   ##
==========================================
+ Coverage   87.54%   87.75%   +0.21%     
==========================================
  Files         283      283              
  Lines       10308    10308              
==========================================
+ Hits         9024     9046      +22     
+ Misses       1284     1262      -22     
Impacted Files Coverage Δ
...ion.AspNetCore/AspNetCoreInstrumentationOptions.cs 100.00% <ø> (ø)
....EventSource/OpenTelemetryEventSourceLogEmitter.cs 96.20% <100.00%> (ø)
src/OpenTelemetry/ProviderExtensions.cs 81.81% <0.00%> (-9.10%) ⬇️
...emetry.Api/Internal/OpenTelemetryApiEventSource.cs 76.47% <0.00%> (-5.89%) ⬇️
...Telemetry/Metrics/PeriodicExportingMetricReader.cs 72.72% <0.00%> (-5.46%) ⬇️
...tpListener/Internal/PrometheusCollectionManager.cs 72.52% <0.00%> (-2.20%) ⬇️
...metryProtocol/Implementation/ActivityExtensions.cs 95.05% <0.00%> (+3.29%) ⬆️
...ZPages/Implementation/ZPagesExporterEventSource.cs 62.50% <0.00%> (+6.25%) ⬆️
src/OpenTelemetry/Logs/Pool/LogRecordSharedPool.cs 100.00% <0.00%> (+21.05%) ⬆️
...xporter.OpenTelemetryProtocol/OtlpTraceExporter.cs 77.27% <0.00%> (+40.90%) ⬆️
... and 1 more

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.

LGTM, this seems like a significant change that needs to be called out in the changelog? (e.g. someone previously targeting netstandard2.0 will now have to introduce multiple targets?)

@alanwest
Copy link
Member Author

LGTM, this seems like a significant change that needs to be called out in the changelog? (e.g. someone previously targeting netstandard2.0 will now have to introduce multiple targets?)

Yes, definitely. I'm drafting a proper announcement that will show up here https://github.com/open-telemetry/opentelemetry-dotnet/discussions. I plan to discuss this PR in the SIG tomorrow to ensure this is really the direction we want to go. If folks feel good about this then I will update the changelogs with a link to the announcement I've drafted. My goal is to get people's attention one way or another.

@cijothomas
Copy link
Member

In the announcements, we need to mention why 3.1 is being dropped. (its EOL in Dec 2022....)

@cijothomas
Copy link
Member

In the announcements, we need to mention why 3.1 is being dropped. (its EOL in Dec 2022....)

Or more like "why not add netcoreapp3.1" target..

@alanwest
Copy link
Member Author

Draft announcement #3701

In the announcements, we need to mention why 3.1 is being dropped. (its EOL in Dec 2022....)

This is described in the "background" section of the announcement.

I propose we settle on the announcement, get it published, then merge this PR with updated changelogs linking to the announcement.

@alanwest
Copy link
Member Author

alanwest commented Oct 4, 2022

Discussed in SIG meeting today to keep .NET Standard targets for now. We can always come back to the decision to remove .NET Standard targets at a later date. #3703 is a draft announcement that addresses the warnings end users may receive when upgrading OpenTelemetry dependencies.

@alanwest alanwest closed this Oct 4, 2022
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.

5 participants