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

[api] Nullable annotations for the trace folder #4873

Merged
merged 15 commits into from
Sep 26, 2023
Merged

[api] Nullable annotations for the trace folder #4873

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
7a65494
Added nullable annotations for the trace folder in the api project.
CodeBlanch Sep 21, 2023
82e0d8c
Tweaks.
CodeBlanch Sep 21, 2023
504464b
Fixes.
CodeBlanch Sep 21, 2023
9741eaa
Fixes.
CodeBlanch Sep 21, 2023
3241534
Cleanup.
CodeBlanch Sep 21, 2023
c596f5b
Test fixes.
CodeBlanch Sep 22, 2023
2ca4f27
Tweaks.
CodeBlanch Sep 22, 2023
36d798c
Fix ApiCompat.
CodeBlanch Sep 22, 2023
6d8d5aa
Cleanup.
CodeBlanch Sep 22, 2023
70ff03f
CHANGELOG patch.
CodeBlanch Sep 22, 2023
b439b1d
Add tests for TracerProviderBuilderBase.
CodeBlanch Sep 22, 2023
df71aaf
Test improvements.
CodeBlanch Sep 22, 2023
4a64a56
Finished annotations for metrics & tracing.
CodeBlanch Sep 22, 2023
6299189
Merge from main.
CodeBlanch Sep 25, 2023
6a46e0f
CHANGELOG fixes.
CodeBlanch Sep 25, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion ...penTelemetry.Api.ProviderBuilderExtensions/Logs/LoggerProviderServiceCollectionBuilder.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,8 @@ public LoggerProviderServiceCollectionBuilder(IServiceCollection services)
public LoggerProvider? Provider => null;

/// <inheritdoc />
public override LoggerProviderBuilder AddInstrumentation<TInstrumentation>(Func<TInstrumentation> instrumentationFactory)
public override LoggerProviderBuilder AddInstrumentation<TInstrumentation>(Func<TInstrumentation?> instrumentationFactory)
where TInstrumentation : class
{
Guard.ThrowIfNull(instrumentationFactory);

Expand Down
3 changes: 2 additions & 1 deletion ...nTelemetry.Api.ProviderBuilderExtensions/Metrics/MeterProviderServiceCollectionBuilder.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

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

where TInstrumentation : class
{
Guard.ThrowIfNull(instrumentationFactory);

Expand Down
3 changes: 2 additions & 1 deletion ...enTelemetry.Api.ProviderBuilderExtensions/Trace/TracerProviderServiceCollectionBuilder.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,8 @@ public TracerProviderServiceCollectionBuilder(IServiceCollection services)
public TracerProvider? Provider => null;

/// <inheritdoc />
public override TracerProviderBuilder AddInstrumentation<TInstrumentation>(Func<TInstrumentation> instrumentationFactory)
public override TracerProviderBuilder AddInstrumentation<TInstrumentation>(Func<TInstrumentation?> instrumentationFactory)
where TInstrumentation : class
{
Guard.ThrowIfNull(instrumentationFactory);

Expand Down
2 changes: 1 addition & 1 deletion src/OpenTelemetry.Api/.publicApi/Experimental/net462/PublicAPI.Unshipped.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
abstract OpenTelemetry.Logs.Logger.EmitLog(in OpenTelemetry.Logs.LogRecordData data, in OpenTelemetry.Logs.LogRecordAttributeList attributes) -> void
abstract OpenTelemetry.Logs.LoggerProviderBuilder.AddInstrumentation<TInstrumentation>(System.Func<TInstrumentation!>! instrumentationFactory) -> OpenTelemetry.Logs.LoggerProviderBuilder!
abstract OpenTelemetry.Logs.LoggerProviderBuilder.AddInstrumentation<TInstrumentation>(System.Func<TInstrumentation?>! instrumentationFactory) -> OpenTelemetry.Logs.LoggerProviderBuilder!
OpenTelemetry.Logs.IDeferredLoggerProviderBuilder
OpenTelemetry.Logs.IDeferredLoggerProviderBuilder.Configure(System.Action<System.IServiceProvider!, OpenTelemetry.Logs.LoggerProviderBuilder!>! configure) -> OpenTelemetry.Logs.LoggerProviderBuilder!
OpenTelemetry.Logs.Logger
Expand Down
2 changes: 1 addition & 1 deletion src/OpenTelemetry.Api/.publicApi/Experimental/net6.0/PublicAPI.Unshipped.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
abstract OpenTelemetry.Logs.Logger.EmitLog(in OpenTelemetry.Logs.LogRecordData data, in OpenTelemetry.Logs.LogRecordAttributeList attributes) -> void
abstract OpenTelemetry.Logs.LoggerProviderBuilder.AddInstrumentation<TInstrumentation>(System.Func<TInstrumentation!>! instrumentationFactory) -> OpenTelemetry.Logs.LoggerProviderBuilder!
abstract OpenTelemetry.Logs.LoggerProviderBuilder.AddInstrumentation<TInstrumentation>(System.Func<TInstrumentation?>! instrumentationFactory) -> OpenTelemetry.Logs.LoggerProviderBuilder!
OpenTelemetry.Logs.IDeferredLoggerProviderBuilder
OpenTelemetry.Logs.IDeferredLoggerProviderBuilder.Configure(System.Action<System.IServiceProvider!, OpenTelemetry.Logs.LoggerProviderBuilder!>! configure) -> OpenTelemetry.Logs.LoggerProviderBuilder!
OpenTelemetry.Logs.Logger
Expand Down
2 changes: 1 addition & 1 deletion src/OpenTelemetry.Api/.publicApi/Experimental/netstandard2.0/PublicAPI.Unshipped.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
abstract OpenTelemetry.Logs.Logger.EmitLog(in OpenTelemetry.Logs.LogRecordData data, in OpenTelemetry.Logs.LogRecordAttributeList attributes) -> void
abstract OpenTelemetry.Logs.LoggerProviderBuilder.AddInstrumentation<TInstrumentation>(System.Func<TInstrumentation!>! instrumentationFactory) -> OpenTelemetry.Logs.LoggerProviderBuilder!
abstract OpenTelemetry.Logs.LoggerProviderBuilder.AddInstrumentation<TInstrumentation>(System.Func<TInstrumentation?>! instrumentationFactory) -> OpenTelemetry.Logs.LoggerProviderBuilder!
OpenTelemetry.Logs.IDeferredLoggerProviderBuilder
OpenTelemetry.Logs.IDeferredLoggerProviderBuilder.Configure(System.Action<System.IServiceProvider!, OpenTelemetry.Logs.LoggerProviderBuilder!>! configure) -> OpenTelemetry.Logs.LoggerProviderBuilder!
OpenTelemetry.Logs.Logger
Expand Down
106 changes: 53 additions & 53 deletions src/OpenTelemetry.Api/.publicApi/Stable/net462/PublicAPI.Shipped.txt
View file
Open in desktop

Large diffs are not rendered by default.

106 changes: 53 additions & 53 deletions src/OpenTelemetry.Api/.publicApi/Stable/net6.0/PublicAPI.Shipped.txt
View file
Open in desktop

Large diffs are not rendered by default.

106 changes: 53 additions & 53 deletions src/OpenTelemetry.Api/.publicApi/Stable/netstandard2.0/PublicAPI.Shipped.txt
View file
Open in desktop

Large diffs are not rendered by default.

2 changes: 1 addition & 1 deletion src/OpenTelemetry.Api/Logs/LoggerProviderBuilder.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,6 @@ protected LoggerProviderBuilder()
/// <param name="instrumentationFactory">Function that builds instrumentation.</param>
/// <returns>Returns <see cref="LoggerProviderBuilder"/> for chaining.</returns>
public abstract LoggerProviderBuilder AddInstrumentation<TInstrumentation>(
Func<TInstrumentation> instrumentationFactory)
Func<TInstrumentation?> instrumentationFactory)
where TInstrumentation : class;
}
4 changes: 3 additions & 1 deletion src/OpenTelemetry.Api/Metrics/MeterProviderBuilder.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,8 @@
// limitations under the License.
// </copyright>

#nullable enable

namespace OpenTelemetry.Metrics;

/// <summary>
Expand All @@ -35,7 +37,7 @@ protected MeterProviderBuilder()
/// <param name="instrumentationFactory">Function that builds instrumentation.</param>
/// <returns>Returns <see cref="MeterProviderBuilder"/> for chaining.</returns>
public abstract MeterProviderBuilder AddInstrumentation<TInstrumentation>(
Func<TInstrumentation> instrumentationFactory)
Func<TInstrumentation?> instrumentationFactory)
where TInstrumentation : class;

/// <summary>
Expand Down
22 changes: 12 additions & 10 deletions src/OpenTelemetry.Api/Trace/ActivityExtensions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,8 @@
// limitations under the License.
// </copyright>

#nullable enable

using System.Diagnostics;
using System.Runtime.CompilerServices;
using OpenTelemetry.Internal;
Expand Down Expand Up @@ -41,10 +43,11 @@ public static class ActivityExtensions
[MethodImpl(MethodImplOptions.AggressiveInlining)]
public static void SetStatus(this Activity activity, Status status)
{
Debug.Assert(activity != null, "Activity should not be null");

activity.SetTag(SpanAttributeConstants.StatusCodeKey, StatusHelper.GetTagValueForStatusCode(status.StatusCode));
activity.SetTag(SpanAttributeConstants.StatusDescriptionKey, status.Description);
if (activity != null)
{
activity.SetTag(SpanAttributeConstants.StatusCodeKey, StatusHelper.GetTagValueForStatusCode(status.StatusCode));
activity.SetTag(SpanAttributeConstants.StatusDescriptionKey, status.Description);
}
}

/// <summary>
Expand All @@ -57,7 +60,8 @@ public static void SetStatus(this Activity activity, Status status)
[MethodImpl(MethodImplOptions.AggressiveInlining)]
public static Status GetStatus(this Activity activity)
{
if (!activity.TryGetStatus(out StatusCode statusCode, out string statusDescription))
if (activity == null
|| !activity.TryGetStatus(out var statusCode, out var statusDescription))
{
return Status.Unset;
}
Expand All @@ -71,10 +75,8 @@ public static Status GetStatus(this Activity activity)
/// <param name="activity">Activity instance.</param>
/// <param name="ex">Exception to be recorded.</param>
[MethodImpl(MethodImplOptions.AggressiveInlining)]
public static void RecordException(this Activity activity, Exception ex)
{
activity?.RecordException(ex, default);
}
public static void RecordException(this Activity activity, Exception? ex)
=> RecordException(activity, ex, default);

/// <summary>
/// Adds an activity event containing information from the specified exception and additional tags.
Expand All @@ -83,7 +85,7 @@ public static void RecordException(this Activity activity, Exception ex)
/// <param name="ex">Exception to be recorded.</param>
/// <param name="tags">Additional tags to record on the event.</param>
[MethodImpl(MethodImplOptions.AggressiveInlining)]
public static void RecordException(this Activity activity, Exception ex, in TagList tags)
public static void RecordException(this Activity activity, Exception? ex, in TagList tags)
{
if (ex == null || activity == null)
{
Expand Down
2 changes: 2 additions & 0 deletions src/OpenTelemetry.Api/Trace/IDeferredTracerProviderBuilder.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,8 @@
// limitations under the License.
// </copyright>

#nullable enable

namespace OpenTelemetry.Trace;

/// <summary>
Expand Down
42 changes: 15 additions & 27 deletions src/OpenTelemetry.Api/Trace/Link.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,8 @@
// limitations under the License.
// </copyright>

#nullable enable

using System.Diagnostics;

namespace OpenTelemetry.Trace;
Expand All @@ -30,16 +32,16 @@ namespace OpenTelemetry.Trace;
/// </summary>
/// <param name="spanContext">Span context of a linked span.</param>
public Link(in SpanContext spanContext)
: this(in spanContext, attributes: null)
{
this.ActivityLink = new ActivityLink(spanContext.ActivityContext);
}

/// <summary>
/// Initializes a new instance of the <see cref="Link"/> struct.
/// </summary>
/// <param name="spanContext">Span context of a linked span.</param>
/// <param name="attributes">Link attributes.</param>
public Link(in SpanContext spanContext, SpanAttributes attributes)
public Link(in SpanContext spanContext, SpanAttributes? attributes)
{
this.ActivityLink = new ActivityLink(spanContext.ActivityContext, attributes?.Attributes);
}
Expand All @@ -48,53 +50,39 @@ public Link(in SpanContext spanContext, SpanAttributes attributes)
/// Gets the span context of a linked span.
/// </summary>
public SpanContext Context
{
get
{
return new SpanContext(this.ActivityLink.Context);
}
}
=> new(this.ActivityLink.Context);

/// <summary>
/// Gets the collection of attributes associated with the link.
/// </summary>
public IEnumerable<KeyValuePair<string, object>> Attributes
{
get
{
return this.ActivityLink.Tags;
}
}
public IEnumerable<KeyValuePair<string, object?>>? Attributes
=> this.ActivityLink.Tags;

/// <summary>
/// Compare two <see cref="Link"/> for equality.
/// </summary>
/// <param name="link1">First link to compare.</param>
/// <param name="link2">Second link to compare.</param>
public static bool operator ==(Link link1, Link link2) => link1.Equals(link2);
public static bool operator ==(Link link1, Link link2)
=> link1.Equals(link2);

/// <summary>
/// Compare two <see cref="Link"/> for not equality.
/// </summary>
/// <param name="link1">First link to compare.</param>
/// <param name="link2">Second link to compare.</param>
public static bool operator !=(Link link1, Link link2) => !link1.Equals(link2);
public static bool operator !=(Link link1, Link link2)
=> !link1.Equals(link2);

/// <inheritdoc />
public override bool Equals(object obj)
{
return (obj is Link link) && this.ActivityLink.Equals(link.ActivityLink);
}
public override bool Equals(object? obj)
=> obj is Link link && this.ActivityLink.Equals(link.ActivityLink);

/// <inheritdoc />
public override int GetHashCode()
{
return this.ActivityLink.GetHashCode();
}
=> this.ActivityLink.GetHashCode();

/// <inheritdoc/>
public bool Equals(Link other)
{
return this.ActivityLink.Equals(other.ActivityLink);
}
=> this.ActivityLink.Equals(other.ActivityLink);
}
18 changes: 10 additions & 8 deletions src/OpenTelemetry.Api/Trace/SpanAttributes.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,8 @@
// limitations under the License.
// </copyright>

#nullable enable

using System.Diagnostics;
using OpenTelemetry.Internal;

Expand All @@ -37,12 +39,12 @@ public SpanAttributes()
/// Initializes a new instance of the <see cref="SpanAttributes"/> class.
/// </summary>
/// <param name="attributes">Initial attributes to store in the collection.</param>
public SpanAttributes(IEnumerable<KeyValuePair<string, object>> attributes)
public SpanAttributes(IEnumerable<KeyValuePair<string, object?>> attributes)
: this()
{
Guard.ThrowIfNull(attributes);

foreach (KeyValuePair<string, object> kvp in attributes)
foreach (KeyValuePair<string, object?> kvp in attributes)
{
this.AddInternal(kvp.Key, kvp.Value);
}
Expand All @@ -65,7 +67,7 @@ public void Add(string key, long value)
/// </summary>
/// <param name="key">Entry key.</param>
/// <param name="value">Entry value.</param>
public void Add(string key, string value)
public void Add(string key, string? value)
{
this.AddInternal(key, value);
}
Expand Down Expand Up @@ -95,7 +97,7 @@ public void Add(string key, double value)
/// </summary>
/// <param name="key">Entry key.</param>
/// <param name="values">Entry value.</param>
public void Add(string key, long[] values)
public void Add(string key, long[]? values)
{
this.AddInternal(key, values);
}
Expand All @@ -105,7 +107,7 @@ public void Add(string key, long[] values)
/// </summary>
/// <param name="key">Entry key.</param>
/// <param name="values">Entry value.</param>
public void Add(string key, string[] values)
public void Add(string key, string?[]? values)
{
this.AddInternal(key, values);
}
Expand All @@ -115,7 +117,7 @@ public void Add(string key, string[] values)
/// </summary>
/// <param name="key">Entry key.</param>
/// <param name="values">Entry value.</param>
public void Add(string key, bool[] values)
public void Add(string key, bool[]? values)
{
this.AddInternal(key, values);
}
Expand All @@ -125,12 +127,12 @@ public void Add(string key, bool[] values)
/// </summary>
/// <param name="key">Entry key.</param>
/// <param name="values">Entry value.</param>
public void Add(string key, double[] values)
public void Add(string key, double[]? values)
{
this.AddInternal(key, values);
}

private void AddInternal(string key, object value)
private void AddInternal(string key, object? value)
{
Guard.ThrowIfNull(key);

Expand Down
Loading
Loading