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

[Instrumentation.AspNetCore] .NET6 library works on .NET7 and .NET8 #5252

Merged

Conversation

Kielek
Copy link
Contributor

@Kielek Kielek commented Jan 24, 2024

Fixes #N/A
Design discussion issue #N/A

Changes

Fixes scenario when .NET6 targeted library is loaded into .NET7/.NET8 process (it is case which we have in AutoInstrumentation side) or even .NET7 to .NET8 for metrics.
If we speaking about additional conditional checks - only .NET6 and .NET7 suffers.

Minimal duplication of code is there. I think all of this stuff will be dropped in November this year.

Merge requirement checklist

  • CONTRIBUTING guidelines followed (license requirements, nullable enabled, static analysis, etc.)
  • Unit tests added/updated
  • Appropriate CHANGELOG.md files updated for non-trivial changes
  • Changes in public API reviewed (if applicable)

Copy link

codecov bot commented Jan 24, 2024

Codecov Report

Attention: 22 lines in your changes are missing coverage. Please review.

Comparison is base (6250307) 83.38% compared to head (3054ec1) 83.01%.
Report is 59 commits behind head on main.

❗ Current head 3054ec1 differs from pull request most recent head b7e2ad3. Consider uploading reports for the commit b7e2ad3 to get more accurate results

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #5252      +/-   ##
==========================================
- Coverage   83.38%   83.01%   -0.38%     
==========================================
  Files         297      272      -25     
  Lines       12531    11975     -556     
==========================================
- Hits        10449     9941     -508     
+ Misses       2082     2034      -48     
Flag Coverage Δ
unittests ?
unittests-Instrumentation-Stable 24.24% <50.00%> (?)
unittests-Solution-Experimental 82.69% <77.77%> (?)
unittests-Solution-Stable 82.96% <77.77%> (?)

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

Files Coverage Δ
....Api/Context/Propagation/TraceContextPropagator.cs 90.00% <100.00%> (+0.52%) ⬆️
...etryProtocol/Implementation/ExperimentalOptions.cs 100.00% <ø> (ø)
...rotocol/Implementation/OtlpLogRecordTransformer.cs 93.45% <100.00%> (ø)
...tation.AspNetCore/Implementation/HttpInListener.cs 89.93% <100.00%> (+0.34%) ⬆️
...AspNetCore/Implementation/HttpInMetricsListener.cs 89.74% <100.00%> (+0.26%) ⬆️
...ntation.GrpcNetClient/GrpcClientInstrumentation.cs 100.00% <100.00%> (ø)
...NetClient/GrpcClientTraceInstrumentationOptions.cs 100.00% <ø> (ø)
...ent/Implementation/GrpcClientDiagnosticListener.cs 75.80% <100.00%> (-2.77%) ⬇️
...n.GrpcNetClient/TracerProviderBuilderExtensions.cs 100.00% <100.00%> (ø)
...plementation/HttpWebRequestActivitySource.netfx.cs 80.77% <100.00%> (ø)
... and 13 more

... and 31 files with indirect coverage changes

@Kielek Kielek marked this pull request as ready for review January 24, 2024 16:16
@Kielek Kielek requested a review from a team as a code owner January 24, 2024 16:16
@Kielek
Copy link
Contributor Author

Kielek commented Jan 24, 2024

@vishweshbankwar, I have heard that you have discussed it with @rajkumar-rangaraj.

If you are fine with the changes. It will be great to release 1.7.1 version (also for HTTP package).

Co-authored-by: Reiley Yang <reyang@microsoft.com>
@vishweshbankwar
Copy link
Member

@vishweshbankwar, I have heard that you have discussed it with @rajkumar-rangaraj.

If you are fine with the changes. It will be great to release 1.7.1 version (also for HTTP package).

@Kielek I discussed this with @rajkumar-rangaraj again in detail. I think this can be handled on auto-instrumentation side without including the proposed changes in this PR. @rajkumar-rangaraj will follow up with you offline.

@Kielek Kielek force-pushed the aspnetcore-net6-lib-working-on-net7 branch from 94287c4 to cbba3d6 Compare January 31, 2024 18:30
@Kielek
Copy link
Contributor Author

Kielek commented Feb 6, 2024

@open-telemetry/dotnet-maintainers, could you please review and merge? We have had pretty long discussion with @rajkumar-rangaraj and @vishweshbankwar, and it is locally the best option to solve the issue on Auto Instrumentation side.

@alanwest alanwest merged commit c057e6a into open-telemetry:main Feb 6, 2024
44 checks passed
@Kielek Kielek deleted the aspnetcore-net6-lib-working-on-net7 branch February 6, 2024 19:26
alanwest added a commit to alanwest/opentelemetry-dotnet that referenced this pull request Feb 10, 2024
alanwest added a commit to alanwest/opentelemetry-dotnet that referenced this pull request Feb 10, 2024
alanwest added a commit that referenced this pull request Feb 10, 2024
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

5 participants