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] Added publishAOT test app to ensure OpenTelemetry SDK is AOT safe. #4392

Merged
merged 32 commits into from
Apr 21, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
32 commits
Select commit Hold shift + click to select a range
ae1849f
draft
Yun-Ting Apr 15, 2023
324a7b2
cleanup
Yun-Ting Apr 15, 2023
46ddb20
format
Yun-Ting Apr 15, 2023
4e04bc1
use net7
Yun-Ting Apr 15, 2023
3c6ee77
sanity
Yun-Ting Apr 15, 2023
60ef365
update
Yun-Ting Apr 17, 2023
e9842d6
update
Yun-Ting Apr 17, 2023
cd6e26e
publish app
Yun-Ting Apr 17, 2023
ff307ab
sln
Yun-Ting Apr 17, 2023
58b7bd1
fix proj
Yun-Ting Apr 17, 2023
460205a
sanity
Yun-Ting Apr 17, 2023
ce51034
fix path and proj
Yun-Ting Apr 17, 2023
e510df3
Merge branch 'main' into yunl/enableAOT_2
Yun-Ting Apr 17, 2023
8f3e6be
update timeout
Yun-Ting Apr 17, 2023
e08c04d
Merge branch 'main' into yunl/enableAOT_2
Yun-Ting Apr 18, 2023
4be2de8
MSBuild
Yun-Ting Apr 18, 2023
ed371fe
space
Yun-Ting Apr 18, 2023
a37eaf6
Merge branch 'main' into yunl/enableAOT_2
Yun-Ting Apr 18, 2023
09bcb4c
use Trait
Yun-Ting Apr 18, 2023
446943a
fix path
Yun-Ting Apr 18, 2023
a7eb400
assert # IL warnings
Yun-Ting Apr 19, 2023
010eed1
adding null check
Yun-Ting Apr 20, 2023
18c7afa
Merge branch 'main' into yunl/enableAOT_2
Yun-Ting Apr 20, 2023
6409cdb
Merge branch 'main' into yunl/enableAOT_2
Yun-Ting Apr 20, 2023
ea81b23
updating baseline after merge main
Yun-Ting Apr 20, 2023
61ce597
use net7
Yun-Ting Apr 21, 2023
4be236e
Merge branch 'main' into yunl/enableAOT_2
Yun-Ting Apr 21, 2023
1e9b10b
Merge branch 'main' into yunl/enableAOT_2
Yun-Ting Apr 21, 2023
618b742
fix merge
Yun-Ting Apr 21, 2023
28b81fc
CI
Yun-Ting Apr 21, 2023
29ba642
CI
Yun-Ting Apr 21, 2023
794d92c
CI
Yun-Ting Apr 21, 2023
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
16 changes: 14 additions & 2 deletions OpenTelemetry.sln
Original file line number Diff line number Diff line change
Expand Up @@ -255,9 +255,13 @@ Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "tail-based-sampling-example
EndProject
Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "stratified-sampling-example", "docs\trace\stratified-sampling-example\stratified-sampling-example.csproj", "{9C99621C-343E-479C-A943-332DB6129B71}"
EndProject
Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "links-sampler", "docs\trace\links-based-sampler\links-sampler.csproj", "{62AF4BD3-DCAE-4D44-AA5B-991C1071166B}"
Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "OpenTelemetry.Api.Tests", "test\OpenTelemetry.Api.Tests\OpenTelemetry.Api.Tests.csproj", "{FD8433F4-EDCF-475C-9B4A-625D3DE11671}"
EndProject
Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "OpenTelemetry.AotCompatibility.TestApp", "test\OpenTelemetry.AotCompatibility.TestApp\OpenTelemetry.AotCompatibility.TestApp.csproj", "{13A59BD9-9475-4991-B74D-7C20F1C63409}"
EndProject
Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "OpenTelemetry.Api.Tests", "test\OpenTelemetry.Api.Tests\OpenTelemetry.Api.Tests.csproj", "{FD8433F4-EDCF-475C-9B4A-625D3DE11671}"
Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "OpenTelemetry.AotCompatibility.Tests", "test\OpenTelemetry.AotCompatibility.Tests\OpenTelemetry.AotCompatibility.Tests.csproj", "{D438EF9C-7959-47A0-B2A2-DEBFCDC2A8DC}"
EndProject
Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "links-sampler", "docs\trace\links-based-sampler\links-sampler.csproj", "{62AF4BD3-DCAE-4D44-AA5B-991C1071166B}"
EndProject
Global
GlobalSection(SolutionConfigurationPlatforms) = preSolution
Expand Down Expand Up @@ -553,6 +557,14 @@ Global
{FD8433F4-EDCF-475C-9B4A-625D3DE11671}.Debug|Any CPU.Build.0 = Debug|Any CPU
{FD8433F4-EDCF-475C-9B4A-625D3DE11671}.Release|Any CPU.ActiveCfg = Release|Any CPU
{FD8433F4-EDCF-475C-9B4A-625D3DE11671}.Release|Any CPU.Build.0 = Release|Any CPU
{13A59BD9-9475-4991-B74D-7C20F1C63409}.Debug|Any CPU.ActiveCfg = Debug|Any CPU
{13A59BD9-9475-4991-B74D-7C20F1C63409}.Debug|Any CPU.Build.0 = Debug|Any CPU
{13A59BD9-9475-4991-B74D-7C20F1C63409}.Release|Any CPU.ActiveCfg = Release|Any CPU
{13A59BD9-9475-4991-B74D-7C20F1C63409}.Release|Any CPU.Build.0 = Release|Any CPU
{D438EF9C-7959-47A0-B2A2-DEBFCDC2A8DC}.Debug|Any CPU.ActiveCfg = Debug|Any CPU
{D438EF9C-7959-47A0-B2A2-DEBFCDC2A8DC}.Debug|Any CPU.Build.0 = Debug|Any CPU
{D438EF9C-7959-47A0-B2A2-DEBFCDC2A8DC}.Release|Any CPU.ActiveCfg = Release|Any CPU
{D438EF9C-7959-47A0-B2A2-DEBFCDC2A8DC}.Release|Any CPU.Build.0 = Release|Any CPU
EndGlobalSection
GlobalSection(SolutionProperties) = preSolution
HideSolutionNode = FALSE
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
<Project Sdk="Microsoft.NET.Sdk">

<PropertyGroup>
<OutputType>Exe</OutputType>
<TargetFramework>net7.0</TargetFramework>
<PublishAot>true</PublishAot>
<TrimmerSingleWarn>false</TrimmerSingleWarn>
</PropertyGroup>
cijothomas marked this conversation as resolved.
Show resolved Hide resolved

<ItemGroup>
<TrimmerRootAssembly Include="OpenTelemetry.Api.ProviderBuilderExtensions" />
<TrimmerRootAssembly Include="OpenTelemetry.Api" />
<TrimmerRootAssembly Include="OpenTelemetry.Exporter.Console" />
<TrimmerRootAssembly Include="OpenTelemetry.Exporter.InMemory" />
<TrimmerRootAssembly Include="OpenTelemetry.Exporter.Jaeger" />
<TrimmerRootAssembly Include="OpenTelemetry.Exporter.Prometheus.AspNetCore" />
<TrimmerRootAssembly Include="OpenTelemetry.Exporter.Prometheus.HttpListener" />
<TrimmerRootAssembly Include="OpenTelemetry.Exporter.Zipkin" />
<TrimmerRootAssembly Include="OpenTelemetry.Exporter.ZPages" />
<TrimmerRootAssembly Include="OpenTelemetry.Extensions.Hosting" />
<TrimmerRootAssembly Include="OpenTelemetry.Extensions.Propagators" />
<TrimmerRootAssembly Include="OpenTelemetry.Instrumentation.AspNetCore" />
<TrimmerRootAssembly Include="OpenTelemetry.Instrumentation.GrpcNetClient" />
<TrimmerRootAssembly Include="OpenTelemetry.Instrumentation.Http" />
<TrimmerRootAssembly Include="OpenTelemetry.Instrumentation.SqlClient" />
<TrimmerRootAssembly Include="OpenTelemetry.SemanticConventions" />
<TrimmerRootAssembly Include="OpenTelemetry.Shims.OpenTracing" />
<TrimmerRootAssembly Include="OpenTelemetry" />

<TrimmerRootAssembly Update="@(TrimmerRootAssembly)" Path="$(RepoRoot)\src\%(Identity)\%(Identity).csproj" />
<ProjectReference Include="@(TrimmerRootAssembly->'%(Path)')" />
</ItemGroup>

</Project>
17 changes: 17 additions & 0 deletions test/OpenTelemetry.AotCompatibility.TestApp/Program.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
// <copyright file="Program.cs" company="OpenTelemetry Authors">
// Copyright The OpenTelemetry Authors
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.
// </copyright>

Console.WriteLine("Hello, World!");
Original file line number Diff line number Diff line change
@@ -0,0 +1,91 @@
// <copyright file="AotCompatibilityTests.cs" company="OpenTelemetry Authors">
// Copyright The OpenTelemetry Authors
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.
// </copyright>

using System.Diagnostics;
using Xunit;
using Xunit.Abstractions;

namespace OpenTelemetry.AotCompatibility.Tests
{
public class AotCompatibilityTests
{
private readonly ITestOutputHelper testOutputHelper;

public AotCompatibilityTests(ITestOutputHelper testOutputHelper)
{
this.testOutputHelper = testOutputHelper;
}

/// <summary>
/// This test ensures that the intended APIs of the OpenTelemetry.AotCompatibility.TestApp libraries are
/// trimming and NativeAOT compatible.
///
/// This test follows the instructions in https://learn.microsoft.com/dotnet/core/deploying/trimming/prepare-libraries-for-trimming#show-all-warnings-with-sample-application
///
/// If this test fails, it is due to adding trimming and/or AOT incompatible changes
/// to code that is supposed to be compatible.
///
/// To diagnose the problem, inspect the test output which will contain the trimming and AOT errors. For example:
///
/// error IL2091: 'T' generic argument does not satisfy 'DynamicallyAccessedMemberTypes.PublicConstructors'.
/// </summary>
[Fact]
Copy link
Member

Choose a reason for hiding this comment

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

Curious - would it make sense to have the AOT test build as a separate CI job instead of using unit test here? (easier to turn on/off, can run in parallel with existing unit test pipelines, etc.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm thinking while we are still developing, i.e. the warnings are > 0, we keep this as a unit test so that it's easier to capture the expected number of warnings and run the test locally.
Once we fixed/suppressed all the warnings, we can bring it to be a CI to get the above mentioning benefits.

Copy link
Contributor

Choose a reason for hiding this comment

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

that it's easier to capture the expected number of warnings and run the test locally.

You should be able to run the CI check locally as well.

Copy link
Member

Choose a reason for hiding this comment

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

Sounds good to me 👍

public void EnsureAotCompatibility()
{
string[] paths = { @"..", "..", "..", "..", "OpenTelemetry.AotCompatibility.TestApp" };
string testAppPath = Path.Combine(paths);
string testAppProject = "OpenTelemetry.AotCompatibility.TestApp.csproj";

// ensure we run a clean publish every time
DirectoryInfo testObjDir = new DirectoryInfo(Path.Combine(testAppPath, "obj"));
if (testObjDir.Exists)
{
testObjDir.Delete(recursive: true);
}

var process = new Process
{
// set '-nodereuse:false /p:UseSharedCompilation=false' so the MSBuild and Roslyn server processes don't hang around, which may hang the test in CI
StartInfo = new ProcessStartInfo("dotnet", $"publish {testAppProject} --self-contained -nodereuse:false /p:UseSharedCompilation=false")
Copy link
Member

Choose a reason for hiding this comment

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

these can be achieved in the CI tasks itself

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for the info, Cijo : )
Please check out this comment: #4392 (comment)

{
RedirectStandardOutput = true,
UseShellExecute = false,
CreateNoWindow = true,
WorkingDirectory = testAppPath,
},
};

var expectedOutput = new System.Text.StringBuilder();
process.OutputDataReceived += (sender, e) =>
{
if (!string.IsNullOrEmpty(e.Data))
{
this.testOutputHelper.WriteLine(e.Data);
expectedOutput.AppendLine(e.Data);
}
};

process.Start();
process.BeginOutputReadLine();

Assert.True(process.WaitForExit(milliseconds: 180_000), "dotnet publish command timed out after 180 seconds.");
Assert.True(process.ExitCode == 0, "Publishing the AotCompatibility app failed. See test output for more details.");
Yun-Ting marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

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

You may want to consider a way to baseline the existing warnings to start, and ensure new warnings aren't introduced as new features are added. I'm not sure the best way to do this, but maybe @vitek-karas @agocke or @sbomer knows of a good way?

In dotnet/sdk, we use the following method to verify the warnings (see the callers above this method):

https://github.com/dotnet/sdk/blob/7d23e9d3e4aad58a5b497d8d91a50ffdf148b238/src/Tests/Microsoft.NET.Publish.Tests/GivenThatWeWantToRunILLink.cs#L803-L834

Copy link
Contributor

Choose a reason for hiding this comment

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

Other than baselining the output of the build I can't think of another way for NativeAOT right now.

ILLink supports suppressing warnings via XML files, which would be the best solution here, unfortunately that functionality isn't implemented in NativeAOT (it's on the list, but I don't know when it's going to happen).
NativeAOT does support suppressing warnings in code via assembly-level attributes, but those would have to go onto the assembly which produces the warnings, which is not useful to this case.


var warnings = expectedOutput.ToString().Split('\n', '\r').Where(line => line.Contains("warning IL"));
Assert.Equal(77, warnings.Count());
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
<Project Sdk="Microsoft.NET.Sdk">

<PropertyGroup>
<Description>Test to ensure AOT compatilibity.</Description>
<TargetFramework>net7.0</TargetFramework>
<ImplicitUsings>enable</ImplicitUsings>
<Nullable>enable</Nullable>
</PropertyGroup>

<ItemGroup>
<PackageReference Include="Microsoft.NET.Test.Sdk" Version="$(MicrosoftNETTestSdkPkgVer)" />
<PackageReference Include="xunit" Version="$(XUnitPkgVer)" />
<PackageReference Include="xunit.runner.visualstudio" Version="$(XUnitRunnerVisualStudioPkgVer)">
<PrivateAssets>all</PrivateAssets>
<IncludeAssets>runtime; build; native; contentfiles; analyzers</IncludeAssets>
</PackageReference>
<DotNetCliToolReference Include="dotnet-xunit" Version="$(DotNetXUnitCliVer)" />
</ItemGroup>

</Project>