-
Notifications
You must be signed in to change notification settings - Fork 5.1k
[AzureMonitorExporter] resolve AOT warnings #38459
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
Merged
Merged
Changes from all commits
Commits
Show all changes
26 commits
Select commit
Hold shift + click to select a range
38a5a2d
initial commit
f83ca49
build fix
99a0945
workaround for StackFrame.GetMethod()
546b8dc
cleanup
eb7a769
fix test
0e59bb4
remove TrimmingAttribute.cs
75c9764
temp disable ApiCompat
e9d72c3
cleanup
b072805
test fix for ApiCompat
cbc9757
update comment
1fa6880
add aotcompat test app
33f5126
add readme
32c0800
readme
6840dc2
recommended fix for ApiCompat
81ff67a
comment
e116952
test fix for validation errors re: aotcompat directory
f697d29
isolate StackFrame.GetMethod
fdb0b8d
cleanup
9ae8ff9
isolate StackFrame.GetMethod (2)
0116a5e
add comment.
8934fb7
fix
2532c7e
pr feedback
da7e594
update comment
9ea4200
fix script
2a3a0e4
refactor as extension method
1c660fa
cleanup
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
7 changes: 4 additions & 3 deletions
7
...itor/Azure.Monitor.OpenTelemetry.Exporter/src/Azure.Monitor.OpenTelemetry.Exporter.csproj
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
24 changes: 24 additions & 0 deletions
24
sdk/monitor/Azure.Monitor.OpenTelemetry.Exporter/src/Internals/SourceGenerationContext.cs
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,24 @@ | ||
| // Copyright (c) Microsoft Corporation. All rights reserved. | ||
| // Licensed under the MIT License. | ||
|
|
||
| using Azure.Monitor.OpenTelemetry.Exporter.Internals.Statsbeat; | ||
| using System.Text.Json.Serialization; | ||
|
|
||
| namespace Azure.Monitor.OpenTelemetry.Exporter.Internals; | ||
|
|
||
| #if NET6_0_OR_GREATER | ||
| // "Source Generation" is feature added to System.Text.Json in .NET 6.0. | ||
| // This is a performance optimization that avoids runtime reflection when performing serialization. | ||
| // Serialization metadata will be computed at compile-time and included in the assembly. | ||
| // https://learn.microsoft.com/dotnet/standard/serialization/system-text-json/source-generation-modes | ||
TimothyMothra marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| // https://learn.microsoft.com/dotnet/standard/serialization/system-text-json/source-generation | ||
| // https://devblogs.microsoft.com/dotnet/try-the-new-system-text-json-source-generator/ | ||
| [JsonSerializable(typeof(VmMetadataResponse))] | ||
| [JsonSerializable(typeof(string))] | ||
| [JsonSerializable(typeof(IngestionResponseHelper.ResponseObject))] | ||
| [JsonSerializable(typeof(IngestionResponseHelper.ErrorObject))] | ||
| [JsonSerializable(typeof(int))] | ||
| internal partial class SourceGenerationContext : JsonSerializerContext | ||
| { | ||
| } | ||
| #endif | ||
26 changes: 26 additions & 0 deletions
26
sdk/monitor/Azure.Monitor.OpenTelemetry.Exporter/src/Internals/StackFrameExtensions.cs
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,26 @@ | ||
| // Copyright (c) Microsoft Corporation. All rights reserved. | ||
| // Licensed under the MIT License. | ||
|
|
||
| using System.Diagnostics.CodeAnalysis; | ||
| using System.Reflection; | ||
| using System.Runtime.CompilerServices; | ||
|
|
||
| namespace Azure.Monitor.OpenTelemetry.Exporter.Internals | ||
| { | ||
| internal static class StackFrameExtensions | ||
| { | ||
| /// <summary> | ||
| /// Wrapper for <see cref="System.Diagnostics.StackFrame.GetMethod"/>. | ||
| /// This method disables the Trimmer warning "IL2026:RequiresUnreferencedCode". | ||
| /// Callers MUST handle the null condition. | ||
| /// </summary> | ||
| /// <remarks> | ||
| /// In an AOT scenario GetMethod() will return null. Note this can happen even in non AOT scenarios. | ||
| /// Instead, call ToString() which gives a string like this: | ||
| /// "MethodName + 0x00 at offset 000 in file:line:column filename unknown:0:0". | ||
| /// </remarks> | ||
| [MethodImpl(MethodImplOptions.AggressiveInlining)] | ||
| [UnconditionalSuppressMessage("ReflectionAnalysis", "IL2026:RequiresUnreferencedCode", Justification = "We will handle the null condition and call ToString() instead.")] | ||
| public static MethodBase? GetMethodWithoutWarning(this System.Diagnostics.StackFrame stackFrame) => stackFrame.GetMethod(); | ||
| } | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
34 changes: 34 additions & 0 deletions
34
...tCompatibilityTestApp/Azure.Monitor.OpenTelemetry.Exporter.AotCompatibilityTestApp.csproj
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,34 @@ | ||
| <Project Sdk="Microsoft.NET.Sdk"> | ||
| <PropertyGroup> | ||
| <OutputType>Exe</OutputType> | ||
| <TargetFrameworks>net7.0;net6.0;net462</TargetFrameworks> | ||
TimothyMothra marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| <IsTestSupportProject>true</IsTestSupportProject> | ||
| </PropertyGroup> | ||
|
|
||
| <PropertyGroup Condition="'$(TargetFramework)' == 'net7.0'"> | ||
| <PublishAot>true</PublishAot> | ||
| <TrimmerSingleWarn>false</TrimmerSingleWarn> | ||
| </PropertyGroup> | ||
|
|
||
| <ItemGroup> | ||
| <ProjectReference Include="..\..\..\..\core\Azure.Core\src\Azure.Core.csproj" /> | ||
| <ProjectReference Include="..\..\src\Azure.Monitor.OpenTelemetry.Exporter.csproj" /> | ||
|
|
||
| <!-- Update this dependency to its latest, which has all the annotations --> | ||
| <PackageReference Include="Microsoft.Extensions.Logging.Configuration" VersionOverride="8.0.0-rc.1.23419.4" /> | ||
| </ItemGroup> | ||
|
|
||
| <ItemGroup Condition="'$(TargetFramework)' == 'net7.0'"> | ||
TimothyMothra marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| <TrimmerRootAssembly Include="Azure.Monitor.OpenTelemetry.Exporter" /> | ||
| </ItemGroup> | ||
|
|
||
| <!--Temp fix for "error : No files matching ;NU5105;CA1812;"--> | ||
| <!--Real fix coming in .NET 8 RC2: https://github.com/dotnet/runtime/issues/91965--> | ||
| <ItemGroup> | ||
| <_NoWarn Include="$(NoWarn)" /> | ||
| </ItemGroup> | ||
| <PropertyGroup> | ||
| <NoWarn>@(_NoWarn)</NoWarn> | ||
| </PropertyGroup> | ||
|
|
||
| </Project> | ||
12 changes: 12 additions & 0 deletions
12
...ry.Exporter/tests/Azure.Monitor.OpenTelemetry.Exporter.AotCompatibilityTestApp/Program.cs
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,12 @@ | ||
| // Copyright (c) Microsoft Corporation. All rights reserved. | ||
| // Licensed under the MIT License. | ||
|
|
||
| namespace Azure.Monitor.OpenTelemetry.Exporter.AotCompatibilityTestApp; | ||
|
|
||
| internal class Program | ||
| { | ||
| public static void Main(string[] args) | ||
| { | ||
| System.Console.WriteLine("Hello, World!"); | ||
| } | ||
| } |
40 changes: 40 additions & 0 deletions
40
...er/tests/Azure.Monitor.OpenTelemetry.Exporter.AotCompatibilityTestApp/test-aot-compat.ps1
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,40 @@ | ||
| param([string]$targetNetFramework) | ||
|
|
||
| dotnet restore | ||
| $publishOutput = dotnet publish --framework net7.0 -nodeReuse:false /p:UseSharedCompilation=false /p:ExposeExperimentalFeatures=true | ||
|
|
||
| if ($LASTEXITCODE -ne 0) | ||
| { | ||
| Write-Host "Publish failed." | ||
| Write-Host $publishOutput | ||
| Exit 2 | ||
| } | ||
|
|
||
| $actualWarningCount = 0 | ||
|
|
||
| foreach ($line in $($publishOutput -split "`r`n")) | ||
| { | ||
| if ($line -like "*analysis warning IL*") | ||
| { | ||
| Write-Host $line | ||
|
|
||
| $actualWarningCount += 1 | ||
| } | ||
| } | ||
|
|
||
| Write-Host "Actual warning count is:", $actualWarningCount | ||
| $expectedWarningCount = 7 | ||
TimothyMothra marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| # Known warnings: | ||
| # - Azure.Core.Serialization.DynamicData: Using member 'Azure.Core.Serialization.DynamicData.DynamicDataJsonConverter.DynamicDataJsonConverter()' | ||
| # - 4x Azure.RequestFailedException.TryExtractErrorContent(): Using member 'System.Text.Json.JsonSerializer.Deserialize<>()' (https://github.com/Azure/azure-sdk-for-net/pull/38996) | ||
| # - Azure.Core.Json.MutableJsonDocument: Using member 'Azure.Core.Json.MutableJsonDocument.MutableJsonDocumentConverter.MutableJsonDocumentConverter()' | ||
| # - Microsoft.Extensions.DependencyInjection.ServiceLookup.IEnumerableCallSite.ServiceType.get: Using member 'System.Type.MakeGenericType(Type[]) | ||
|
|
||
| $testPassed = 0 | ||
| if ($actualWarningCount -ne $expectedWarningCount) | ||
| { | ||
| $testPassed = 1 | ||
| Write-Host "Actual warning count:", $actualWarningCount, "is not as expected. Expected warning count is:", $expectedWarningCount | ||
| } | ||
|
|
||
| Exit $testPassed | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Out of curiosity, why do we need this change?
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.
When we shipped last week, the CI added
ApiCompatVersion 1.0.0to ensure that our Public API doesn't change. (Similar to the PublicApi analyzer we have in other repos).When I added
net6.0, the ApiCompat fails because there was no net6.0 in the 1.0.0 release.I checked with the Azure Experts and they advised to add this condition; we only need to check the api compat for the framework that we shipped already (netstandard2.0).
When we ship our next version, the CI should regenerate this line without the condition. :)