[AOT] Fix RuntimeContext warnings #4460

Yun-Ting · 2023-05-03T23:05:13Z

Changes

Fixed 3 AoT warnings:

C:\repos\opentelemetry-dotnet\src\OpenTelemetry.Api\Context\RuntimeContext.cs(52): Trim analysis warning IL2055: OpenTelemetry.Context.RuntimeContext.RegisterSlot<T>(String): Call to 'System.Type.MakeGenericType(Type[])' can not be statically analyzed. It's not possible to guarantee the availability of requirements of the generic type. [C:\repos\opentelemetry-dotnet\test\OpenTelemetry.AotCompatibility.TestApp\OpenTelemetry.AotCompatibility.TestApp.csproj]
C:\repos\opentelemetry-dotnet\src\OpenTelemetry.Api\Context\RuntimeContext.cs(52): AOT analysis warning IL3050: OpenTelemetry.Context.RuntimeContext.RegisterSlot<T>(String): Using member 'System.Type.MakeGenericType(Type[])' which has 'RequiresDynamicCodeAttribute' can break functionality when AOT compiling. The native code for this instantiation might not be available at runtime. [C:\repos\opentelemetry-dotnet\test\OpenTelemetry.AotCompatibility.TestApp\OpenTelemetry.AotCompatibility.TestApp.csproj]
C:\repos\opentelemetry-dotnet\src\OpenTelemetry.Api\Context\RuntimeContext.cs(53): Trim analysis warning IL2075: OpenTelemetry.Context.RuntimeContext.RegisterSlot<T>(String): 'this' argument does not satisfy 'DynamicallyAccessedMemberTypes.PublicConstructors' in call to 'System.Type.GetConstructor(Type[])'. The return value of method 'OpenTelemetry.Context.RuntimeContext.ContextSlotType.get' 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:\repos\opentelemetry-dotnet\test\OpenTelemetry.AotCompatibility.TestApp\OpenTelemetry.AotCompatibility.TestApp.csproj]

Towards #3429
Related to: #3429 (comment)

Enumerate 4 different ContextSlotType:
AsyncLocalRuntimeContextSlot (the default type),
ThreadLocalRuntimeContextSlot,
RemotingRuntimeContextSlot and
ReflectionRuntimeContextSlot.
Handle
var type = ContextSlotType.MakeGenericType(typeof(T));
by adding attributes to mark ReflectionRuntimeContextSlotFactory trimming/AoT unsafe, that requires using Reflection to get the user-accustomed RuntimeContextSlot.

[RequiresUnreferencedCode("ReflectionRuntimeContextSlotFactory is trimmer unsafe.")]
[RequiresDynamicCode("ReflectionRuntimeContextSlotFactory requires the ability to generate new code at runtime.")]

Merge requirement checklist

CONTRIBUTING guidelines followed (nullable enabled, static analysis, etc.)
Unit tests added/updated
Appropriate CHANGELOG.md files updated for non-trivial changes
Changes in public API reviewed (if applicable)

src/OpenTelemetry.Api/Context/RuntimeContext.cs

codecov · 2023-05-08T21:16:45Z

Codecov Report

Merging #4460 (9ba6626) into main (f4f4469) will decrease coverage by 0.14%.
The diff coverage is 28.57%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4460      +/-   ##
==========================================
- Coverage   85.16%   85.02%   -0.14%     
==========================================
  Files         316      319       +3     
  Lines       12583    12614      +31     
==========================================
+ Hits        10716    10725       +9     
- Misses       1867     1889      +22

Impacted Files	Coverage Δ
src/OpenTelemetry.Api/Baggage.cs	`100.00% <ø> (ø)`
...Api/Internal/Shims/RequiresDynamicCodeAttribute.cs	`0.00% <0.00%> (ø)`
...nternal/Shims/RequiresUnreferencedCodeAttribute.cs	`0.00% <0.00%> (ø)`
...nal/Shims/UnconditionalSuppressMessageAttribute.cs	`0.00% <ø> (ø)`
src/OpenTelemetry.Api/Trace/Tracer.cs	`97.22% <ø> (ø)`
src/OpenTelemetry/Logs/LoggerProviderSdk.cs	`92.78% <ø> (ø)`
...Telemetry.Api/Context/RuntimeContextSlotFactory.cs	`11.11% <11.11%> (ø)`
src/OpenTelemetry.Api/Context/RuntimeContext.cs	`57.77% <56.25%> (-5.86%)`	⬇️

... and 3 files with indirect coverage changes

reyang

LGTM with a changelog entry: "A System.RuntimeException will be thrown if a custom RuntimeContextSlot is used."

eerhardt

Thanks!

src/OpenTelemetry.Api/Context/RuntimeContext.cs

This reverts commit cc00b30.

eerhardt · 2023-05-17T02:42:37Z

src/OpenTelemetry/Internal/OpenTelemetrySdkEventSource.cs

@@ -15,6 +15,7 @@
 // </copyright>

 #nullable enable
+#pragma warning disable 0436


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

eerhardt · 2023-05-17T02:43:05Z

src/OpenTelemetry.Api/OpenTelemetry.Api.csproj

+    <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'" />


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'" />

…/opentelemetry-dotnet into yunl/runtimeContextEnum

Yun-Ting · 2023-05-19T20:35:42Z

src/OpenTelemetry/Logs/LoggerProviderSdk.cs

@@ -201,7 +204,12 @@ public bool ContainsBatchProcessor(BaseProcessor<LogRecord> processor)
    }

    /// <inheritdoc />
-    protected override bool TryCreateLogger(string? name, out Logger? logger)
+    protected override bool TryCreateLogger(


This is for Fixing:
https://github.com/open-telemetry/opentelemetry-dotnet/actions/runs/5009473252/jobs/8978448877

eerhardt

LGTM.

Since the previous version doesn't have a net6.0 target, run ApiCompat for net6.0 against the netstandard2.0 target.

docs/logs/correlation/README.md

CodeBlanch · 2023-05-24T00:18:49Z

src/OpenTelemetry.Api/Internal/Shims/RequiresDynamicCodeAttribute.cs

+// </copyright>
+
+#nullable enable
+#if !NET7_0_OR_GREATER


Don't we need a net7.0 target for this to work?

This attribute is in the runtime of .net7.0 and .net8.0: https://learn.microsoft.com/en-us/dotnet/api/system.diagnostics.codeanalysis.requiresdynamiccodeattribute?view=net-7.0

Therefore, we will only need to compile this file if it is not net7.0 or greater.

It's basically future proofing the code. That way if/when we add a net7.0+ target, this code doesn't get compiled for it. Yes it isn't strictly necessary now, but it is saving some effort/confusion for a future dev who adds a new TFM.

This kind of forces us to always add the new target to the API project as well. If in the future, the SDK project also ends up using this attribute, we wouldn't be able to add a net7.0+ target only to the SDK project as it would complain about the attribute existing in both OpenTelemetry.Api (because of InternalsVisible) and System.Runtime.

Do you have a recommendation to resolve this? The only other option I could imagine is to #if NET7_OR_GREATER where we need to use this attribute AND add a net7.0 target now in order to use the attribute.

This kind of forces us to always add the new target to the API project as well

Using InternalsVisibleTo across separate NuGet packages isn't recommended, since the package versions can get out of sync and users will see errors.

In general, matching TFMs across these packages is preferrable. It makes maintenance much easier. It is how we manage our packages we ship out of dotnet/runtime. They all have a consistent set of targets. For the 8.0-* packages it is: net6.0;net7.0;net8.0;netstandard2.0;net462. For example see https://www.nuget.org/packages/System.Collections.Immutable/8.0.0-preview.4.23259.5#dependencies-body-tab:

utpilla · 2023-05-24T01:48:16Z

src/OpenTelemetry.Api/Baggage.cs

@@ -17,6 +17,8 @@
 using OpenTelemetry.Context;
 using OpenTelemetry.Internal;

+#pragma warning disable RS0026 // Do not add multiple overloads with optional parameters.


You should scope it down to individual methods instead of suppressing the warning for the entire file.

Consider using the SuppressMessage attribute for each of the methods that it warns about.

src/OpenTelemetry.Api/Internal/Shims/RequiresUnreferencedCodeAttribute.cs

github-actions · 2023-06-02T03:16:45Z

This PR was marked stale due to lack of activity and will be closed in 7 days. Commenting or Pushing will instruct the bot to automatically remove the label. This bot runs once per day.

eerhardt · 2023-06-02T14:19:33Z

@Yun-Ting - what's the status on this PR? I think we should get this change merged, as it fixes the major blocking issue when using OpenTelemetry in an AOT'd application.

utpilla · 2023-06-02T22:38:56Z

@Yun-Ting - what's the status on this PR? I think we should get this change merged, as it fixes the major blocking issue when using OpenTelemetry in an AOT'd application.

@eerhardt We have decided to drop the support for allowing custom types. That makes the changes very simple. This PR had originally started with that, but we wanted to understand how difficult it would be to continue supporting the custom types. Sorry for the confusion. Here's the PR #4542

Yun-Ting · 2023-06-02T22:43:11Z

@Yun-Ting - what's the status on this PR? I think we should get this change merged, as it fixes the major blocking issue when using OpenTelemetry in an AOT'd application.

Hi @eerhardt, after discussions, we decided to drop the custom type and make this a breaking change:
#4542 (review)
(Apologies for the hassle along the way.)

As adapting AOT attributes to different .NET framework / runtime would add another layer of complexity when it comes to maintaining the code base (i.e. https://github.com/open-telemetry/opentelemetry-dotnet/blob/9ba6626f7e94b94e6e4e51f78f51090fde80b215/src/OpenTelemetry.Api/Internal/Shims/RequiresUnreferencedCodeAttribute.cs#LL18C1-L18C46).

(I had been discussed with @vitek-karas in making the internal AOT attributes a package with the preprocessors but we haven't had a conclusion yet.)

Yun-Ting commented May 3, 2023

View reviewed changes

src/OpenTelemetry.Api/Context/RuntimeContext.cs Outdated Show resolved Hide resolved

Yun-Ting added 2 commits May 9, 2023 08:48

initial commit

87cfa2a

Merge branch 'main' into yunl/runtimeContextEnum

bf8a530

Yun-Ting force-pushed the yunl/runtimeContextEnum branch from 7a578ad to bf8a530 Compare May 9, 2023 15:49

Yun-Ting marked this pull request as ready for review May 9, 2023 15:58

Yun-Ting requested a review from a team May 9, 2023 15:58

reyang approved these changes May 9, 2023

View reviewed changes

eerhardt approved these changes May 9, 2023

View reviewed changes

utpilla reviewed May 9, 2023

View reviewed changes

src/OpenTelemetry.Api/Context/RuntimeContext.cs Show resolved Hide resolved

utpilla reviewed May 9, 2023

View reviewed changes

src/OpenTelemetry.Api/Context/RuntimeContext.cs Show resolved Hide resolved

Yun-Ting added 9 commits May 11, 2023 15:46

add custom type

b133982

CI

236a5f1

update

d4c38af

update

bd2d709

update

db04bd1

fix sanity check

260327b

format

6be9d0c

static ctor

5dae942

added support for ReflectionRuntimeContextSlotFactory

ebee07a

Yun-Ting force-pushed the yunl/runtimeContextEnum branch from 8188408 to ebee07a Compare May 16, 2023 22:37

Yun-Ting added 6 commits May 16, 2023 15:39

sanity

299fc2a

Merge branch 'main' into yunl/runtimeContextEnum

e93903f

test CI

cc00b30

Revert "test CI"

99c8baa

This reverts commit cc00b30.

CI

a773917

update

fc32b25

eerhardt reviewed May 17, 2023

View reviewed changes

Yun-Ting added 2 commits May 17, 2023 14:43

Merge branch 'yunl/runtimeContextEnum' of https://github.com/Yun-Ting…

6ef81cd

…/opentelemetry-dotnet into yunl/runtimeContextEnum

comment

23b77b5

Yun-Ting added 4 commits May 19, 2023 10:39

address comments

d2024a8

sanity check

b9c0b33

fix test

4120ffb

indentation

633da1b

Yun-Ting commented May 19, 2023

View reviewed changes

Yun-Ting added 3 commits May 19, 2023 13:39

increased timeout and removed RS0026

7c1fde4

RS0026

48d0b7f

CI

19fce6a

eerhardt approved these changes May 22, 2023

View reviewed changes

Fix ApiCompat for OpenTelemetry.Api's net6.0 target.

35173c8

Since the previous version doesn't have a net6.0 target, run ApiCompat for net6.0 against the netstandard2.0 target.

eerhardt force-pushed the yunl/runtimeContextEnum branch from d8373a5 to 35173c8 Compare May 22, 2023 15:58

CI

21495bc

utpilla reviewed May 22, 2023

View reviewed changes

docs/logs/correlation/README.md Outdated Show resolved Hide resolved

Merge branch 'main' into yunl/runtimeContextEnum

586511d

CodeBlanch reviewed May 24, 2023

View reviewed changes

fixed merge; scope down suppresion of RS0026

9ba6626

utpilla reviewed May 24, 2023

View reviewed changes

alanwest reviewed May 25, 2023

View reviewed changes

src/OpenTelemetry.Api/Internal/Shims/RequiresUnreferencedCodeAttribute.cs Show resolved Hide resolved

eerhardt mentioned this pull request Jun 1, 2023

[AOT] Resolve ConfigurationExtensions and EventSource warnings #4534

Merged

github-actions bot added the Stale Issues and pull requests which have been flagged for closing due to inactivity label Jun 2, 2023

Yun-Ting closed this Jun 2, 2023

This was referenced Jun 8, 2023

[AOT] Adding annotation attributes. #4562

Closed

[AOT] Adding annotation attributes. vitek-karas/opentelemetry-dotnet#1

Closed

[AOT] Adding annotation attributes. #4564

Closed

vitek-karas mentioned this pull request Jun 12, 2023

Adds trimming and AOT related attribute definitions for down-level TFMs #4580

Closed

4 tasks

Yun-Ting mentioned this pull request Jun 14, 2023

What is the current protocol in deciding what TFM to use for a project? #4591

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[AOT] Fix RuntimeContext warnings #4460

[AOT] Fix RuntimeContext warnings #4460

Yun-Ting commented May 3, 2023 •

edited

Loading

codecov bot commented May 8, 2023 •

edited

Loading

reyang left a comment

eerhardt left a comment

eerhardt May 17, 2023

eerhardt May 17, 2023

Yun-Ting May 19, 2023

eerhardt left a comment

CodeBlanch May 24, 2023

Yun-Ting May 24, 2023

eerhardt May 24, 2023

utpilla May 24, 2023

eerhardt May 25, 2023

utpilla May 24, 2023

github-actions bot commented Jun 2, 2023

eerhardt commented Jun 2, 2023

utpilla commented Jun 2, 2023

Yun-Ting commented Jun 2, 2023 •

edited

Loading

[AOT] Fix RuntimeContext warnings #4460

[AOT] Fix RuntimeContext warnings #4460

Conversation

Yun-Ting commented May 3, 2023 • edited Loading

Changes

Merge requirement checklist

codecov bot commented May 8, 2023 • edited Loading

Codecov Report

reyang left a comment

Choose a reason for hiding this comment

eerhardt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eerhardt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Jun 2, 2023

eerhardt commented Jun 2, 2023

utpilla commented Jun 2, 2023

Yun-Ting commented Jun 2, 2023 • edited Loading

Yun-Ting commented May 3, 2023 •

edited

Loading

codecov bot commented May 8, 2023 •

edited

Loading

Yun-Ting commented Jun 2, 2023 •

edited

Loading