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.

Sign up for GitHub

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

Jump to bottom

[Api] Nullable #5801

Open
wants to merge 18 commits into
base: main
Choose a base branch
from
Open

[Api] Nullable #5801

wants to merge 18 commits into from

Conversation

ysolomchenko
Copy link
Contributor

Towards #3958

Changes

OpenTelemety.Api mark as nullable

Merge requirement checklist

  • CONTRIBUTING guidelines followed (license requirements, 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)

@ysolomchenko ysolomchenko requested a review from a team August 22, 2024 14:17
@github-actions github-actions bot added pkg:OpenTelemetry.Api Issues related to OpenTelemetry.Api NuGet package pkg:OpenTelemetry Issues related to OpenTelemetry NuGet package labels Aug 22, 2024
@codecov Codecov
Copy link

codecov bot commented Aug 22, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 66.66667% with 6 lines in your changes missing coverage. Please review.

Project coverage is 86.39%. Comparing base (6250307) to head (fe4d9bf).
Report is 316 commits behind head on main.

Files with missing lines Patch % Lines
...elemetry.Api/Context/RemotingRuntimeContextSlot.cs 0.00% 3 Missing ⚠️
...metry.Api/Context/ThreadLocalRuntimeContextSlot.cs 0.00% 2 Missing ⚠️
...emetry.Api/Context/AsyncLocalRuntimeContextSlot.cs 50.00% 1 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #5801      +/-   ##
==========================================
+ Coverage   83.38%   86.39%   +3.01%     
==========================================
  Files         297      257      -40     
  Lines       12531    11169    -1362     
==========================================
- Hits        10449     9650     -799     
+ Misses       2082     1519     -563
Flag Coverage Δ
unittests ?
unittests-Project-Experimental 86.26% <66.66%> (?)
unittests-Project-Stable 86.26% <66.66%> (?)
unittests-Solution 86.37% <66.66%> (?)
unittests-UnstableCoreLibraries-Experimental 85.71% <ø> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
src/OpenTelemetry.Api/ActivityContextExtensions.cs 100.00% <ø> (ø)
src/OpenTelemetry.Api/Baggage.cs 100.00% <100.00%> (ø)
src/OpenTelemetry.Api/BaseProvider.cs 100.00% <ø> (ø)
...nTelemetry.Api/Context/Propagation/B3Propagator.cs 86.02% <ø> (+0.15%) ⬆️
...metry.Api/Context/Propagation/BaggagePropagator.cs 85.00% <100.00%> (-0.49%) ⬇️
.../Context/Propagation/CompositeTextMapPropagator.cs 100.00% <ø> (+16.66%) ⬆️
...y.Api/Context/Propagation/NoopTextMapPropagator.cs 33.33% <ø> (+33.33%) ⬆️
...etry.Api/Context/Propagation/PropagationContext.cs 64.28% <ø> (ø)
...enTelemetry.Api/Context/Propagation/Propagators.cs 100.00% <ø> (ø)
....Api/Context/Propagation/TraceContextPropagator.cs 90.82% <ø> (+1.35%) ⬆️
... and 25 more

... and 227 files with indirect coverage changes

@Kielek Kielek force-pushed the api-mark-as-nullable branch 2 times, most recently from de9f690 to 8ebf150 Compare August 30, 2024 10:17
Kielek
Kielek approved these changes Aug 30, 2024
View reviewed changes
Copy link
Contributor

@Kielek Kielek left a comment

Choose a reason for hiding this comment

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

LGTM with fixes.
While reviewing, please focus on Baggage and RuntimeContext related things.

@github-actions 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 Sep 13, 2024
@Kielek Kielek removed the Stale Issues and pull requests which have been flagged for closing due to inactivity label Sep 13, 2024
TimothyMothra
TimothyMothra reviewed Sep 16, 2024
View reviewed changes
src/OpenTelemetry/Metrics/PullMetricScope.cs Outdated
@@ -7,7 +7,7 @@ namespace OpenTelemetry.Metrics;

internal sealed class PullMetricScope : IDisposable
{
private static readonly RuntimeContextSlot<bool> Slot = RuntimeContext.RegisterSlot<bool>("otel.pull_metric");
private static readonly RuntimeContextSlot<bool> Slot = RuntimeContext.RegisterSlot<bool>("otel.pull_metric")!;
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this one needs a closer look.
I'm not familiar with the RuntimeContext api. If the RegisterSlot method can return a nullable, that might suggest something failed.

Copy link
Contributor

Choose a reason for hiding this comment

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

Fixed in fe4d9bf

I double checked. There is no possibility to return null (at least as long someone does no play with reflection with contextSlotType field in RuntimeContext class.

TimothyMothra
TimothyMothra reviewed Sep 16, 2024
View reviewed changes
src/Shared/Guard.cs
@@ -19,12 +19,12 @@ namespace System.Runtime.CompilerServices
[AttributeUsage(AttributeTargets.Parameter, AllowMultiple = false, Inherited = false)]
internal sealed class CallerArgumentExpressionAttribute : Attribute
{
public CallerArgumentExpressionAttribute(string parameterName)
public CallerArgumentExpressionAttribute(string? parameterName)
Copy link
Contributor

Choose a reason for hiding this comment

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

Should doublecheck this. The original class doesn't have nullable. (link)

My understanding is that this should never be null.

Copy link
Contributor

Choose a reason for hiding this comment

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

See also https://github.com/dotnet/runtime/blob/v9.0.0-rc.1.24431.7/src/libraries/System.Runtime/tests/System.Runtime.Tests/System/Runtime/CompilerServices/CallerArgumentExpressionAttributeTests.cs (the same tests we have copied to this repository).

I am not against dropping support for null, but it is kind of break for what we have in dotnet repository.

Whatever we decide, the impact should be low. It is extremely internal class. Any recommendation based on this?

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks. I think this changes is good.

@github-actions github-actions bot removed the pkg:OpenTelemetry Issues related to OpenTelemetry NuGet package label Sep 17, 2024
@CodeBlanch CodeBlanch mentioned this pull request Sep 17, 2024
Merged
3 tasks
TimothyMothra
TimothyMothra approved these changes Sep 19, 2024
View reviewed changes
Copy link
Contributor

@TimothyMothra TimothyMothra left a comment

Choose a reason for hiding this comment

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

these changes LGTM.

One thing to note, I don't know the Baggage or RuntimeContext classes well enough to say if these values SHOULD be nullable.

@CodeBlanch CodeBlanch mentioned this pull request Sep 19, 2024
Open
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Kielek Kielek Kielek approved these changes

@TimothyMothra TimothyMothra TimothyMothra approved these changes

Assignees
No one assigned
Labels
pkg:OpenTelemetry.Api Issues related to OpenTelemetry.Api NuGet package
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@ysolomchenko @Kielek @TimothyMothra