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

[vitek-karas]

Related to #3429

Changes

Adds the definition of RequiresUnreferencedCodeAttribute, RequiresDynamicCodeAttribute and DynamicallyAccessedMembersAttribute when one is not provided by the framework (so for anything but .NET6+).

There's a complexity associated with this change. Normally the attributes are added as internal to any build target which doesn't have them in the framework. For example, if an assembly is built for netstandard2.0 an internal definition of the attributes would be added to the compilation. All the tools consuming the attributes only look at the Namespace.TypeName of the attribute and they ignore which assembly the attribute came from.

OpenTelemetry builds some assemblies for net6.0 while other assemblies are just netstandard2.0. For example, OpenTelemetry targets net6.0, but its dependency OpenTelemetry.Api only targets netstandard2.0. That would normally not be a problem since the added attribute definitions in OpenTelemetry.Api are internal. Unfortunately, OpenTelemetry has InternalsVisibleTo into OpenTelemetry.Api. When building the net6.0 target of OpenTelemetry the compiler sees the attributes defined in the framework as well as the ones in OpenTelemetry.Api.

This situation has already been discussed in #4460 (comment).

The solution implemented in this change is to add net6.0 as a target for OpenTelemetry.Api. That this does is that when building OpenTelemetry for net6.0 it builds against OpenTelemetry.Api for net6.0 as well and neither assembly will have the attributes defined internally, they will all come from the framework.

This internal attribute definitions are in OpenTelemetry.Api project and are compiled into other assemblies as necessary. Part of this change moves the UnconditionalSuppressedMessageAttribute (which is used for trimming/AOT suppressions) to OpenTelemetry.Api as well to keep all of those attributes together.

It also adds these attributes to the compilation whenever the PropertyFetcher.cs is added. This is currently not needed in this PR, but it will be needed in the future, and it was cleanest to do it here. It also allows for testing that the ifdefs are correct across the projects.

The attribute definitions are copied from dotnet/runtime repo and I modified them to fit the code style in this repo - so I changed the file headers and some other small formatting changes. They are also defined as internal instead of public.

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)

[linux-foundation-easycla]

The committers listed above are authorized under a signed CLA.

• ✅ login: vitek-karas / name: Vitek Karas (7439f07, 2cb85bc, 7364854, 35bb940, ab9b068, 79eeae4, 1d7ccae, fa8bc61, 21baea0, 7cc3277)
• ✅ login: eerhardt / name: Eric Erhardt (79eeae4)
• ✅ login: Yun-Ting / name: Yun-Ting Lin (bc35f0b, 344f036, d151841)

[eerhardt]

What exactly does "descending order" mean here? Is this still in "descending order"?

[vitek-karas]

Thanks for pointing this out. This seems to originate here: #2015 (comment)

And that in turn says that VS Code only uses the first TFM in the list. So this would change that selection to .NET 6.
Personally I would argue that's a good thing. And it's also consistent with OpenTelemetry.csproj which also starts with net6.0.

[eerhardt]

Who sets IncludeCodeAnalysisAttributes? I'm not seeing it used.

[vitek-karas]

Currently no one. So maybe I should remove it - it was meant to be used if a project wants to include the attributes. But I guess this is typically done one by one here. What do you think?

[eerhardt]

I kind of like the model of just including what you need. But others may have different opinions.

If no project is using the setting, I would drop it until someone uses it.

[eerhardt]

Is it weird to include Or '$(IncludeInstrumentationHelpers)'=='true' Or '$(IncludeDiagnosticSourceInstrumentationHelpers)'=='true' here? Why are they needed?

[vitek-karas]

Currently this is not needed, but once I annotate PropertyFetcher this will be necessary because both of these properties will include PropertyFetcher.

Alternatively I could include these files in the same group as PropertyFetcher.

[eerhardt]

This approach looks good to me. But it looks like the build is broken:

https://github.com/open-telemetry/opentelemetry-dotnet/actions/runs/5250186245/jobs/9483858384

Build FAILED.

D:\a\opentelemetry-dotnet\opentelemetry-dotnet\src\OpenTelemetry.Api\Logs\IDeferredLoggerProviderBuilder.cs(26,18): error RS0016: Symbol 'IDeferredLoggerProviderBuilder' is not part of the declared API [D:\a\opentelemetry-dotnet\opentelemetry-dotnet\src\OpenTelemetry.Api\OpenTelemetry.Api.csproj::TargetFramework=net6.0]
D:\a\opentelemetry-dotnet\opentelemetry-dotnet\src\OpenTelemetry.Api\Logs\Logger.cs(24,23): error RS0016: Symbol 'Logger' is not part of the declared API [D:\a\opentelemetry-dotnet\opentelemetry-dotnet\src\OpenTelemetry.Api\OpenTelemetry.Api.csproj::TargetFramework=net6.0]
D:\a\opentelemetry-dotnet\opentelemetry-dotnet\src\OpenTelemetry.Api\Logs\LoggerProvider.cs(28,14): error RS0016: Symbol 'LoggerProvider' is not part of the declared API [D:\a\opentelemetry-dotnet\opentelemetry-dotnet\src\OpenTelemetry.Api\OpenTelemetry.Api.csproj::TargetFramework=net6.0]
D:\a\opentelemetry-dotnet\opentelemetry-dotnet\src\OpenTelemetry.Api\Logs\LoggerProvider.cs(28,14): error RS0016: Symbol 'implicit constructor for 'LoggerProvider'' is not part of the declared API [D:\a\opentelemetry-dotnet\opentelemetry-dotnet\src\OpenTelemetry.Api\OpenTelemetry.Api.csproj::TargetFramework=net6.0]
D:\a\opentelemetry-dotnet\opentelemetry-dotnet\src\OpenTelemetry.Api\Logs\LoggerProviderBuilder.cs(24,23): error RS0016: Symbol 'LoggerProviderBuilder' is not part of the declared API [D:\a\opentelemetry-dotnet\opentelemetry-dotnet\src\OpenTelemetry.Api\OpenTelemetry.Api.csproj::TargetFramework=net6.0]
D:\a\opentelemetry-dotnet\opentelemetry-dotnet\src\OpenTelemetry.Api\Logs\LogRecordAttributeList.cs(30,15): error RS0016: Symbol 'LogRecordAttributeList' is not part of the declared API [D:\a\opentelemetry-dotnet\opentelemetry-dotnet\src\OpenTelemetry.Api\OpenTelemetry.Api.csproj::TargetFramework=net6.0]
D:\a\opentelemetry-dotnet\opentelemetry-dotnet\src\OpenTelemetry.Api\Logs\LogRecordAttributeList.cs(30,15): error RS0016: Symbol 'implicit constructor for 'LogRecordAttributeList'' is not part of the declared API [D:\a\opentelemetry-dotnet\opentelemetry-dotnet\src\OpenTelemetry.Api\OpenTelemetry.Api.csproj::TargetFramework=net6.0]
D:\a\opentelemetry-dotnet\opentelemetry-dotnet\src\OpenTelemetry.Api\Logs\LogRecordAttributeList.cs(299,19): error RS0016: Symbol 'Enumerator' is not part of the declared API [D:\a\opentelemetry-dotnet\opentelemetry-dotnet\src\OpenTelemetry.Api\OpenTelemetry.Api.csproj::TargetFramework=net6.0]
D:\a\opentelemetry-dotnet\opentelemetry-dotnet\src\OpenTelemetry.Api\Logs\LogRecordAttributeList.cs(299,19): error RS0016: Symbol 'implicit constructor for 'Enumerator'' is not part of the declared API [D:\a\opentelemetry-dotnet\opentelemetry-dotnet\src\OpenTelemetry.Api\OpenTelemetry.Api.csproj::TargetFramework=net6.0]

[Yun-Ting]

@vitek-karas @eerhardt @utpilla @CodeBlanch: Throwing out potential alternatives for discussion.
use #if preprocessors for lower .NET frameworks with AOT attributes as internal rather than public.

From the below prototype that fixed AOT warnings to the point where the app can successfully publish with AOT flag equals to true: main...vitek-karas:opentelemetry-dotnet:FixWarnings,
the statistics of the usage for the attributes are as follows:

[DynamicallyAccessedMembers] - 7 occurrences
[RequiresDynamicCode] - 0 occurrences
[RequiresUnreferencedCode] - 9 occurrences
[UnconditionalSuppress] - 12 occurrences

There are 28 places we'll need to add the preprocessors for the AOT attributes in order to bring the OTel packages to be AOT compatible. Performance-wise there should be no difference in using preprocessors or the approach in this PR. It's just different styling.

Pros I can think of with the preprocessor approach is that it respects the API/SDK separation of OpenTelemetry - to prevent cross-cutting concerns and it also helps to keep the publicAPI of the API project clean.
https://opentelemetry.io/docs/specs/otel/overview/#sdk

Another potential alternative is to use a mixture of both approaches - the preprocessor approach and directly define different include files in .csproj file. For the projects with a direct dependency on the SDK project, we leverage the .csproj file for conditional compilation between different .NET frameworks. And for the projects that references API projects directly, we use preprocessors.

[eerhardt]

Throwing out potential alternatives for discussion.
use #if preprocessors for lower .NET frameworks with AOT attributes as internal rather than public.

Can you prototype what this change would look like? Are you suggesting to just use #if NETCOREAPP around the application of the attributes in the source code?

If that is what you are proposing, you will still need to target net6.0 for OpenTelemetry.Api, assuming that assembly needs to apply these attributes to its code. It only targets netstandard2.0;net461 today, which means the #if NETCOREAPP will never be true and the attributes will never be applied in that assembly.

[vitek-karas]

As of right now, only the OpenTelemetry.Api.ProviderBuilderExtensions needs the attributes, OpenTelemetry.Api itself doesn't need them. So as @eerhardt points out we would have to target OpenTelemetry.Api.ProviderBuilderExtensions to net6.0. And any other assembly which needs the attributes (though I think all the other ones are already targeting net6.0)

[Yun-Ting]

@vitek-karas and @eerhardt -
Yes to we will need to add net6.0 as the target to the projects for the preprocessors.
Basically this https://github.com/open-telemetry/opentelemetry-dotnet/pull/4370/commits change (instead of net7.0, use net6.0) + #if NETCOREAPP.

[vitek-karas]

I modified this to reduce the impact of the change.

I kept the attribute definition files in OpenTelemetry.Api directory, since there's no "common" and it makes sense to me to have them in the lowest level assembly, as they're going to be used by other assemblies all over the place.
But OpenTelemetry.Api doesn't compile them, and so I removed retargeting it to net6.0 which felt like the most contentious issue.

Instead I retargeted OpenTelemetry.Api.ProviderBuilderExtensions as that one makes use of the new attributes. I added the attributes to this assembly as necessary to make it trim compatible (note that it doesn't reduce the number of warnings in the AOT test because the AOT test uses 7.0 AOT compiler which had some bugs in it).
For consideration - also add a pure trimming test (PublishTrimmed=true), to make sure that both AOT and trimming work correctly.

[eerhardt]

Would it make more sense to put these in https://github.com/open-telemetry/opentelemetry-dotnet/tree/main/src/Shared instead? I think it is a bit confusing to put them in this folder, but not actually used by this project.

[eerhardt]

1.5.0 is already released.

Suggested change

	v1.4.0 doesn't contain a net6.0 target, so we can't run ApiCompat against it.
	Instead, run net6.0's ApiCompat check against netstandard2.0.
	Remove this once 1.5.0 is released.
	v1.5.0 doesn't contain a net6.0 target, so we can't run ApiCompat against it.
	Instead, run net6.0's ApiCompat check against netstandard2.0.
	Remove this once 1.6.0 is released.

[eerhardt]

For consideration - also add a pure trimming test (PublishTrimmed=true), to make sure that both AOT and trimming work correctly.

Can you elaborate more on this? I know that AOT and trimming run different tools. But from a .NET library's perspective, shouldn't AOT warnings be a superset of trimming warnings? (assuming the underlying tools have no bugs).

[vitek-karas]

I know that AOT and trimming run different tools. But from a .NET library's perspective, shouldn't AOT warnings be a superset of trimming warnings? (assuming the underlying tools have no bugs).

It's basically about the fact that AOT compiler in .NET 7 doesn't have fully compliant analysis engine, so it misreports some warnings (both reports warnings where it should not, and doesn't report them where it should). Since upgrading the AOT test to .NET 8 is not going to happen anytime soon (I doubt OTel would want to require SDK previews in CI/CD), adding a variation of the test which just runs the trimmer is a good workaround.

[eerhardt]

Do we need to still target net6.0 for this project? That was necessary for OpenTelemetry.Api because of InternalsVisibleTo. Is the same necessary here?

[vitek-karas]

Yes - it has IVT to OpenTelemetry, so basically the same problem as OpenTelemetry.Api.

opentelemetry-dotnet/src/OpenTelemetry.Api.ProviderBuilderExtensions/AssemblyInfo.cs

Line 19 in 5f825cb

[assembly: InternalsVisibleTo("OpenTelemetry" + AssemblyInfo.PublicKey)]

[vitek-karas]

It's also in-line with the result of a discussion around retargeting to .NET 6. The conclusion was that retargeting projects which actually make use of the new attributes is a good solution.

[github-actions]

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.

[github-actions]

Closed as inactive. Feel free to reopen if this PR is still being worked on.