Skip to content

Commit b13a5e3

Browse files
authored
Fix nullable annotations on Activity duck types (#8038)
## Summary of changes Fix incorrect nullable annotations on `Activity` duck types ## Reason for change While working on other performance things, noticed that the nullable annotations often declared non-nullability when they actually could be null. A particularly confusing part are the `TraceId` and `SpanId` values in `IW3CActivity`. These were marked non-nullable because when you call `Activity.Start()` then these will always be non-null, but _only_ if you're using W3C IDs. If you're using hierarchical IDs ([the default in <.NET 5](https://learn.microsoft.com/en-us/dotnet/core/compatibility/core-libraries/5.0/default-activityidformat-changed)) then these values _will_ be null. As a side note, I suspect this explains the "we saw errors about these being null in error tracking but don't understand why" scenarios 😄 Also, I think we should rename `IW3CActivity` to `IActivity3` instead. W3C _implies_ that it's a W3C activity, but that's not necessarily the case, and is essentially the source of the above confusion I think. `IActivity3` would then be consistently named with `IActivity5` and `IActivity6` we also currently have. ## Implementation details Add nullable annotations to values that _can_ be null, and fix the fallout (mostly in `ActivityKey`) ## Test coverage Covered by existing tests sufficiently I think ## Other details https://datadoghq.atlassian.net/browse/LANGPLAT-915 Part of a stack working to improve OTel performance - #8036 - #8037 👈 - #8038 - #8039 - #8040 - #8041 - #8042
1 parent c971199 commit b13a5e3

File tree

5 files changed

+20
-18
lines changed

5 files changed

+20
-18
lines changed

tracer/src/Datadog.Trace/Activity/DuckTypes/IActivity.cs

Lines changed: 9 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -13,10 +13,13 @@ namespace Datadog.Trace.Activity.DuckTypes
1313
{
1414
internal interface IActivity : IDuckType
1515
{
16-
// The corresponding property on the Activity object is nullable,
17-
// but we are guaranteed that the value will not be null once the Activity is started.
18-
// Since we are only accessing the Activity after it has started, we can treat the value
19-
// as never having a null value.
16+
/// <summary>
17+
/// Gets an ID for the activity. If the Activity is using hierarchical IDs, the Id has a hierarchical structure:
18+
/// '|root-id.id1_id2.id3_' and is generated when Start() is called by appending suffix to Parent.Id
19+
/// or ParentId; Activity has no Id until it started, but as we only see started IDs, this will always be non null.
20+
/// </summary>
21+
/// <remarks>NOTE: this property allocates when using W3C activities, and so should generally not be called
22+
/// unless you know that you're not using W3C activities (because <see cref="IW3CActivity.SpanId"/> is null</remarks>
2023
string Id { get; }
2124

2225
string? ParentId { get; }
@@ -35,9 +38,9 @@ internal interface IActivity : IDuckType
3538

3639
DateTime StartTimeUtc { get; }
3740

38-
IEnumerable<KeyValuePair<string, string>> Baggage { get; }
41+
IEnumerable<KeyValuePair<string, string?>> Baggage { get; }
3942

40-
IEnumerable<KeyValuePair<string, string>> Tags { get; }
43+
IEnumerable<KeyValuePair<string, string?>> Tags { get; }
4144

4245
object AddBaggage(string key, string value);
4346

tracer/src/Datadog.Trace/Activity/DuckTypes/IActivity5.cs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,7 @@ internal interface IActivity5 : IW3CActivity
1818

1919
ActivityKind Kind { get; }
2020

21-
IEnumerable<KeyValuePair<string, object>> TagObjects { get; }
21+
IEnumerable<KeyValuePair<string, object?>> TagObjects { get; }
2222

2323
ActivitySource Source { get; }
2424

tracer/src/Datadog.Trace/Activity/DuckTypes/IW3CActivity.cs

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -11,11 +11,14 @@ namespace Datadog.Trace.Activity.DuckTypes
1111
{
1212
internal interface IW3CActivity : IActivity
1313
{
14+
// Note that TraceId and SpanId will not be null when using W3C IDs, but they
15+
// _Can_ be nullable when using Hierarchical IDs. Also note that IW3CActivity
16+
// could be using _either_ type of ID
1417
[DuckField(Name = "_traceId")]
15-
string TraceId { get; set; }
18+
string? TraceId { get; set; }
1619

1720
[DuckField(Name = "_spanId")]
18-
string SpanId { get; set; }
21+
string? SpanId { get; set; }
1922

2023
[DuckField(Name = "_parentSpanId")]
2124
string ParentSpanId { get; set; }

tracer/src/Datadog.Trace/Activity/Handlers/AzureServiceBusActivityHandler.cs

Lines changed: 4 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -46,15 +46,11 @@ public void ActivityStopped<T>(string sourceName, T activity)
4646
// then we can retrieve the active message object using our mapping. With access to the message
4747
// object, we can accurately calculate the payload size for the DataStreamsCheckpoint
4848

49-
ActivityKey key;
50-
if (activity is IW3CActivity w3cActivity)
49+
ActivityKey key = activity switch
5150
{
52-
key = new(w3cActivity.TraceId, w3cActivity.SpanId);
53-
}
54-
else
55-
{
56-
key = new ActivityKey(activity.Id);
57-
}
51+
IW3CActivity { TraceId: not null, SpanId: not null } w3cActivity => new(w3cActivity.TraceId, w3cActivity.SpanId),
52+
_ => new(activity.Id)
53+
};
5854

5955
if (key.IsValid() && ActivityHandlerCommon.ActivityMappingById.TryRemove(key, out ActivityMapping activityMapping)
6056
&& activityMapping.Scope?.Span is Span span)

tracer/src/Datadog.Trace/Activity/Handlers/QuartzActivityHandler.cs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -47,7 +47,7 @@ public void ActivityStopped<T>(string sourceName, T activity)
4747
// Find the span and update it before the common handler processes it
4848
ActivityKey key = activity switch
4949
{
50-
IW3CActivity w3cActivity => new(w3cActivity.TraceId, w3cActivity.SpanId),
50+
IW3CActivity { TraceId: not null, SpanId: not null } w3cActivity => new(w3cActivity.TraceId, w3cActivity.SpanId),
5151
_ => new(activity.Id)
5252
};
5353

0 commit comments

Comments
 (0)