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

Merge net7 branch to main: Updates DiagnosticSource to 7.0.0 + net7 instrumentation improvements #3539

Merged
merged 4 commits into from
Aug 4, 2022

Conversation

alanwest
Copy link
Member

@alanwest alanwest commented Aug 4, 2022

This reverts commit 25df7e8.
@alanwest alanwest requested a review from a team August 4, 2022 05:46
@codecov
Copy link

codecov bot commented Aug 4, 2022

Codecov Report

Merging #3539 (a243a4e) into main (2b6a3b0) will decrease coverage by 0.08%.
The diff coverage is 87.87%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #3539      +/-   ##
==========================================
- Coverage   86.80%   86.71%   -0.09%     
==========================================
  Files         275      275              
  Lines        9949     9959      +10     
==========================================
  Hits         8636     8636              
- Misses       1313     1323      +10     
Impacted Files Coverage Δ
...tp/Implementation/HttpHandlerDiagnosticListener.cs 72.63% <73.33%> (-0.87%) ⬇️
...tation.AspNetCore/Implementation/HttpInListener.cs 89.87% <100.00%> (+0.12%) ⬆️
...tion.AspNetCore/TracerProviderBuilderExtensions.cs 100.00% <100.00%> (ø)
....Instrumentation.Http/HttpClientInstrumentation.cs 100.00% <100.00%> (ø)
...umentation.Http/TracerProviderBuilderExtensions.cs 75.00% <100.00%> (+3.57%) ⬆️
...mentation/ExportClient/ExporterClientValidation.cs 30.00% <0.00%> (-70.00%) ⬇️
...c/OpenTelemetry/Internal/TagAndValueTransformer.cs 80.00% <0.00%> (-20.00%) ⬇️
...emetry.Api/Internal/OpenTelemetryApiEventSource.cs 76.47% <0.00%> (-5.89%) ⬇️
...rc/OpenTelemetry.Exporter.Zipkin/ZipkinExporter.cs 86.17% <0.00%> (-2.26%) ⬇️
...Exporter.Jaeger/ApacheThrift/Protocol/TProtocol.cs 91.30% <0.00%> (-0.86%) ⬇️
... and 8 more

@alanwest
Copy link
Member Author

alanwest commented Aug 4, 2022

Fixed a few small things. I think this is good to merge. We definitely need a changelog - maybe there's some dependency on #3448, but assuming we're dropping netcoreapp3 as a target, we should probably update the changelog for all relevant packages. I'll stage up a follow up PR so we can continue the discussion.

@reyang
Copy link
Member

reyang commented Aug 4, 2022

These CI can be removed from the branch policy:

image

@cijothomas cijothomas merged commit a789bc0 into main Aug 4, 2022
@cijothomas cijothomas deleted the net7.0 branch August 4, 2022 17:25
@xiang17
Copy link
Contributor

xiang17 commented Aug 4, 2022

Fixed a few small things. I think this is good to merge. We definitely need a changelog - maybe there's some dependency on #3448, but assuming we're dropping netcoreapp3 as a target, we should probably update the changelog for all relevant packages. I'll stage up a follow up PR so we can continue the discussion.

Will we have a version where we support both .NET Core 3.1 and .NET 7? There might be overlapping time where both are officially supported.

.NET Core 3.1 ends support in December 13, 2022.

Edit: .NET 7 is scheduled to release in November 2022. Only one month later. Maybe OTel will just release after .NET Core 3.1 ends support.

@alanwest
Copy link
Member Author

alanwest commented Aug 5, 2022

Will we have a version where we support both .NET Core 3.1 and .NET 7? There might be overlapping time where both are officially supported.

@xiang17 Technically OpenTelemetry .NET would likely continue to work with netcoreapp3.1 even though we've upgraded to DiagnosticSource v7. A netcoreapp3.1 application could still add us as a dependency because we currently still target netstandard2.0. Though, users will receive a build warning. However, one option on the table in #3448 is to drop our netstandard targets.

That said, we haven't settled on a final strategy or timeline yet for how we'll coordinate our next stable release with the release of .NET 7.

@xiang17
Copy link
Contributor

xiang17 commented Aug 5, 2022

Sounds cool!

I was trying to validate what you said but got a build error after adding netcoreapp3.1 in one of the test projects, only to find the PR was only merged 10 days ago. So, no action for now. :D

@alanwest alanwest changed the title Merge net7.0 to main Merge net7 branch to main: Updates DiagnosticSource to 7.0.0 + net7 instrumentation improvements Aug 17, 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.

4 participants