-
Notifications
You must be signed in to change notification settings - Fork 857
Consolidate NullableAttributes across OpenTelemetry and OpenTelemetry.Api #4293
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
Consolidate NullableAttributes across OpenTelemetry and OpenTelemetry.Api #4293
Conversation
Codecov Report
Additional details and impacted files
|
| @@ -0,0 +1,37 @@ | |||
| // <copyright file="NullableAttributes.cs" company="OpenTelemetry Authors"> | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that content from this file should be merged with existing one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These should absolutely be merged!
I didn't know this file was here! This is probably what was causing conflicts in my nullable branch. Thanks for pointing this out! :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've run into an issue merging these two files and I'm still exploring options before I make a recommendation.
Both OpenTelemetry and OpenTelemetry.Api projects need access to nullables.
OTel has a build dependency on OTel.Api.
OTel.Api makes internals visible to OTel.
The blocker is that OTel targets net6.0;netstandard2.1;netstandard2.0;net462 where as OTel.Api targets netstandard2.0;net462.
OTel (net6 and netstardard2.1) will take a dependency on OTel.Api (netstandard2.0).
And then OTel (net6 and netstardard2.1) has a conflict with runtime.
Error CS0433 The type 'AllowNullAttribute' exists in both 'OpenTelemetry.Api, Version=1.0.0.0, Culture=neutral, PublicKeyToken=7bd6737fe5b67e3c' and 'System.Runtime, Version=6.0.0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a' OpenTelemetry (net6.0)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The fix is to add netstandard2.1 to the OTel.Api project. I'll bring this up in the SIG meeting tomorrow.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Discussed this in today's SIG and there are no blockers with this approach. :)
It would be preferred if we can move shared files to a separate project, but this only works if we can also remove the internals visible to between OTel and OTel.Api. I'm going to take a day to explore if this would work.
|
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. |
|
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. |
|
I'm closing this PR in favor of making a new Shared project. |
Towards #4338
NullableAttributesare copied from dotnet runtime (link) to support older frameworks.This repo currently has nullable attributes spread across both
OpenTelemetryproject (link) andOpenTelemetry.Api(link).This PR merges these into a single file.
Another issue that needed to be addressed is target versions between
OpenTelemetryandOpenTelemetry.Apiprojects.OpenTelemetryproject targetsnet6.0;netstandard2.1;netstandard2.0;net462and has a dependency onOpenTelemetry.Apiwhich targetsnetstandard2.0;net462.Both projects need access to
NullableAttributesandOpenTelemetry.Apimakes its internals visible toOpenTelemetry.Consider that the highest version of OpenTelemetry.Api is netstandard2.0, which means the NullableAttributes are always present.
The OpenTelemetry project will have access to these NullableAttributes in the Api project and will conflict with the dotnet runtime.
To address this, OpenTelemetry.Api needs to target netstandard2.1 so it produces a dll without those internals. See also this comment: SDK: Nullable annotations for base classes & batch + shims to enable compiler features #3374 (comment)
Changes
Please provide a brief description of the changes here.
netstandard2.1toOpenTelemetry.Apiproject.NullableAttributesfrom OpenTelemetry to OpenTelemetry.Api.OpenTelemetry.Api's Guard.cs and add to NullableAttributes.Merge requirement checklist
CHANGELOG.mdfiles updated for non-trivial changes