Aligning TFMs under src folder.

[Yun-Ting]

Related to:
#4591 #4580

Changes

Introduced a variable "DefaultTargetFrameworks" in Common.prod.props with the value: net6.0;netstandard2.0;net462: All the projects under the src folder will ONLY target "DefaultTargetFrameworks" with the below exceptions:

1. Projects that already target netstandard2.1 will keep nestandard2.1 as one of their targets, i.e: OpenTelemetry.Exporter.Jaeger, OpenTelemtry and OpenTelemetry.Exporter.OpenTelemetry.Protocol.
2. netstandard2.0 and ne462 will NOT be added to project that is only targeting .NET core, i.e: OpenTelemetry.Exporter.Prometheus.AspNetCore.
3. Not updating TFM for this OpenTelemetry.Exporter.OpenTelemetryProtocol.Grpc as the project is subject to change. Use Grpc.Net.Client with netstandard2.0 for OpenTelemetry.Exporter.OpenTelemetryProtocol #3421 (comment)
4. OpenTelemetry.Instrumentation.AspNetCore will keep using "net7.0;net6.0;netstandard2.1;netstandard2.0" as its TFMs.

TODO in this PR: enable Microsoft.CodeAnalysis.PublicApiAnalyzers check back once people agree on project TFMs.
TODO with follow-up PRs: need to sort out test projects TFMs.

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)

[alanwest]

I won't say I'm opposed to considering the option that we align projects with the same set of TFMs, but I do not think this is a task we should do in a single PR.

For starters, if we add a TFM to a project then we need to make sure that our tests cover that new target. We already have problematic test coverage in that we do not adequately test our netstandard builds.

Additionally, I'm wary of adding netstandard2.1 to any more projects.

Taking a step back though, does AOT compatibility really make it necessary that we consider unifying all of our target frameworks at this point in time? I was previously under the impression that simply adding net6.0 to the API project would resolve things for us.

[eerhardt]

Additionally, I'm wary of adding netstandard2.1 to any more projects.

+1. Unless we have a strong reason, I think just net6.0;netstandard2.0;net462 are good enough.

Taking a step back though, does AOT compatibility really make it necessary that we consider unifying all of our target frameworks at this point in time?

It isn't strictly necessary to support AOT to unify all the libraries' TFMs. Adding net6.0 to the API project is all that is necessary to support AOT.

But there are advantages to being consistent across the repo. See #4591 (comment).

[utpilla]

I feel we should not add a new target to a project unless it's actually using an API that is only available in the newer target. Why should OpenTelemetry.Api add a net6.0 target when it does not even use a single API that was exclusively available on net6.0? This is also pointed out by the guidance provided for library authors here: https://learn.microsoft.com/en-us/dotnet/standard/library-guidance/cross-platform-targeting#multi-targeting

What are the specific benefits for this repo to having consistent targets?

AOT features are not required for lower targets such as net462 anyway. We could simply use preprocessor directives for any project that wants to use AOT-related attributes.

Also, if you don't want to target netstandard2.1 unless really necessary, then doesn't that mean you are okay with being inconsistent here? (as OpenTelemetry project does target netstandard2.1)

[vitek-karas]

Why should OpenTelemetry.Api add a net6.0 target when it does not even use a single API that was exclusively available on net6.0

This is basically caused by the frequent usage of InternalsVisibleTo. #4580 tries to add the trim/AOT attributes to the OpenTelemetry.Api project so that they can be used by other libraries in the product. This is normally done by defining the attribute as internal for TFMs where the attributes are not part of the framework. But this caused problems:

• If there's a use of for example RequiresUnreferencedCodeAttribute in the SDK assembly
• Since OpenTelemetry.Api only targets netstandard2.0 it will have an internal copy of the attribute
• Compiler when building SDK for net6.0 will see two definitions of the attribute, one from the framework and another one from OpenTelemetry.Api because there's InternalsVisibleTo between Api and SDK.

The only viable way to fix this we could come up with is to retarget Api to net6.0 so that when we build SDK for net6.0 it will get the net6.0 version of Api as its dependency which will NOT have the internal definition of the attribute.

AOT features are not required for lower targets such as net462 anyway. We could simply use preprocessor directives for any project that wants to use AOT-related attributes.

You're right that for net462 it doesn't matter. But the problems are even with netstandard2.0 which is targeted by basically everything. We did consider using ifdefs and only use the attributes when targeting net6.0. But there are still problems with that approach:

• We would still have to retarget some projects - for example OpenTelemetry.Api.ProviderBuilderExtensions currently targets netstandard2.0 but it will need some of the trim/AOT attributes. So it will need to be retargeted to net6.0 as well.
• Not having the trim/AOT attributes in netstandard2.0 builds may cause problems upstream. The trim/AOT analyzers rely on these attributes on public APIs (and as part of the AOT compatibility work, some of the OTel public APIs would get these attributes). If I had a netstandar2.0 library which uses OTel, the analyzers would not provide the right feedback since they would not see the attributes. This degrades the UX and makes it more difficult to develop against OTel.

For the above reasons I think the best approach is to:

• Add internal definitions of the attributes to projects which will need them, so that they can use them even when targeting netstandard2.0.
• Due to pervasive use of InternalsVisibleTo, retarget projects which internally define these attributes to net6.0 as well to avoid the above mentioned compilation issues.

[utpilla]

If I had a netstandar2.0 library which uses OTel, the analyzers would not provide the right feedback since they would not see the attributes. This degrades the UX and makes it more difficult to develop against OTel.

@vitek-karas @eerhardt

Before we decide on what do for OpenTelemetry, I would like to know what is .NET team's general recommendation for libraries targeting netstandard that want to be AOT-safe? Do you expect every such library out there to copy these attributes over to their codebase?

[vitek-karas]

@utpilla

We recommend including internal definitions of the attributes for TFMs which don't provide them in the framework. It's exactly what OOB packages from dotnet/runtime and some other repos already do. It provides the best experience for developers consuming the library.

That said if you don't want to do this for your library, adding the annotations (attributes) for .NET 6+ TFMs only is an acceptable approach. As mentioned above it may provide degraded experience for some consumers of the library but doesn't hinder usage from trimmed/AOT-ed applications (just makes it potentially harder).

It's also true that if the negative experiences become frequent/problematic, the attributes can always be added later without breaking anything.

[Yun-Ting]

@agocke, @eerhardt, @reyang, @utpilla @vitek-karas and I had an offline discussion of this issue, and we agreed on keeping TFMs across projects consistent. I've updated this PR accordingly.
Related discussion: #4591 (comment)
FYI - @tarekgh and @noahfalk

[codecov]

Codecov Report

Merging #4595 (a148f27) into main (56fdb16) will decrease coverage by 0.02%.
The diff coverage is n/a.

❗ Current head a148f27 differs from pull request most recent head 5f7b626. Consider uploading reports for the commit 5f7b626 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4595      +/-   ##
==========================================
- Coverage   84.95%   84.93%   -0.02%     
==========================================
  Files         314      314              
  Lines       12673    12673              
==========================================
- Hits        10766    10764       -2     
- Misses       1907     1909       +2     

Impacted Files	Coverage Δ
src/OpenTelemetry.Api/Baggage.cs	100.00% <ø> (ø)
src/OpenTelemetry.Api/Trace/Tracer.cs	97.22% <ø> (ø)
src/OpenTelemetry/Logs/LoggerProviderSdk.cs	92.78% <ø> (ø)

... and 4 files with indirect coverage changes

[reyang]

Minor - double check and make sure netstandard2.1 can be removed.

[Yun-Ting]

Updated PR description just now; will use follow-up PRs to decide whether to remove netstandard2.1 or not.

"
Introduced a variable "DefaultTargetFrameworks" in Common.prod.props with the value: net6.0;netstandard2.0;net462: All the projects under the src folder will ONLY target "DefaultTargetFrameworks" with the below exceptions:

1. Projects that already target netstandard2.1 will keep nestandard2.1 as one of their targets, i.e: OpenTelemetry.Exporter.Jaeger, OpenTelemtry and OpenTelemetry.Exporter.OpenTelemetry.Protocol.
2. netstandard2.0 and ne462 will NOT be added to project that is only targeting .NET core, i.e: OpenTelemetry.Exporter.Prometheus.AspNetCore.
3. Not updating TFM for this OpenTelemetry.Exporter.OpenTelemetryProtocol.Grpc as the project is subject to change. Use Grpc.Net.Client with netstandard2.0 for OpenTelemetry.Exporter.OpenTelemetryProtocol #3421 (comment)
4. OpenTelemetry.Instrumentation.AspNetCore will keep using "net7.0;net6.0;netstandard2.1;netstandard2.0" as its TFMs.
   "

[Yun-Ting]

@open-telemetry/dotnet-approvers and @open-telemetry/dotnet-maintainers:
Please help in reviewing the TFMs. Once I get a good idea of people's take of the current shape of the TFMs, I will enable the API check back. Thank you.

[utpilla]

This set of TFMs look good to me.

[utpilla]

It would be good to add a comment listing the only exceptions to this:

• OpenTelemetry.Instrumentation.AspNetCore
• OpenTelemetry.Exporter.Prometheus.AspNetCore

[reyang]

LGTM assuming the ApiAnalyzers will be updated.

[eerhardt]

This set of TFMs looks good to me.

Just need to add back the PublicApiAnalyzers reference.