[api] Nullable annotations for the trace folder #4873

CodeBlanch · 2023-09-21T20:47:44Z

Changes

Added missing nullable annotations for everything in the Trace folder inside the API project
Fixed a bug where TracerProviderBuilderBase was not invoking the instrumentationFactory delegate passed to the protected AddInstrumentation method.

Merge requirement checklist

CONTRIBUTING guidelines followed (nullable enabled, static analysis, etc.)
Unit tests added/updated
Appropriate CHANGELOG.md files updated for non-trivial changes

codecov · 2023-09-21T20:59:41Z

Codecov Report

Merging #4873 (6a46e0f) into main (9302891) will increase coverage by 0.00%.
The diff coverage is 98.33%.

@@           Coverage Diff           @@
##             main    #4873   +/-   ##
=======================================
  Coverage   82.90%   82.90%           
=======================================
  Files         294      294           
  Lines       12193    12188    -5     
=======================================
- Hits        10109    10105    -4     
+ Misses       2084     2083    -1

Flag	Coverage Δ
unittests	`82.90% <98.33%> (+<0.01%)`	⬆️

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

Files	Coverage Δ
src/OpenTelemetry.Api/ActivityContextExtensions.cs	`100.00% <ø> (ø)`
src/OpenTelemetry.Api/BaseProvider.cs	`100.00% <ø> (ø)`
...emetry.Api/Internal/OpenTelemetryApiEventSource.cs	`81.25% <ø> (+3.12%)`	⬆️
...rc/OpenTelemetry.Api/Logs/LoggerProviderBuilder.cs	`100.00% <ø> (ø)`
src/OpenTelemetry.Api/Metrics/MeterProvider.cs	`100.00% <ø> (ø)`
.../OpenTelemetry.Api/Metrics/MeterProviderBuilder.cs	`100.00% <ø> (ø)`
src/OpenTelemetry.Api/Trace/ActivityExtensions.cs	`95.45% <100.00%> (+0.21%)`	⬆️
src/OpenTelemetry.Api/Trace/Link.cs	`100.00% <100.00%> (ø)`
src/OpenTelemetry.Api/Trace/SpanAttributes.cs	`100.00% <100.00%> (ø)`
src/OpenTelemetry.Api/Trace/Status.cs	`100.00% <100.00%> (ø)`
... and 13 more

... and 2 files with indirect coverage changes

alanwest · 2023-09-22T18:28:10Z

...OpenTelemetry.Api.ProviderBuilderExtensions/Metrics/MeterProviderServiceCollectionBuilder.cs

@@ -33,7 +33,8 @@ public MeterProviderServiceCollectionBuilder(IServiceCollection services)
 public MeterProvider? Provider => null;

 /// <inheritdoc />
- public override MeterProviderBuilder AddInstrumentation<TInstrumentation>(Func<TInstrumentation> instrumentationFactory)
+ public override MeterProviderBuilder AddInstrumentation<TInstrumentation>(Func<TInstrumentation?> instrumentationFactory)


Is it meaningful for instrumentation factories to return null? This code suggests that we expect them to always return a non-null value

opentelemetry-dotnet/src/OpenTelemetry/Metrics/Builder/MeterProviderBuilderSdk.cs

Lines 125 to 138 in 535c819

public MeterProviderBuilder AddInstrumentation(

string instrumentationName,

string instrumentationVersion,

object instrumentation)

{

Debug.Assert(!string.IsNullOrWhiteSpace(instrumentationName), "instrumentationName was null or whitespace");

Debug.Assert(!string.IsNullOrWhiteSpace(instrumentationVersion), "instrumentationVersion was null or whitespace");

Debug.Assert(instrumentation != null, "instrumentation was null");

this.Instrumentation.Add(

new InstrumentationRegistration(

instrumentationName,

instrumentationVersion,

instrumentation!));

Good catch/question!

Short answer: Turns out that assert is based on invalid assumptions.

Long answer:

Today you can do this...

using OpenTelemetry; using OpenTelemetry.Trace; Console.WriteLine("Starting"); using (var provider = Sdk.CreateTracerProviderBuilder() .AddInstrumentation<object>(() => { Console.WriteLine("Instrumentation created"); return null; }) .Build()) { Console.WriteLine("Provider created"); } Console.WriteLine("Ending");

Everything will work fine. We don't actually validate that thing returned by the factory is non-null or access it in any way.

What will be logged out is:

Starting Instrumentation created Provider created Ending

SDK logs look like:

2023-09-22T21:55:35.5402710Z:TracerProviderSdk event: '{0}'{Building TracerProvider.} 2023-09-22T21:55:35.5645778Z:TracerProviderSdk event: '{0}'{Sampler added = "OpenTelemetry.Trace.ParentBasedSampler".} 2023-09-22T21:55:35.5650944Z:TracerProviderSdk event: '{0}'{Instrumentations added = "Object".} 2023-09-22T21:55:35.5652432Z:TracerProviderSdk event: '{0}'{TracerProvider built successfully.} 2023-09-22T21:55:35.5660583Z:'{0}' Disposed.{TracerProvider}

So null is possible today and allowed. There is that Debug.Assert in there, but that won't do anything for real code using release builds. That is only validating the repo's code doesn't return a null anywhere. Could actually lead us to make the wrong decisions!

It was probably an oversight to allow null in the first place, but in order to not break anything I annotated it as supported and made sure the code handles it well. If I go the other way and annotate it as not allowing null that could be breaking. We would get warnings in our code where we don't validate things (adding that validation would be breaking), and it may generate warnings for consumers and break their builds. Could also be binary breaking with annotations disapearing, not sure. In the end I decided just to embrace the behavior that we have in place.

Thoughts?

PS: Updated tests to make sure this case is captured.

CodeBlanch · 2023-09-22T22:21:39Z

src/OpenTelemetry/ApiCompatBaseline.txt

+Compat issues with assembly OpenTelemetry:
+CannotChangeAttribute : Attribute 'System.Runtime.CompilerServices.NullableAttribute' on generic param 'TInstrumentation' on member 'OpenTelemetry.Metrics.MeterProviderBuilderBase.AddInstrumentation<TInstrumentation>(System.Func<TInstrumentation>)' changed from '[NullableAttribute((byte)0)]' in the contract to '[NullableAttribute((byte)2)]' in the implementation.
+CannotChangeAttribute : Attribute 'System.Runtime.CompilerServices.NullableAttribute' on generic param 'TInstrumentation' on member 'OpenTelemetry.Trace.TracerProviderBuilderBase.AddInstrumentation<TInstrumentation>(System.Func<TInstrumentation>)' changed from '[NullableAttribute((byte)0)]' in the contract to '[NullableAttribute((byte)2)]' in the implementation.
+Total Issues: 2


This was very tricky. I had to get help from @ericstj.

Short version: ApiCompat detects that something has changed but it doesn't attempt to figure out if the change is safe.

Long version:

What is going on here is explained by Nullable Metadata.

Our API before was...

public abstract TracerProviderBuilder AddInstrumentation<TInstrumentation>( Func<TInstrumentation> instrumentationFactory) where TInstrumentation : class;

That is WITHOUT a nullable context so TInstrumentation in TracerProviderBuilderBase got [NullableAttribute((byte)0)] in its metadata.

Now we have annotations & a nullable context for this API as:

public abstract TracerProviderBuilder AddInstrumentation<TInstrumentation>( Func<TInstrumentation> instrumentationFactory) where TInstrumentation : class?;

Therefore TInstrumentation in TracerProviderBuilderBase gets changed to [NullableAttribute((byte)2)].

Both are null-allowing just one is "without annotations" and one is "with annotations."

My understanding is this is safe.

Thanks @ericstj!

CodeBlanch · 2023-09-22T22:28:25Z

src/OpenTelemetry/Trace/Builder/TracerProviderBuilderBase.cs

@@ -128,7 +128,7 @@ internal TracerProvider InvokeBuild()
 tracerProviderBuilderState.AddInstrumentation(
 instrumentationName,
 instrumentationVersion,
- instrumentationFactory);
+ instrumentationFactory());


This is really a bugfix. I'll add to CHANGELOG.

Also added tests.

CodeBlanch · 2023-09-22T22:57:50Z

src/OpenTelemetry.Api/Trace/Tracer.cs

 [MethodImpl(MethodImplOptions.AggressiveInlining)]
- public static TelemetrySpan WithSpan(TelemetrySpan span)
+ public static TelemetrySpan? WithSpan(TelemetrySpan? span)


@alanwest This was another weird one. Doesn't seem to really be valid to pass a null in or get one back out but for all of these annotations I just tried to capture what was in place today and not change any behaviors. Probably would have been better initially for this to throw a NRE for null input but it is what it is at this point 🤣

alanwest · 2023-09-25T20:50:03Z

src/OpenTelemetry/Trace/TracerProviderSdk.cs

- if (instrumentation.Instance != null)
+ if (instrumentation.Instance is not null)


The reason for this change was not immediately obvious to me. Just commenting in case this catches anyone else's eye:

Using != may case an overridden operator to be called. Using is not null will not.

reyang · 2023-09-25T22:38:28Z

src/OpenTelemetry/CHANGELOG.md

+* Fixed a bug where `TracerProviderBuilderBase` was not invoking the
+ `instrumentationFactory` delegate passed to the `protected`
+ `AddInstrumentation` method.
+ ([#4873](https:/open-telemetry/opentelemetry-dotnet/issues/4873))


Suggested change

([#4873](https:/open-telemetry/opentelemetry-dotnet/issues/4873))

([#4873](https:/open-telemetry/opentelemetry-dotnet/pull/4873))

It seems there are other entries in the CHANGELOG which need to be fixed.

Updated the one I added and all the ones at the ~top using /issues/ path instead of /pull/.

Added nullable annotations for the trace folder in the api project.

7a65494

CodeBlanch requested a review from a team September 21, 2023 20:47

CodeBlanch added 5 commits September 21, 2023 14:04

Tweaks.

82e0d8c

Fixes.

504464b

Fixes.

9741eaa

Cleanup.

3241534

Test fixes.

c596f5b

alanwest reviewed Sep 22, 2023

View reviewed changes

CodeBlanch added 2 commits September 22, 2023 14:11

Tweaks.

2ca4f27

Fix ApiCompat.

36d798c

CodeBlanch commented Sep 22, 2023

View reviewed changes

Cleanup.

6d8d5aa

CodeBlanch commented Sep 22, 2023

View reviewed changes

CodeBlanch added 3 commits September 22, 2023 15:30

CHANGELOG patch.

70ff03f

Add tests for TracerProviderBuilderBase.

b439b1d

Test improvements.

df71aaf

CodeBlanch commented Sep 22, 2023

View reviewed changes

Finished annotations for metrics & tracing.

4a64a56

alanwest reviewed Sep 25, 2023

View reviewed changes

alanwest approved these changes Sep 25, 2023

View reviewed changes

reyang approved these changes Sep 25, 2023

View reviewed changes

reyang reviewed Sep 25, 2023

View reviewed changes

CodeBlanch added 2 commits September 25, 2023 16:06

Merge from main.

6299189

CHANGELOG fixes.

6a46e0f

CodeBlanch merged commit 48b4a8c into open-telemetry:main Sep 26, 2023
50 checks passed

CodeBlanch deleted the api-trace-annotations branch September 26, 2023 00:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[api] Nullable annotations for the trace folder #4873

[api] Nullable annotations for the trace folder #4873

CodeBlanch commented Sep 21, 2023 •

edited

Loading

codecov bot commented Sep 21, 2023 •

edited

Loading

alanwest Sep 22, 2023

CodeBlanch Sep 22, 2023

CodeBlanch Sep 22, 2023

CodeBlanch Sep 22, 2023 •

edited

Loading

reyang Sep 25, 2023

CodeBlanch Sep 22, 2023

CodeBlanch Sep 22, 2023

CodeBlanch Sep 22, 2023 •

edited

Loading

alanwest Sep 25, 2023

reyang Sep 25, 2023

reyang Sep 25, 2023

CodeBlanch Sep 25, 2023

	public MeterProviderBuilder AddInstrumentation(
	string instrumentationName,
	string instrumentationVersion,
	object instrumentation)
	{
	Debug.Assert(!string.IsNullOrWhiteSpace(instrumentationName), "instrumentationName was null or whitespace");
	Debug.Assert(!string.IsNullOrWhiteSpace(instrumentationVersion), "instrumentationVersion was null or whitespace");
	Debug.Assert(instrumentation != null, "instrumentation was null");

	this.Instrumentation.Add(
	new InstrumentationRegistration(
	instrumentationName,
	instrumentationVersion,
	instrumentation!));

		if (instrumentation.Instance != null)
		if (instrumentation.Instance is not null)

	([#4873](https:/open-telemetry/opentelemetry-dotnet/issues/4873))
	([#4873](https:/open-telemetry/opentelemetry-dotnet/pull/4873))

[api] Nullable annotations for the trace folder #4873

[api] Nullable annotations for the trace folder #4873

Conversation

CodeBlanch commented Sep 21, 2023 • edited Loading

Changes

Merge requirement checklist

codecov bot commented Sep 21, 2023 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CodeBlanch Sep 22, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CodeBlanch Sep 22, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CodeBlanch commented Sep 21, 2023 •

edited

Loading

codecov bot commented Sep 21, 2023 •

edited

Loading

CodeBlanch Sep 22, 2023 •

edited

Loading

CodeBlanch Sep 22, 2023 •

edited

Loading