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] Fix RuntimeContext warnings #4460

Closed
wants to merge 41 commits into from
Closed
Show file tree
Hide file tree
Changes from 17 commits
Commits
Show all changes
41 commits
Select commit Hold shift + click to select a range
87cfa2a
initial commit
Yun-Ting May 9, 2023
bf8a530
Merge branch 'main' into yunl/runtimeContextEnum
Yun-Ting May 9, 2023
b133982
add custom type
Yun-Ting May 11, 2023
236a5f1
CI
Yun-Ting May 12, 2023
d4c38af
update
Yun-Ting May 12, 2023
bd2d709
update
Yun-Ting May 12, 2023
db04bd1
update
Yun-Ting May 12, 2023
260327b
fix sanity check
Yun-Ting May 12, 2023
6be9d0c
format
Yun-Ting May 12, 2023
5dae942
static ctor
Yun-Ting May 15, 2023
ebee07a
added support for ReflectionRuntimeContextSlotFactory
Yun-Ting May 16, 2023
299fc2a
sanity
Yun-Ting May 16, 2023
e93903f
Merge branch 'main' into yunl/runtimeContextEnum
Yun-Ting May 16, 2023
cc00b30
test CI
Yun-Ting May 16, 2023
99c8baa
Revert "test CI"
Yun-Ting May 17, 2023
a773917
CI
Yun-Ting May 17, 2023
fc32b25
update
Yun-Ting May 17, 2023
6ef81cd
Merge branch 'yunl/runtimeContextEnum' of https://github.com/Yun-Ting…
Yun-Ting May 17, 2023
23b77b5
comment
Yun-Ting May 17, 2023
6faf599
update
Yun-Ting May 17, 2023
e6b016c
ci
Yun-Ting May 17, 2023
44bb210
CI
Yun-Ting May 18, 2023
8d76270
preprossesor
Yun-Ting May 18, 2023
05bb4b6
fix build
Yun-Ting May 18, 2023
8586200
Add the trimming attributes to only OpenTelemetry.Api, and use #if to…
eerhardt May 18, 2023
452f6a6
CI
Yun-Ting May 18, 2023
649e39d
Adjust the folder path to the attributes.
eerhardt May 18, 2023
f0c7f44
CI
Yun-Ting May 18, 2023
f8714f9
Fix nullable warning in LoggerProviderSdk
eerhardt May 18, 2023
7956ab1
CI
Yun-Ting May 18, 2023
d2024a8
address comments
Yun-Ting May 19, 2023
b9c0b33
sanity check
Yun-Ting May 19, 2023
4120ffb
fix test
Yun-Ting May 19, 2023
633da1b
indentation
Yun-Ting May 19, 2023
7c1fde4
increased timeout and removed RS0026
Yun-Ting May 19, 2023
48d0b7f
RS0026
Yun-Ting May 19, 2023
19fce6a
CI
Yun-Ting May 19, 2023
35173c8
Fix ApiCompat for OpenTelemetry.Api's net6.0 target.
eerhardt May 22, 2023
21495bc
CI
Yun-Ting May 22, 2023
586511d
Merge branch 'main' into yunl/runtimeContextEnum
eerhardt May 22, 2023
9ba6626
fixed merge; scope down suppresion of RS0026
Yun-Ting May 24, 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
64 changes: 60 additions & 4 deletions src/OpenTelemetry.Api/Context/RuntimeContext.cs
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
// </copyright>

using System.Collections.Concurrent;
using System.Diagnostics.CodeAnalysis;
using System.Runtime.CompilerServices;
using OpenTelemetry.Internal;

Expand All @@ -27,10 +28,66 @@ public static class RuntimeContext
{
private static readonly ConcurrentDictionary<string, object> Slots = new();

private static Type contextSlotType;

private static RuntimeContextSlotFactory runtimeContextSlotFactory;

[UnconditionalSuppressMessage("ReflectionAnalysis", "IL2026:RequiresUnreferencedCode", Justification = "User-defined RuntimeContextSlotFactory that relies on Reflection is not timmer safe.")]
[UnconditionalSuppressMessage("ReflectionAnalysis", "IL3050:RequiresDynamicCode", Justification = "User-defined RuntimeContextSlotFactory that relies on Reflection is not AoT safe.")]
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
[UnconditionalSuppressMessage("ReflectionAnalysis", "IL2026:RequiresUnreferencedCode", Justification = "User-defined RuntimeContextSlotFactory that relies on Reflection is not timmer safe.")]
[UnconditionalSuppressMessage("ReflectionAnalysis", "IL3050:RequiresDynamicCode", Justification = "User-defined RuntimeContextSlotFactory that relies on Reflection is not AoT safe.")]
[UnconditionalSuppressMessage("ReflectionAnalysis", "IL2026:RequiresUnreferencedCode", Justification = "The AsyncLocalRuntimeContextSlot type is handled without using Reflection..")]
[UnconditionalSuppressMessage("ReflectionAnalysis", "IL3050:RequiresDynamicCode", Justification = "The AsyncLocalRuntimeContextSlot type is handled without using Reflection.")]

Copy link
Contributor Author

@Yun-Ting Yun-Ting May 19, 2023

Choose a reason for hiding this comment

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

For code reviewers:
These 2 attributes are suppressing the below warnings

C:\repos\opentelemetry-dotnet\src\OpenTelemetry.Api\Context\RuntimeContext.cs(39): Trim analysis warning IL2026: OpenTelemetry.Context.RuntimeContext..cctor(): Using member 'OpenTelemetry.Context.RuntimeContext.ContextSlotType.set' which has 'RequiresUnreferencedCodeAttribute' can break functionality when trimming application code. Setting a custom ContextSlotType requires using Reflection to initialize the context slot. [C:\repos\opentelemetry-dotnet\test\OpenTelemetry.AotCompatibility.TestApp\OpenTelemetry.AotCompatibility.TestApp.csproj]
C:\repos\opentelemetry-dotnet\src\OpenTelemetry.Api\Context\RuntimeContext.cs(39): AOT analysis warning IL3050: OpenTelemetry.Context.RuntimeContext..cctor(): Using member 'OpenTelemetry.Context.RuntimeContext.ContextSlotType.set' which has 'RequiresDynamicCodeAttribute' can break functionality when AOT compiling. Setting a custom ContextSlotType requires using MakeGenericType to initialize the context slot. The native code for the generic type might not be available at runtime. [C:\repos\opentelemetry-dotnet\test\OpenTelemetry.AotCompatibility.TestApp\OpenTelemetry.AotCompatibility.TestApp.csproj]

static RuntimeContext()
{
ContextSlotType = typeof(AsyncLocalRuntimeContextSlot<>);
}

/// <summary>
/// Gets or sets the actual context carrier implementation.
/// </summary>
public static Type ContextSlotType { get; set; } = typeof(AsyncLocalRuntimeContextSlot<>);
public static Type ContextSlotType
{
get
{
return contextSlotType;
}

[RequiresUnreferencedCode("ReflectionRuntimeContextSlotFactory is trimmer unsafe.")]
[RequiresDynamicCode("ReflectionRuntimeContextSlotFactory requires the ability to generate new code at runtime.")]
Yun-Ting marked this conversation as resolved.
Show resolved Hide resolved
set
{
Guard.ThrowIfNull(value, nameof(value));
if (!value.IsGenericType || !value.IsGenericTypeDefinition || value.GetGenericArguments().Length != 1)
{
throw new NotSupportedException($"Type '{value}' must be generic with a single generic type argument.");
}

if (value == typeof(AsyncLocalRuntimeContextSlot<>))
{
runtimeContextSlotFactory = new RuntimeContextSlotFactory.AsyncLocalRuntimeContextSlotFactory();
}
else if (value == typeof(ThreadLocalRuntimeContextSlot<>))
{
runtimeContextSlotFactory = new RuntimeContextSlotFactory.ThreadLocalRuntimeContextSlotFactory();
}
#if NETFRAMEWORK
else if (value == typeof(RemotingRuntimeContextSlot<>))
{
runtimeContextSlotFactory = new RuntimeContextSlotFactory.RemotingRuntimeContextSlotFactory();
}
#endif
else
{
#if NETSTANDARD2_1_OR_GREATER || NET6_OR_GREATER
if (!RuntimeFeature.IsDynamicCodeSupported)
{
throw new NotSupportedException($"Custom RuntimeContextSlot type '{value}' cannot be used because dynamic code is not supported");
}
#endif

runtimeContextSlotFactory = new RuntimeContextSlotFactory.ReflectionRuntimeContextSlotFactory(contextSlotType);
}

contextSlotType = value;
}
}

/// <summary>
/// Register a named context slot.
Expand All @@ -49,9 +106,8 @@ public static RuntimeContextSlot<T> RegisterSlot<T>(string slotName)
throw new InvalidOperationException($"Context slot already registered: '{slotName}'");
}

var type = ContextSlotType.MakeGenericType(typeof(T));
var ctor = type.GetConstructor(new Type[] { typeof(string) });
var slot = (RuntimeContextSlot<T>)ctor.Invoke(new object[] { slotName });
var slot = runtimeContextSlotFactory.Create<T>(slotName);
utpilla marked this conversation as resolved.
Show resolved Hide resolved
utpilla marked this conversation as resolved.
Show resolved Hide resolved

Slots[slotName] = slot;
return slot;
}
Expand Down
66 changes: 66 additions & 0 deletions src/OpenTelemetry.Api/Context/RuntimeContextSlotFactory.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,66 @@
// <copyright file="RuntimeContextSlotFactory.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>

#nullable enable

using System.Diagnostics.CodeAnalysis;

namespace OpenTelemetry.Context
{
internal abstract class RuntimeContextSlotFactory
{
public abstract RuntimeContextSlot<T> Create<T>(string name);

public sealed class AsyncLocalRuntimeContextSlotFactory : RuntimeContextSlotFactory
{
public override RuntimeContextSlot<T> Create<T>(string name)
=> new AsyncLocalRuntimeContextSlot<T>(name);
}

#if NETFRAMEWORK
public sealed class RemotingRuntimeContextSlotFactory : RuntimeContextSlotFactory
{
public override RuntimeContextSlot<T> Create<T>(string name)
=> new RemotingRuntimeContextSlot<T>(name);
}
#endif

public sealed class ThreadLocalRuntimeContextSlotFactory : RuntimeContextSlotFactory
{
public override RuntimeContextSlot<T> Create<T>(string name)
=> new ThreadLocalRuntimeContextSlot<T>(name);
Yun-Ting marked this conversation as resolved.
Show resolved Hide resolved
}

[RequiresUnreferencedCode("ReflectionRuntimeContextSlotFactory is trimmer unsafe.")]
[RequiresDynamicCode("ReflectionRuntimeContextSlotFactory requires the ability to generate new code at runtime.")]
public sealed class ReflectionRuntimeContextSlotFactory : RuntimeContextSlotFactory
{
private readonly Type runtimeContextSlotType;

public ReflectionRuntimeContextSlotFactory(Type runtimeContextSlotType)
{
this.runtimeContextSlotType = runtimeContextSlotType;
}

public override RuntimeContextSlot<T> Create<T>(string name)
{
var type = this.runtimeContextSlotType.MakeGenericType(typeof(T));
var ctor = type.GetConstructor(new Type[] { typeof(string) });
return (RuntimeContextSlot<T>)ctor.Invoke(new object[] { name });
}
}
}
}
6 changes: 6 additions & 0 deletions src/OpenTelemetry.Api/OpenTelemetry.Api.csproj
Original file line number Diff line number Diff line change
Expand Up @@ -15,4 +15,10 @@
<ItemGroup>
<PackageReference Include="System.Diagnostics.DiagnosticSource" />
</ItemGroup>

<ItemGroup>
<Compile Include="$(RepoRoot)\src\OpenTelemetry\Internal\Shims\RequiresDynamicCodeAttribute.cs" Link ="Includes\RequiresDynamicCodeAttribute.cs" />
<Compile Include="$(RepoRoot)\src\OpenTelemetry\Internal\Shims\RequiresUnreferencedCodeAttribute.cs" Link="Includes\RequiresUnreferencedCodeAttribute.cs" />
<Compile Include="$(RepoRoot)\src\OpenTelemetry\Internal\Shims\UnconditionalSuppressMessageAttribute.cs" Link ="Includes\UnconditionalSuppressMessageAttribute.cs" Condition="'$(TargetFrameworkIdentifier)' != '.NETCoreApp'" />
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
<Compile Include="$(RepoRoot)\src\OpenTelemetry\Internal\Shims\RequiresDynamicCodeAttribute.cs" Link ="Includes\RequiresDynamicCodeAttribute.cs" />
<Compile Include="$(RepoRoot)\src\OpenTelemetry\Internal\Shims\RequiresUnreferencedCodeAttribute.cs" Link="Includes\RequiresUnreferencedCodeAttribute.cs" />
<Compile Include="$(RepoRoot)\src\OpenTelemetry\Internal\Shims\UnconditionalSuppressMessageAttribute.cs" Link ="Includes\UnconditionalSuppressMessageAttribute.cs" Condition="'$(TargetFrameworkIdentifier)' != '.NETCoreApp'" />
<Compile Include="$(RepoRoot)\src\OpenTelemetry\Internal\Shims\RequiresDynamicCodeAttribute.cs" Link ="Includes\RequiresDynamicCodeAttribute.cs" Condition="'$(TargetFrameworkIdentifier)' != '.NETCoreApp'" />
<Compile Include="$(RepoRoot)\src\OpenTelemetry\Internal\Shims\RequiresUnreferencedCodeAttribute.cs" Link="Includes\RequiresUnreferencedCodeAttribute.cs" Condition="'$(TargetFrameworkIdentifier)' != '.NETCoreApp'" />
<Compile Include="$(RepoRoot)\src\OpenTelemetry\Internal\Shims\UnconditionalSuppressMessageAttribute.cs" Link ="Includes\UnconditionalSuppressMessageAttribute.cs" Condition="'$(TargetFrameworkIdentifier)' != '.NETCoreApp'" />

</ItemGroup>
</Project>
2 changes: 2 additions & 0 deletions src/OpenTelemetry/Internal/OpenTelemetrySdkEventSource.cs
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
// </copyright>

#nullable enable
#pragma warning disable 0436
Copy link
Contributor

Choose a reason for hiding this comment

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

We should remove this. Here is what is happening:

we have 2 projects:

OpenTelemetry.Api (targets netstandard2.0;net462)
OpenTelemetry (targets net6.0;netstandard2.1;netstandard2.0;net462) and references OpenTelemetry.Api

The UnconditionalSuppressMessageAttribute is defined in net6.0, but isn't defined in netstandard2.0. When we build OpenTelemetry.Api, we are putting the type UnconditionalSuppressMessageAttribute as an internal type into OpenTelemetery.Api, since that attribute isn't defined in netstandard2.0.

Normally this is fine because only that assembly sees types that are internal. However, OpenTelemetry can see OpenTelemetry.Api's internals because it has an InternalsVisibleTo relationship.

When OpenTelemetry builds for net6.0, it sees 2 different UnconditionalSuppressMessageAttribute types - one in the OpenTelemetry.Api netstandard2.0 assembly (through internals visible to). and one in .NET itself. So the compiler complains.

The easiest fix for this is to change the TargetFrameworks property in OpenTelemetry.Api.csproj to be:

<TargetFrameworks>net6.0;netstandard2.0;net462</TargetFrameworks>

(add net6.0)

Then when OpenTelemetry builds for net6.0, it won't see the attribute from OpenTelemetry.Api anymore, since it won't be there. It will only see the attribute from the framework in net6.0

using System.Diagnostics;
using System.Diagnostics.CodeAnalysis;
using System.Diagnostics.Tracing;
Expand Down Expand Up @@ -370,3 +371,4 @@ protected override void OnEventWritten(EventWrittenEventArgs e)
#endif
}
}
#pragma warning restore 0436
59 changes: 59 additions & 0 deletions src/OpenTelemetry/Internal/Shims/RequiresDynamicCodeAttribute.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,59 @@
// <copyright file="RequiresDynamicCodeAttribute.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>

#nullable enable

namespace System.Diagnostics.CodeAnalysis
{
/// <summary>
/// Indicates that the specified method requires the ability to generate new code at runtime,
/// for example through <see cref="Reflection"/>.
/// </summary>
/// <remarks>
/// This allows tools to understand which methods are unsafe to call when compiling ahead of time.
/// </remarks>
[AttributeUsage(AttributeTargets.Method | AttributeTargets.Constructor | AttributeTargets.Class, Inherited = false)]
#if SYSTEM_PRIVATE_CORELIB
public
#else
internal
#endif
sealed class RequiresDynamicCodeAttribute : Attribute
{
/// <summary>
/// Initializes a new instance of the <see cref="RequiresDynamicCodeAttribute"/> class
/// with the specified message.
/// </summary>
/// <param name="message">
/// A message that contains information about the usage of dynamic code.
/// </param>
public RequiresDynamicCodeAttribute(string message)
{
this.Message = message;
}

/// <summary>
/// Gets a message that contains information about the usage of dynamic code.
/// </summary>
public string Message { get; }

/// <summary>
/// Gets or sets an optional URL that contains more information about the method,
/// why it requires dynamic code, and what options a consumer has to deal with it.
/// </summary>
public string? Url { get; set; }
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,55 @@
// <copyright file="RequiresUnreferencedCodeAttribute.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>

#nullable enable

namespace System.Diagnostics.CodeAnalysis
{
/// <summary>
/// Indicates that the specified method requires dynamic access to code that is not referenced
/// statically, for example through <see cref="Reflection"/>.
/// </summary>
/// <remarks>
/// This allows tools to understand which methods are unsafe to call when removing unreferenced
/// code from an application.
/// </remarks>
[AttributeUsage(AttributeTargets.Method | AttributeTargets.Constructor | AttributeTargets.Class, Inherited = false)]
internal sealed class RequiresUnreferencedCodeAttribute : Attribute
{
/// <summary>
/// Initializes a new instance of the <see cref="RequiresUnreferencedCodeAttribute"/> class
/// with the specified message.
/// </summary>
/// <param name="message">
/// A message that contains information about the usage of unreferenced code.
/// </param>
public RequiresUnreferencedCodeAttribute(string message)
{
this.Message = message;
}

/// <summary>
/// Gets a message that contains information about the usage of unreferenced code.
/// </summary>
public string Message { get; }

/// <summary>
/// Gets or sets an optional URL that contains more information about the method,
/// why it requires unreferenced code, and what options a consumer has to deal with it.
/// </summary>
public string? Url { get; set; }
}
}
4 changes: 0 additions & 4 deletions src/OpenTelemetry/OpenTelemetry.csproj
Original file line number Diff line number Diff line change
Expand Up @@ -21,8 +21,4 @@
<ProjectReference Include="$(RepoRoot)\src\OpenTelemetry.Api.ProviderBuilderExtensions\OpenTelemetry.Api.ProviderBuilderExtensions.csproj" />
</ItemGroup>

<ItemGroup>
<Compile Remove="Internal\Shims\UnconditionalSuppressMessageAttribute.cs" Condition="'$(TargetFrameworkIdentifier)' == '.NETCoreApp'" />
</ItemGroup>

</Project>
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,7 @@ public void EnsureAotCompatibility()
Assert.True(process.ExitCode == 0, "Publishing the AotCompatibility app failed. See test output for more details.");

var warnings = expectedOutput.ToString().Split('\n', '\r').Where(line => line.Contains("warning IL"));
Assert.Equal(58, warnings.Count());
Assert.Equal(55, warnings.Count());
}
}
}