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

[AOT] suppressed trimming warnings in HTTP Instrumentation package #4770

Merged
merged 11 commits into from Aug 16, 2023

Conversation

Yun-Ting
Copy link
Contributor

@Yun-Ting Yun-Ting commented Aug 15, 2023

Towards #3429

cc: @eerhardt, @vitek-karas

Changes

Suppressed trimming warnings IL2026 in HTTP Instrumentation package because the AOT-annotation DynamicallyAccessedMembers
https://learn.microsoft.com/dotnet/api/system.diagnostics.codeanalysis.dynamicallyaccessedmembersattribute?view=net-7.0
in Systm.Net.Http library ensures that top-level properties on the payload object are always preserved.
See https://github.com/dotnet/runtime/blob/f9246538e3d49b90b0e9128d7b1defef57cd6911/src/libraries/System.Net.Http/src/System/Net/Http/DiagnosticsHandler.cs#L325

In .NET runtime repo, there is an API proposal to make payload types in System.Net.Http.DiagnosticsHandler public: dotnet/runtime#82910.
Once .NET runtime makes the above change, OTel should update the HTTP instrumentation implementation to access the public payload types directly.

Merge requirement checklist

  • CONTRIBUTING guidelines followed (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)

@Yun-Ting Yun-Ting changed the title [AOT] suppressed trim warnings in HTTP Instrumentation package [AOT] suppressed trimming warnings in HTTP Instrumentation package Aug 15, 2023
@codecov
Copy link

codecov bot commented Aug 15, 2023

Codecov Report

Merging #4770 (8afd865) into main (031ed48) will decrease coverage by 0.03%.
Report is 1 commits behind head on main.
The diff coverage is 88.46%.

❗ Current head 8afd865 differs from pull request most recent head 451554f. Consider uploading reports for the commit 451554f to get more accurate results

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #4770      +/-   ##
==========================================
- Coverage   82.12%   82.10%   -0.03%     
==========================================
  Files         313      313              
  Lines       12783    12796      +13     
==========================================
+ Hits        10498    10506       +8     
- Misses       2285     2290       +5     
Files Changed Coverage Δ
...ementation/HttpHandlerMetricsDiagnosticListener.cs 77.14% <85.71%> (+1.38%) ⬆️
...tp/Implementation/HttpHandlerDiagnosticListener.cs 69.60% <89.47%> (+1.17%) ⬆️

... and 3 files with indirect coverage changes

@Yun-Ting Yun-Ting marked this pull request as ready for review August 15, 2023 02:14
@Yun-Ting Yun-Ting requested a review from a team as a code owner August 15, 2023 02:14
Co-authored-by: Eric Erhardt <eric.erhardt@microsoft.com>
…lerMetricsDiagnosticListener.cs

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

@eerhardt eerhardt left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks!

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

@CodeBlanch CodeBlanch merged commit 5784b45 into open-telemetry:main Aug 16, 2023
28 checks passed
@Yun-Ting Yun-Ting deleted the yunl/aotHttpListener branch August 16, 2023 20:08
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