-
Notifications
You must be signed in to change notification settings - Fork 773
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
Address AOT warnings in Exporter.OpenTelemetryProtocol #4859
Conversation
This addresses the AOT warnings in the last library in this opentelemetry-dotnet. 1. Google.Protobuf v3.19.4 isn't trimming and AOT compatible. We need to upgrade to a newer version. I chose the latest patch in v3.22.x. v3.22.0 was the first version that is AOT compatible. 2. There were 2 places in Exporter.OpenTelemetryProtocol that was using System.Reflection.Emit to set a private field on Protobuf's RepeatedField class in order to return it to a pool. Setting this private field is no longer necessary since v3.22.0 because the Clear method was updated to support this scenario. See protocolbuffers/protobuf@a4fd216 Fix open-telemetry#3429
@CodeBlanch - I see you commented on the Google.Protobuf issue and PR that fixed the |
Codecov Report
@@ Coverage Diff @@
## main #4859 +/- ##
==========================================
- Coverage 83.25% 83.16% -0.10%
==========================================
Files 293 293
Lines 12012 11984 -28
==========================================
- Hits 10001 9966 -35
- Misses 2011 2018 +7
Flags with carried forward coverage won't be shown. Click here to find out more.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM with a changelog entry.
This commit: CodeBlanch@18c2d40 I'll bring this up for discussion in tomorrow's SIG meeting. |
@eerhardt @Yun-Ting We had updated this package version earlier but had to revert the changes in #4407. The reason behind it was mainly to not affect any auto-instrumentation users who could be using a lower version of the package. Auto-instrumentation dynamically loads this assembly and its dependencies. Check this comment. Back then we didn't have any immediate need to be AOT-safe. However, AOT-safety is the major feature to be offered in |
@eerhardt, please check this PR #4201 You should be able to drop reference to You should be able to remove also https://github.com/open-telemetry/opentelemetry-dotnet/blob/7cb92d3b6dbad169048e5aa07939ef613f2587e2/Directory.Packages.props#L44C1-L45 to fully cleanup this reference. The reason to keep older version is AutoInstrumentation (it will narrow the set of application which can be handled without code changes). |
- Add Changelog - Remove unneeded reference to System.Reflection.Emit.Lightweight
Thank you @utpilla for sharing this! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM assuming this comment got addressed: #4859 (comment). Thanks!
This addresses the AOT warnings in the last library in opentelemetry-dotnet.
Google.Protobuf v3.19.4 isn't trimming and AOT compatible. We need to upgrade to a newer version. I chose the latest patch in v3.22.x. v3.22.0 was the first version that is AOT compatible.
There were 2 places in Exporter.OpenTelemetryProtocol that was using System.Reflection.Emit to set a private field on Protobuf's RepeatedField class in order to return it to a pool. Setting this private field is no longer necessary since v3.22.0 because the Clear method was updated to support this scenario. See protocolbuffers/protobuf@a4fd216
Fix #3429
@vitek-karas @Yun-Ting @reyang @utpilla