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.

Sign up for GitHub

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

Improve AoT validation and fix OpenTelemetryProtocol AoT warnings #5520

Merged
merged 5 commits into from
Apr 8, 2024
+45 −12
Merged

Improve AoT validation and fix OpenTelemetryProtocol AoT warnings #5520

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
5b707f6
Improve AoT validation
martincostello Apr 8, 2024
9a9919d
Fix AoT warnings
martincostello Apr 8, 2024
2c5dc9a
Update CHANGELOG
martincostello Apr 8, 2024
087cacb
Use ConfigurationBinder source generator to solve AOT incompatibility
eerhardt Apr 8, 2024
be60873
Merge branch 'main' into improve-aot-validation
CodeBlanch Apr 8, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 1 addition & 2 deletions .github/workflows/verifyaotcompat.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ jobs:
strategy:
fail-fast: false # ensures the entire test matrix is run, even if one permutation fails
matrix:
os: [ ubuntu-latest ]
os: [ ubuntu-latest, windows-latest ]
version: [ net8.0 ]

runs-on: ${{ matrix.os }}
Expand All @@ -24,4 +24,3 @@ jobs:
- name: publish AOT testApp, assert static analysis warning count, and run the app
shell: pwsh
run: .\build\test-aot-compatibility.ps1 ${{ matrix.version }}

1 change: 1 addition & 0 deletions Directory.Packages.props
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@
-->

<PackageVersion Include="Microsoft.Extensions.Configuration" Version="8.0.0" />
<PackageVersion Include="Microsoft.Extensions.Configuration.Binder" Version="8.0.1" />
<PackageVersion Include="Microsoft.Extensions.DependencyInjection.Abstractions" Version="8.0.0" />
<PackageVersion Include="Microsoft.Extensions.Diagnostics.Abstractions" Version="8.0.0" />
<PackageVersion Include="Microsoft.Extensions.Hosting.Abstractions" Version="8.0.0" />
Expand Down
26 changes: 17 additions & 9 deletions build/test-aot-compatibility.ps1
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,35 +1,43 @@
param([string]$targetNetFramework)

$rootDirectory = Split-Path $PSScriptRoot -Parent
$publishOutput = dotnet publish $rootDirectory/test/OpenTelemetry.AotCompatibility.TestApp/OpenTelemetry.AotCompatibility.TestApp.csproj -nodeReuse:false /p:UseSharedCompilation=false /p:ExposeExperimentalFeatures=true
$publishOutput = dotnet publish $rootDirectory/test/OpenTelemetry.AotCompatibility.TestApp/OpenTelemetry.AotCompatibility.TestApp.csproj --framework $targetNetFramework -nodeReuse:false /p:UseSharedCompilation=false /p:ExposeExperimentalFeatures=true

$actualWarningCount = 0

foreach ($line in $($publishOutput -split "`r`n"))
{
if ($line -like "*analysis warning IL*")
if (($line -like "*analysis warning IL*") -or ($line -like "*analysis error IL*"))
{
Write-Host $line

$actualWarningCount += 1
}
}

pushd $rootDirectory/test/OpenTelemetry.AotCompatibility.TestApp/bin/Release/$targetNetFramework/linux-x64
Write-Host "Actual warning count is:", $actualWarningCount
$expectedWarningCount = 0

if ($LastExitCode -ne 0)
{
Write-Host "There was an error while publishing AotCompatibility Test App. LastExitCode is:", $LastExitCode
Write-Host $publishOutput
}

$runtime = $IsWindows ? "win-x64" : ($IsMacOS ? "macos-x64" : "linux-x64")
$app = $IsWindows ? "./OpenTelemetry.AotCompatibility.TestApp.exe" : "./OpenTelemetry.AotCompatibility.TestApp"

Push-Location $rootDirectory/test/OpenTelemetry.AotCompatibility.TestApp/bin/Release/$targetNetFramework/$runtime

Write-Host "Executing test App..."
./OpenTelemetry.AotCompatibility.TestApp
$app
Write-Host "Finished executing test App"

if ($LastExitCode -ne 0)
{
Write-Host "There was an error while executing AotCompatibility Test App. LastExitCode is:", $LastExitCode
}

popd

Write-Host "Actual warning count is:", $actualWarningCount
$expectedWarningCount = 0
Pop-Location

$testPassed = 0
if ($actualWarningCount -ne $expectedWarningCount)
Expand Down
3 changes: 3 additions & 0 deletions src/OpenTelemetry.Exporter.OpenTelemetryProtocol/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,9 @@

## Unreleased

* Fix native AoT warnings in `OpenTelemetry.Exporter.OpenTelemetryProtocol`.
([#5520](https://github.com/open-telemetry/opentelemetry-dotnet/pull/5520))

## 1.8.0

Released 2024-Apr-02
Expand Down
4 changes: 4 additions & 0 deletions ...emetry.Exporter.OpenTelemetryProtocol/OpenTelemetry.Exporter.OpenTelemetryProtocol.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,9 @@
<!-- this is temporary. will remove in future PR. -->
<Nullable>disable</Nullable>
<DefineConstants>BUILDING_INTERNAL_PERSISTENT_STORAGE;$(DefineConstants)</DefineConstants>
<EnableConfigurationBindingGenerator>true</EnableConfigurationBindingGenerator>
<!-- SYSLIB1100;SYSLIB1101 - Configuration.Binder: can't create instance and unsupported type -->
<NoWarn>$(NoWarn);SYSLIB1100;SYSLIB1101</NoWarn>
Copy link
Member

Choose a reason for hiding this comment

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

@martincostello What code is generating these warnings is it DelegatingOptionsFactory?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

From #5518:

  LondonTravel.Skill failed with errors (45.7s) → artifacts\bin\LondonTravel.Skill\release\LondonTravel.Skill.dll
    ILC : Trim analysis error IL2091: Microsoft.Extensions.DependencyInjection.DelegatingOptionsFactoryServiceCollectionExtensions.RegisterOptionsFactory<T>(IServiceCollection,Func`2<IConfiguration,!!0>): 'TOptions' generic argument does not satisfy 'DynamicallyAccessedMemberTypes.PublicParameterlessConstructor' in 'Microsoft.Extensions.Options.IOptionsFactory`1'. The generic parameter 'T' of 'Microsoft.Extensions.DependencyInjection.DelegatingOptionsFactoryServiceCollectionExtensions.RegisterOptionsFactory<T>(IServiceCollection,Func`2<IConfiguration,!!0>)' does not have matching annotations. The source value must declare at least the same requirements as those declared on the target location it is assigned to. [C:\Coding\martincostello\alexa-london-travel\src\LondonTravel.Skill\LondonTravel.Skill.csproj]
    ILC : Trim analysis error IL2091: Microsoft.Extensions.DependencyInjection.DelegatingOptionsFactoryServiceCollectionExtensions.RegisterOptionsFactory<T>(IServiceCollection,Func`2<IConfiguration,!!0>): 'TOptions' generic argument does not satisfy 'DynamicallyAccessedMemberTypes.PublicParameterlessConstructor' in 'Microsoft.Extensions.Options.IOptionsFactory`1'. The generic parameter 'T' of 'Microsoft.Extensions.DependencyInjection.DelegatingOptionsFactoryServiceCollectionExtensions.RegisterOptionsFactory<T>(IServiceCollection,Func`2<IConfiguration,!!0>)' does not have matching annotations. The source value must declare at least the same requirements as those declared on the target location it is assigned to. [C:\Coding\martincostello\alexa-london-travel\src\LondonTravel.Skill\LondonTravel.Skill.csproj]
    ILC : Trim analysis error IL2091: Microsoft.Extensions.DependencyInjection.DelegatingOptionsFactoryServiceCollectionExtensions.<>c__DisplayClass0_0`1.<RegisterOptionsFactory>b__0(IServiceProvider): 'TOptions' generic argument does not satisfy 'DynamicallyAccessedMemberTypes.PublicParameterlessConstructor' in 'Microsoft.Extensions.Options.IOptionsFactory`1'. The generic parameter 'T' of 'Microsoft.Extensions.DependencyInjection.DelegatingOptionsFactoryServiceCollectionExtensions.<>c__DisplayClass0_0`1' does not have matching annotations. The source value must declare at least the same requirements as those declared on the target location it is assigned to. [C:\Coding\martincostello\alexa-london-travel\src\LondonTravel.Skill\LondonTravel.Skill.csproj]
    ILC : Trim analysis error IL2091: Microsoft.Extensions.DependencyInjection.DelegatingOptionsFactoryServiceCollectionExtensions.RegisterOptionsFactory<T>(IServiceCollection,Func`4<IServiceProvider,IConfiguration,String,!!0>): 'TOptions' generic argument does not satisfy 'DynamicallyAccessedMemberTypes.PublicParameterlessConstructor' in 'Microsoft.Extensions.Options.IOptionsFactory`1'. The generic parameter 'T' of 'Microsoft.Extensions.DependencyInjection.DelegatingOptionsFactoryServiceCollectionExtensions.RegisterOptionsFactory<T>(IServiceCollection,Func`4<IServiceProvider,IConfiguration,String,!!0>)' does not have matching annotations. The source value must declare at least the same requirements as those declared on the target location it is assigned to. [C:\Coding\martincostello\alexa-london-travel\src\LondonTravel.Skill\LondonTravel.Skill.csproj]
    ILC : Trim analysis error IL2091: Microsoft.Extensions.DependencyInjection.DelegatingOptionsFactoryServiceCollectionExtensions.RegisterOptionsFactory<T>(IServiceCollection,Func`4<IServiceProvider,IConfiguration,String,!!0>): 'TOptions' generic argument does not satisfy 'DynamicallyAccessedMemberTypes.PublicParameterlessConstructor' in 'Microsoft.Extensions.Options.IOptionsFactory`1'. The generic parameter 'T' of 'Microsoft.Extensions.DependencyInjection.DelegatingOptionsFactoryServiceCollectionExtensions.RegisterOptionsFactory<T>(IServiceCollection,Func`4<IServiceProvider,IConfiguration,String,!!0>)' does not have matching annotations. The source value must declare at least the same requirements as those declared on the target location it is assigned to. [C:\Coding\martincostello\alexa-london-travel\src\LondonTravel.Skill\LondonTravel.Skill.csproj]
    ILC : Trim analysis error IL2091: Microsoft.Extensions.DependencyInjection.DelegatingOptionsFactoryServiceCollectionExtensions.<>c__DisplayClass1_0`1.<RegisterOptionsFactory>b__0(IServiceProvider): 'TOptions' generic argument does not satisfy 'DynamicallyAccessedMemberTypes.PublicParameterlessConstructor' in 'Microsoft.Extensions.Options.IOptionsFactory`1'. The generic parameter 'T' of 'Microsoft.Extensions.DependencyInjection.DelegatingOptionsFactoryServiceCollectionExtensions.<>c__DisplayClass1_0`1' does not have matching annotations. The source value must declare at least the same requirements as those declared on the target location it is assigned to. [C:\Coding\martincostello\alexa-london-travel\src\LondonTravel.Skill\LondonTravel.Skill.csproj]
    ILC : Trim analysis error IL2091: Microsoft.Extensions.Options.DelegatingOptionsFactory`1: 'TOptions' generic argument does not satisfy 'DynamicallyAccessedMemberTypes.PublicParameterlessConstructor' in 'Microsoft.Extensions.Options.IOptionsFactory`1'. The generic parameter 'TOptions' of 'Microsoft.Extensions.Options.DelegatingOptionsFactory`1' does not have matching annotations. The source value must declare at least the same requirements as those declared on the target location it is assigned to. [C:\Coding\martincostello\alexa-london-travel\src\LondonTravel.Skill\LondonTravel.Skill.csproj]

You can also see the errors here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh sorry, you meant the SYSLIB* warnings - I don't know - @eerhardt presumably found them locally when working on his commit.

Copy link
Contributor

@eerhardt eerhardt Apr 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

These warnings are documented in https://learn.microsoft.com/en-us/dotnet/fundamentals/syslib-diagnostics/syslib1100-1118. The code that is emitting those warnings is from the Configuration Binder Source Generator. It is happening because the IOtlpExporterOptions public property is an interface, which can't be instantiated. So the config binder emits a warning saying "FYI - I can't create one of these objects".

opentelemetry-dotnet/src/OpenTelemetry.Exporter.OpenTelemetryProtocol/Builder/OtlpExporterBuilderOptions.cs

Lines 57 to 63 in c7c7a69

public IOtlpExporterOptions DefaultOptions => this.DefaultOptionsInstance;
public IOtlpExporterOptions LoggingOptions => this.LoggingOptionsInstance;
public IOtlpExporterOptions MetricsOptions => this.MetricsOptionsInstance;
public IOtlpExporterOptions TracingOptions => this.TracingOptionsInstance;

Here this is safe to suppress because we are creating instances of those objects in the ctor (which is then created through the DelegatingOptionsFactory:

opentelemetry-dotnet/src/OpenTelemetry.Exporter.OpenTelemetryProtocol/Builder/OtlpExporterBuilder.cs

Lines 183 to 198 in c7c7a69

services!.RegisterOptionsFactory((sp, configuration, name) => new OtlpExporterBuilderOptions(
configuration,
/* Note: We don't use name for SdkLimitOptions. There should only be
one provider for a given service collection so SdkLimitOptions is
treated as a single default instance. */
sp.GetRequiredService<IOptionsMonitor<SdkLimitOptions>>().CurrentValue,
sp.GetRequiredService<IOptionsMonitor<ExperimentalOptions>>().Get(name),
/* Note: We allow LogRecordExportProcessorOptions,
MetricReaderOptions, & ActivityExportProcessorOptions to be null
because those only exist if the corresponding signal is turned on.
Currently this extension turns on all signals so they will always be
there but that may change in the future so it is handled
defensively. */
sp.GetService<IOptionsMonitor<LogRecordExportProcessorOptions>>()?.Get(name),
sp.GetService<IOptionsMonitor<MetricReaderOptions>>()?.Get(name),
sp.GetService<IOptionsMonitor<ActivityExportProcessorOptions>>()?.Get(name)));

The way the source generated code works is this:

            if (AsConfigWithChildren(configuration.GetSection("DefaultOptions")) is IConfigurationSection section8)
            {
                global::OpenTelemetry.Exporter.IOtlpExporterOptions? temp10 = instance.DefaultOptions;
                if (temp10 is not null)
                {
                    BindCore(section8, ref temp10, defaultValueIfNotFound: false, binderOptions);
                }
                else
                {
                    throw new InvalidOperationException("Cannot create instance of type 'OpenTelemetry.Exporter.IOtlpExporterOptions' because it is missing a public instance constructor.");
                }
            }

It grabs the property first, and if it is a valid object, it binds the configuration to that object. If the object is null and someone set configuration for that object, it throws because an interface isn't instantiable.

Copy link
Member

Choose a reason for hiding this comment

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

@eerhardt Is there any way to suppress these closer to the thing causing the issue(s) rather than at the project level? I tried to do this...

#pragma warning disable SYSLIB1100, SYSLIB1101
        services!.Configure<OtlpExporterBuilderOptions>(name, configuration!);
#pragma warning restore SYSLIB1100, SYSLIB1101

...but it doesn't seem to work.

Copy link
Contributor

@eerhardt eerhardt Apr 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

According to dotnet/runtime#88865 this is supposed to work, but doesn't for some reason.

I've opened dotnet/runtime#100785 for this issue. EDIT: this was a duplicate of dotnet/runtime#92509

</PropertyGroup>

<ItemGroup>
Expand All @@ -19,6 +22,7 @@
<PackageReference Include="Grpc" Condition="'$(TargetFramework)' == 'netstandard2.0' OR '$(TargetFramework)' == '$(NetFrameworkMinimumSupportedVersion)'" />
<PackageReference Include="Google.Protobuf" />
<PackageReference Include="Grpc.Tools" PrivateAssets="All" />
<PackageReference Include="Microsoft.Extensions.Configuration.Binder" />
</ItemGroup>

<ItemGroup>
Expand Down
7 changes: 7 additions & 0 deletions src/Shared/Options/DelegatingOptionsFactory.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,9 @@ example of how that works.
#nullable enable

using System.Diagnostics;
#if NET6_0_OR_GREATER
using System.Diagnostics.CodeAnalysis;
#endif
using Microsoft.Extensions.Configuration;

namespace Microsoft.Extensions.Options;
Expand All @@ -24,7 +27,11 @@ namespace Microsoft.Extensions.Options;
/// Implementation of <see cref="IOptionsFactory{TOptions}"/>.
/// </summary>
/// <typeparam name="TOptions">The type of options being requested.</typeparam>
#if NET6_0_OR_GREATER
internal sealed class DelegatingOptionsFactory<[DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicParameterlessConstructor)] TOptions> :
#else
internal sealed class DelegatingOptionsFactory<TOptions> :
#endif
IOptionsFactory<TOptions>
where TOptions : class
{
Expand Down
11 changes: 11 additions & 0 deletions src/Shared/Options/DelegatingOptionsFactoryServiceCollectionExtensions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,9 @@
#nullable enable

using System.Diagnostics;
#if NET6_0_OR_GREATER
using System.Diagnostics.CodeAnalysis;
#endif
using Microsoft.Extensions.Configuration;
using Microsoft.Extensions.DependencyInjection.Extensions;
using Microsoft.Extensions.Options;
Expand All @@ -12,7 +15,11 @@ namespace Microsoft.Extensions.DependencyInjection;

internal static class DelegatingOptionsFactoryServiceCollectionExtensions
{
#if NET6_0_OR_GREATER
public static IServiceCollection RegisterOptionsFactory<[DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicConstructors)] T>(
#else
public static IServiceCollection RegisterOptionsFactory<T>(
#endif
this IServiceCollection services,
Func<IConfiguration, T> optionsFactoryFunc)
where T : class
Expand All @@ -33,7 +40,11 @@ public static IServiceCollection RegisterOptionsFactory<T>(
return services!;
}

#if NET6_0_OR_GREATER
public static IServiceCollection RegisterOptionsFactory<[DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicConstructors)] T>(
#else
public static IServiceCollection RegisterOptionsFactory<T>(
#endif
this IServiceCollection services,
Func<IServiceProvider, IConfiguration, string, T> optionsFactoryFunc)
where T : class
Expand Down
2 changes: 1 addition & 1 deletion test/OpenTelemetry.AotCompatibility.TestApp/OpenTelemetry.AotCompatibility.TestApp.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@

<PropertyGroup>
<OutputType>Exe</OutputType>
<TargetFramework>$(TargetFrameworksForAotCompatibilityTests)</TargetFramework>
<TargetFrameworks>$(TargetFrameworksForAotCompatibilityTests)</TargetFrameworks>
<PublishAot>true</PublishAot>
<TrimmerSingleWarn>false</TrimmerSingleWarn>
<SelfContained>true</SelfContained>
Expand Down