Skip to content

Commit ebcdc7c

Browse files
andrewlockandrewlock
committed
Introduce ActivityKey to avoid concat
1 parent efb97cb commit ebcdc7c

File tree

7 files changed

+159
-31
lines changed

7 files changed

+159
-31
lines changed

‎tracer/src/Datadog.Trace/Activity/Handlers/ActivityHandlerCommon.cs‎

Lines changed: 22 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,7 @@ namespace Datadog.Trace.Activity.Handlers
2121
internal sealed class ActivityHandlerCommon
2222
{
2323
private static readonly IDatadogLogger Log = DatadogLogging.GetLoggerFor(typeof(ActivityHandlerCommon));
24-
internal static readonly ConcurrentDictionary<string, ActivityMapping> ActivityMappingById = new();
24+
internal static readonly ConcurrentDictionary<ActivityKey, ActivityMapping> ActivityMappingById = new();
2525
private static readonly IntegrationId IntegrationId = IntegrationId.OpenTelemetry;
2626

2727
/// <summary>
@@ -48,10 +48,10 @@ public static void ActivityStarted<T>(string sourceName, T activity, OpenTelemet
4848
string? rawTraceId = null;
4949
string? rawSpanId = null;
5050

51-
// for non-IW3CActivity interfaces we'll use Activity.Id as the key as they don't have a guaranteed TraceId+SpanId
51+
// for non-IW3CActivity interfaces we'll use Activity.Id and string.Empty as the key as they don't have a guaranteed TraceId+SpanId
5252
// for IW3CActivity interfaces we'll use the Activity.TraceId + Activity.SpanId as the key
5353
// have to also validate that the TraceId and SpanId actually exist and aren't null - as they can be in some cases
54-
string? activityKey = null;
54+
ActivityKey? activityKey = null;
5555

5656
if (activity is IW3CActivity w3cActivity)
5757
{
@@ -67,7 +67,7 @@ public static void ActivityStarted<T>(string sourceName, T activity, OpenTelemet
6767
// Doing a lookup on just the TraceId+ParentSpanId seems to be more resilient.
6868
if (activityTraceId != null!)
6969
{
70-
if (ActivityMappingById.TryGetValue(activityTraceId + parentSpanId, out ActivityMapping mapping))
70+
if (ActivityMappingById.TryGetValue(new ActivityKey(activityTraceId, parentSpanId), out ActivityMapping mapping))
7171
{
7272
parent = mapping.Scope.Span.Context;
7373
}
@@ -88,7 +88,7 @@ public static void ActivityStarted<T>(string sourceName, T activity, OpenTelemet
8888
else
8989
{
9090
// we have a ParentSpanId/ParentId, but no TraceId/SpanId, so default to use the ParentId for lookup
91-
if (ActivityMappingById.TryGetValue(parentId, out ActivityMapping mapping))
91+
if (ActivityMappingById.TryGetValue(new ActivityKey(parentId), out ActivityMapping mapping))
9292
{
9393
parent = mapping.Scope.Span.Context;
9494
}
@@ -136,7 +136,7 @@ public static void ActivityStarted<T>(string sourceName, T activity, OpenTelemet
136136

137137
if (activityTraceId != null! && activitySpanId != null!)
138138
{
139-
activityKey = activityTraceId + activitySpanId;
139+
activityKey = new(traceId: activityTraceId, spanId: activitySpanId);
140140
}
141141
}
142142

@@ -164,9 +164,9 @@ public static void ActivityStarted<T>(string sourceName, T activity, OpenTelemet
164164
return;
165165
}
166166

167-
activityKey ??= activity.Id;
167+
activityKey ??= new(activity.Id);
168168

169-
if (activityKey is null)
169+
if (!activityKey.Value.IsValid())
170170
{
171171
// identified by Error Tracking
172172
// unsure how exactly this occurs after reading through the Activity source code
@@ -176,7 +176,7 @@ public static void ActivityStarted<T>(string sourceName, T activity, OpenTelemet
176176
return;
177177
}
178178

179-
activityMapping = ActivityMappingById.GetOrAdd(activityKey, _ => new(activity.Instance!, CreateScopeFromActivity(activity, tags, parent, traceId, spanId, rawTraceId, rawSpanId)));
179+
activityMapping = ActivityMappingById.GetOrAdd(activityKey.Value, _ => new(activity.Instance!, CreateScopeFromActivity(activity, tags, parent, traceId, spanId, rawTraceId, rawSpanId)));
180180
}
181181
catch (Exception ex)
182182
{
@@ -213,17 +213,20 @@ public static void ActivityStopped<T>(string sourceName, T activity)
213213
return;
214214
}
215215

216-
string key;
217-
if (activity is IW3CActivity w3cActivity)
216+
// Non-w3c activities will have null trace/span IDs, even though they implement IW3CActivity
217+
ActivityKey key;
218+
if (activity is IW3CActivity w3cActivity
219+
&& w3cActivity.TraceId != null!
220+
&& w3cActivity.SpanId != null!)
218221
{
219-
key = w3cActivity.TraceId + w3cActivity.SpanId;
222+
key = new(w3cActivity.TraceId, w3cActivity.SpanId);
220223
}
221224
else
222225
{
223-
key = activity.Id;
226+
key = new(activity.Id);
224227
}
225228

226-
if (key is null)
229+
if (!key.IsValid())
227230
{
228231
// Adding this as a protective measure as Error Tracking identified
229232
// instances where StartActivity had an Activity with null Id, SpanId, TraceId
@@ -259,9 +262,11 @@ public static void ActivityStopped<T>(string sourceName, T activity)
259262
Log.Information($"DefaultActivityHandler.ActivityStopped: [Missing Activity]");
260263
}
261264

262-
List<string>? toDelete = null;
263-
foreach (var (activityId, item) in ActivityMappingById)
265+
List<ActivityKey>? toDelete = null;
266+
foreach (var kvp in ActivityMappingById)
264267
{
268+
var activityId = kvp.Key;
269+
var item = kvp.Value;
265270
var activityObject = item.Activity;
266271
var scope = item.Scope;
267272
var hasClosed = false;
@@ -293,7 +298,7 @@ public static void ActivityStopped<T>(string sourceName, T activity)
293298

294299
if (hasClosed)
295300
{
296-
toDelete ??= new List<string>();
301+
toDelete ??= new List<ActivityKey>();
297302
toDelete.Add(activityId);
298303
}
299304
}

‎tracer/src/Datadog.Trace/Activity/Handlers/ActivityKey.cs‎

Lines changed: 50 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,50 @@
1+
// <copyright file="ActivityKey.cs" company="Datadog">
2+
// Unless explicitly stated otherwise all files in this repository are licensed under the Apache 2 License.
3+
// This product includes software developed at Datadog (https://www.datadoghq.com/). Copyright 2017 Datadog, Inc.
4+
// </copyright>
5+
6+
#nullable enable
7+
8+
using System;
9+
using System.Collections.Generic;
10+
11+
namespace Datadog.Trace.Activity.Handlers;
12+
13+
internal readonly struct ActivityKey : IEquatable<ActivityKey>
14+
{
15+
private readonly string _traceId;
16+
private readonly string _spanId;
17+
18+
public ActivityKey(string traceId, string spanId)
19+
{
20+
_traceId = traceId;
21+
_spanId = spanId;
22+
}
23+
24+
public ActivityKey(string id)
25+
{
26+
// Arbitrary which way around these are
27+
_spanId = id;
28+
_traceId = string.Empty;
29+
}
30+
31+
public bool IsValid() => _traceId is not null && _spanId is not null;
32+
33+
public bool Equals(ActivityKey other) =>
34+
string.Equals(_traceId, other._traceId, StringComparison.Ordinal) &&
35+
string.Equals(_spanId, other._spanId, StringComparison.Ordinal);
36+
37+
public override bool Equals(object? obj) =>
38+
obj is ActivityKey other && Equals(other);
39+
40+
public override int GetHashCode()
41+
{
42+
// TraceId and SpanId must be non-null, so can just just the values directly.
43+
// If we allow this to be called with any null values, then we must add null checks here
44+
unchecked
45+
{
46+
return (StringComparer.Ordinal.GetHashCode(_traceId) * 397)
47+
^ StringComparer.Ordinal.GetHashCode(_spanId);
48+
}
49+
}
50+
}

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

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -46,17 +46,17 @@ 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-
string key;
49+
ActivityKey key;
5050
if (activity is IW3CActivity w3cActivity)
5151
{
52-
key = w3cActivity.TraceId + w3cActivity.SpanId;
52+
key = new(w3cActivity.TraceId, w3cActivity.SpanId);
5353
}
5454
else
5555
{
56-
key = activity.Id;
56+
key = new ActivityKey(activity.Id);
5757
}
5858

59-
if (ActivityHandlerCommon.ActivityMappingById.TryRemove(key, out ActivityMapping activityMapping)
59+
if (key.IsValid() && ActivityHandlerCommon.ActivityMappingById.TryRemove(key, out ActivityMapping activityMapping)
6060
&& activityMapping.Scope?.Span is Span span)
6161
{
6262
// Copy over the data to our Span object so we can do an efficient tags lookup

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

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -45,13 +45,13 @@ public void ActivityStopped<T>(string sourceName, T activity)
4545
where T : IActivity
4646
{
4747
// Find the span and update it before the common handler processes it
48-
string key = activity switch
48+
ActivityKey key = activity switch
4949
{
50-
IW3CActivity w3cActivity => w3cActivity.TraceId + w3cActivity.SpanId,
51-
_ => activity.Id
50+
IW3CActivity w3cActivity => new(w3cActivity.TraceId, w3cActivity.SpanId),
51+
_ => new(activity.Id)
5252
};
5353

54-
if (key != null && ActivityHandlerCommon.ActivityMappingById.TryRemove(key, out var activityMapping) && activityMapping.Scope.Span is Span span)
54+
if (key.IsValid() && ActivityHandlerCommon.ActivityMappingById.TryRemove(key, out var activityMapping) && activityMapping.Scope.Span is Span span)
5555
{
5656
Log.Debug("ActivityStopped: Processing span for activity '{ActivityId}'", activity.Id);
5757

‎tracer/src/Datadog.Trace/ClrProfiler/AutoInstrumentation/OpenTelemetry/ResourceAttributeProcessorHelper.cs‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -31,17 +31,17 @@ public static void OnStart(object processor, object activityData)
3131
return;
3232
}
3333

34-
string key;
34+
ActivityKey key;
3535
if (activityData.TryDuckCast<IW3CActivity>(out var w3cActivity) && w3cActivity.TraceId is { } traceId && w3cActivity.SpanId is { } spanId)
3636
{
37-
key = traceId + spanId;
37+
key = new(traceId, spanId);
3838
}
3939
else
4040
{
41-
key = activity.Id;
41+
key = new(activity.Id);
4242
}
4343

44-
if (ActivityHandlerCommon.ActivityMappingById.TryGetValue(key, out var activityMapping))
44+
if (key.IsValid() && ActivityHandlerCommon.ActivityMappingById.TryGetValue(key, out var activityMapping))
4545
{
4646
if (baseProcessor.ParentProvider is not null)
4747
{

‎tracer/test/Datadog.Trace.Tests/Activity/ActivityKeyTests.cs‎

Lines changed: 73 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,73 @@
1+
// <copyright file="ActivityKeyTests.cs" company="Datadog">
2+
// Unless explicitly stated otherwise all files in this repository are licensed under the Apache 2 License.
3+
// This product includes software developed at Datadog (https://www.datadoghq.com/). Copyright 2017 Datadog, Inc.
4+
// </copyright>
5+
6+
using System.Collections.Generic;
7+
using Datadog.Trace.Activity.Handlers;
8+
using FluentAssertions;
9+
using Xunit;
10+
11+
namespace Datadog.Trace.Tests.Activity;
12+
13+
public class ActivityKeyTests
14+
{
15+
[Theory]
16+
[InlineData("spanId", "traceId")]
17+
[InlineData("id", "")]
18+
public void Equality(string spanId, string traceId)
19+
{
20+
var activity1 = new ActivityKey(traceId: traceId, spanId: spanId);
21+
var activity2 = new ActivityKey(traceId: traceId, spanId: spanId);
22+
activity1.Equals(activity2).Should().BeTrue();
23+
activity1.GetHashCode().Should().Be(activity2.GetHashCode());
24+
25+
var other = new ActivityKey(traceId: traceId, spanId: "newId");
26+
activity1.Equals(other).Should().BeFalse();
27+
activity1.GetHashCode().Should().NotBe(other.GetHashCode());
28+
activity2.Equals(other).Should().BeFalse();
29+
activity2.GetHashCode().Should().NotBe(other.GetHashCode());
30+
}
31+
32+
[Theory]
33+
[InlineData("spanId", "traceId")]
34+
[InlineData("id", "")]
35+
public void Equality_DefaultComparer(string spanId, string traceId)
36+
{
37+
var activity1 = new ActivityKey(traceId: traceId, spanId: spanId);
38+
var activity2 = new ActivityKey(traceId: traceId, spanId: spanId);
39+
40+
EqualityComparer<ActivityKey>.Default.Equals(activity1, activity2).Should().BeTrue();
41+
var hashCode1 = EqualityComparer<ActivityKey>.Default.GetHashCode(activity1).GetHashCode();
42+
var hashCode2 = EqualityComparer<ActivityKey>.Default.GetHashCode(activity2);
43+
hashCode1.Should().Be(hashCode2);
44+
45+
var other = new ActivityKey(traceId: traceId, spanId: "newId");
46+
var hashCodeOther = EqualityComparer<ActivityKey>.Default.GetHashCode(other).GetHashCode();
47+
EqualityComparer<ActivityKey>.Default.Equals(activity1, other).Should().BeFalse();
48+
hashCode1.Should().NotBe(hashCodeOther);
49+
EqualityComparer<ActivityKey>.Default.Equals(activity2, other).Should().BeFalse();
50+
hashCode2.Should().NotBe(hashCodeOther);
51+
}
52+
53+
[Theory]
54+
[InlineData("spanId", "traceId", true)]
55+
[InlineData("spanId", "", true)]
56+
[InlineData("", "traceId", true)]
57+
[InlineData("", "", true)]
58+
[InlineData("spanId", null, false)]
59+
[InlineData(null, "traceId", false)]
60+
public void IsValid_SpanId_TraceId(string traceId, string spanId, bool isValid)
61+
{
62+
new ActivityKey(traceId: traceId, spanId: spanId).IsValid().Should().Be(isValid);
63+
}
64+
65+
[Theory]
66+
[InlineData("some_id", true)]
67+
[InlineData("", true)]
68+
[InlineData(null, false)]
69+
public void IsValid_SpanId_Id(string id, bool isValid)
70+
{
71+
new ActivityKey(id).IsValid().Should().Be(isValid);
72+
}
73+
}

‎tracer/test/Datadog.Trace.Tests/ActivityTests.cs‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -207,12 +207,12 @@ public class ActivityFixture : IDisposable
207207

208208
public ActivityFixture()
209209
{
210-
Activity.ActivityListener.Initialize();
210+
Datadog.Trace.Activity.ActivityListener.Initialize();
211211
}
212212

213213
public void Dispose()
214214
{
215-
Activity.ActivityListener.StopListeners();
215+
Datadog.Trace.Activity.ActivityListener.StopListeners();
216216
}
217217

218218
public void StartActivity(SD.Activity activity)

0 commit comments

Comments
 (0)