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

Consolidate NullableAttributes across OpenTelemetry and OpenTelemetry.Api #4293

Closed
wants to merge 13 commits into from
Closed

Consolidate NullableAttributes across OpenTelemetry and OpenTelemetry.Api #4293

wants to merge 13 commits into from

Conversation

TimothyMothra
Copy link
Contributor

@TimothyMothra TimothyMothra commented Mar 10, 2023

Towards #4338

  1. NullableAttributes are copied from dotnet runtime (link) to support older frameworks.
    This repo currently has nullable attributes spread across both OpenTelemetry project (link) and OpenTelemetry.Api (link).
    This PR merges these into a single file.

  2. Another issue that needed to be addressed is target versions between OpenTelemetry and OpenTelemetry.Api projects.
    OpenTelemetry project targets net6.0;netstandard2.1;netstandard2.0;net462 and has a dependency on OpenTelemetry.Api which targets netstandard2.0;net462.
    Both projects need access to NullableAttributes and OpenTelemetry.Api makes its internals visible to OpenTelemetry.
    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.

  • add netstandard2.1 to OpenTelemetry.Api project.
  • move NullableAttributes from OpenTelemetry to OpenTelemetry.Api.
  • remove nullable attributes from OpenTelemetry.Api's Guard.cs and add to NullableAttributes.

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)

@TimothyMothra TimothyMothra requested a review from a team March 10, 2023 23:40
@codecov
Copy link

codecov bot commented Mar 10, 2023

Codecov Report

Merging #4293 (c6534a7) into main (6918866) will decrease coverage by 0.06%.
The diff coverage is n/a.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #4293      +/-   ##
==========================================
- Coverage   84.77%   84.72%   -0.06%     
==========================================
  Files         298      298              
  Lines       11984    11984              
==========================================
- Hits        10159    10153       -6     
- Misses       1825     1831       +6     
Impacted Files Coverage Δ
src/OpenTelemetry.Api/Internal/Guard.cs 100.00% <ø> (ø)
...c/OpenTelemetry.Api/Internal/NullableAttributes.cs 0.00% <ø> (ø)

... and 6 files with indirect coverage changes

@@ -0,0 +1,37 @@
// <copyright file="NullableAttributes.cs" company="OpenTelemetry Authors">
Copy link
Contributor

@Kielek Kielek Mar 13, 2023

Choose a reason for hiding this comment

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

Copy link
Contributor Author

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! :)

Copy link
Contributor Author

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)

Copy link
Contributor Author

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.

Copy link
Contributor Author

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.

@github-actions
Copy link
Contributor

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 github-actions bot added the Stale Issues and pull requests which have been flagged for closing due to inactivity label Mar 21, 2023
@github-actions github-actions bot removed the Stale Issues and pull requests which have been flagged for closing due to inactivity label Mar 23, 2023
@TimothyMothra TimothyMothra changed the title move nullable attributes to new file Consolidate NullableAttributes across OpenTelemetry and OpenTelemetry.Api Mar 27, 2023
@TimothyMothra TimothyMothra marked this pull request as draft March 27, 2023 23:35
@TimothyMothra TimothyMothra marked this pull request as ready for review March 27, 2023 23:56
@github-actions
Copy link
Contributor

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 github-actions bot added the Stale Issues and pull requests which have been flagged for closing due to inactivity label Apr 11, 2023
@TimothyMothra
Copy link
Contributor Author

I'm closing this PR in favor of making a new Shared project.
See: #4348

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Stale Issues and pull requests which have been flagged for closing due to inactivity
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants