From fbe182f87c3a5d507e8fa87997d20b09b87d61ca Mon Sep 17 00:00:00 2001 From: Zach Montoya Date: Fri, 22 Nov 2024 13:02:59 -0800 Subject: [PATCH 1/4] SpanLink API Update: Remove DecoratedSpan property so that we only operate on SpanContext --- tracer/src/Datadog.Trace/Span.cs | 2 +- tracer/src/Datadog.Trace/SpanLink.cs | 16 +++------------- .../test/Datadog.Trace.Tests/SpanLinksTests.cs | 9 ++++++--- 3 files changed, 10 insertions(+), 17 deletions(-) diff --git a/tracer/src/Datadog.Trace/Span.cs b/tracer/src/Datadog.Trace/Span.cs index fc4c5d2d8a8c..b6191dfd72e4 100644 --- a/tracer/src/Datadog.Trace/Span.cs +++ b/tracer/src/Datadog.Trace/Span.cs @@ -550,7 +550,7 @@ internal SpanLink AddSpanLink(Span spanLinkToAdd, List(); - var spanLink = new SpanLink(spanLinkToAdd, this, attributes); + var spanLink = new SpanLink(spanLinkToAdd, attributes); SpanLinks.Add(spanLink); return spanLink; } diff --git a/tracer/src/Datadog.Trace/SpanLink.cs b/tracer/src/Datadog.Trace/SpanLink.cs index 627536e90693..fc6bc00b6e9f 100644 --- a/tracer/src/Datadog.Trace/SpanLink.cs +++ b/tracer/src/Datadog.Trace/SpanLink.cs @@ -25,17 +25,15 @@ internal class SpanLink /// in OpenTelemetry that's called a Span Context, which may also include tracestate and trace flags. /// /// The context of the spanlink to extract attributes from - /// Reference to the span you're adding SpanLinks to /// Optional dictionary of attributes to take for the spanlink. - public SpanLink(SpanContext spanLinkContext, Span decoratedSpan, List>? attributes = null) + public SpanLink(SpanContext spanLinkContext, List>? attributes = null) { Context = spanLinkContext; - DecoratedSpan = decoratedSpan; Attributes = attributes; } - public SpanLink(Span spanToLink, Span decoratedSpan, List>? attributes = null) - : this(spanToLink.Context, decoratedSpan, attributes) + public SpanLink(Span spanToLink, List>? attributes = null) + : this(spanToLink.Context, attributes) { } @@ -43,8 +41,6 @@ public SpanLink(Span spanToLink, Span decoratedSpan, List /// Adds an Attribute to the SpanLink. /// @@ -52,12 +48,6 @@ public SpanLink(Span spanToLink, Span decoratedSpan, Listvalue of attribute public void AddAttribute(string name, string value) { - if (DecoratedSpan.IsFinished) - { - Log.Warning("AddAttribute should not be called after the decorated span was closed"); - return; - } - var newAttribute = new KeyValuePair(name, value); Attributes ??= []; diff --git a/tracer/test/Datadog.Trace.Tests/SpanLinksTests.cs b/tracer/test/Datadog.Trace.Tests/SpanLinksTests.cs index ce2904f34d41..830c02f2fe2b 100644 --- a/tracer/test/Datadog.Trace.Tests/SpanLinksTests.cs +++ b/tracer/test/Datadog.Trace.Tests/SpanLinksTests.cs @@ -3,10 +3,12 @@ // This product includes software developed at Datadog (https://www.datadoghq.com/). Copyright 2017 Datadog, Inc. // +using System.Collections.Generic; using Castle.Core.Internal; using Datadog.Trace.Agent; using Datadog.Trace.Configuration; using Datadog.Trace.Sampling; +using FluentAssertions; using Moq; using Xunit; @@ -39,7 +41,7 @@ public void AddLink_InCloseSpan() } [Fact] - public void AddAttribute_ToLink_InCloseSpan() + public void AddAttribute_Succeeds_AfterSpanFinished() { var parentScope = (Scope)_tracer.StartActive("Parent"); var childScope = (Scope)_tracer.StartActive("Child"); @@ -48,8 +50,9 @@ public void AddAttribute_ToLink_InCloseSpan() var childSpan = childScope.Span; var spanLink = childSpan.AddSpanLink(parentSpan); childSpan.Finish(); - spanLink.AddAttribute("should", "return null"); - Assert.Null(spanLink.Attributes); + spanLink.AddAttribute("key", "value"); + + spanLink.Attributes.Should().BeEquivalentTo(new[] { new KeyValuePair("key", "value") }); } } } From 6c3d2931c4f4d6b1c882340b70cc7ab56893a5f5 Mon Sep 17 00:00:00 2001 From: Zach Montoya Date: Tue, 19 Nov 2024 17:02:10 -0800 Subject: [PATCH 2/4] Update Span.AddSpanLink API to accept a SpanLink instead of a Span. This updated interface is eaiser to use from instrumentations when there are no Spans for incoming headers, only span contexts. --- tracer/src/Datadog.Trace/Activity/OtlpHelpers.cs | 4 ++-- tracer/src/Datadog.Trace/Span.cs | 10 +++------- .../MessagePack/SpanMessagePackFormatterTests.cs | 9 ++++++--- tracer/test/Datadog.Trace.Tests/SpanLinksTests.cs | 15 ++++++++++----- 4 files changed, 21 insertions(+), 17 deletions(-) diff --git a/tracer/src/Datadog.Trace/Activity/OtlpHelpers.cs b/tracer/src/Datadog.Trace/Activity/OtlpHelpers.cs index 443135368ded..f95801dc5405 100644 --- a/tracer/src/Datadog.Trace/Activity/OtlpHelpers.cs +++ b/tracer/src/Datadog.Trace/Activity/OtlpHelpers.cs @@ -254,8 +254,8 @@ private static void ExtractActivityLinks(Span span, IActivity5? activity spanContext.LastParentId = traceState.LastParent; spanContext.PropagatedTags = traceTags; - var extractedSpan = new Span(spanContext, DateTimeOffset.Now, new CommonTags()); - var spanLink = span.AddSpanLink(extractedSpan); + var spanLink = new SpanLink(spanContext); + span.AddSpanLink(spanLink); if (duckLink.Tags is not null) { diff --git a/tracer/src/Datadog.Trace/Span.cs b/tracer/src/Datadog.Trace/Span.cs index b6191dfd72e4..e4b0a12fe00a 100644 --- a/tracer/src/Datadog.Trace/Span.cs +++ b/tracer/src/Datadog.Trace/Span.cs @@ -538,21 +538,17 @@ private void WriteCloseDebugMessage() /// /// Adds a SpanLink to the current Span if the Span is active. /// - /// The Span to add as a SpanLink - /// List of KeyValue pairings of attributes to add to the SpanLink. Defaults to null - /// returns the SpanLink on success or null on failure (span is closed already) - internal SpanLink AddSpanLink(Span spanLinkToAdd, List> attributes = null) + /// The SpanLink to add + internal void AddSpanLink(SpanLink spanLink) { if (IsFinished) { Log.Warning("AddSpanLink should not be called after the span was closed"); - return null; + return; } SpanLinks ??= new List(); - var spanLink = new SpanLink(spanLinkToAdd, attributes); SpanLinks.Add(spanLink); - return spanLink; } } } diff --git a/tracer/test/Datadog.Trace.Tests/Agent/MessagePack/SpanMessagePackFormatterTests.cs b/tracer/test/Datadog.Trace.Tests/Agent/MessagePack/SpanMessagePackFormatterTests.cs index 76928a95c333..4660675dbcbe 100644 --- a/tracer/test/Datadog.Trace.Tests/Agent/MessagePack/SpanMessagePackFormatterTests.cs +++ b/tracer/test/Datadog.Trace.Tests/Agent/MessagePack/SpanMessagePackFormatterTests.cs @@ -141,11 +141,14 @@ public void SpanLink_Tag_Serialization() new("pair", "false"), new("arbitrary", "56709") }; - spans[0].AddSpanLink(spans[1], attributesToAdd); - var tmpSpanLink = spans[1].AddSpanLink(spans[2]); + spans[0].AddSpanLink(new SpanLink(spans[1].Context, attributesToAdd)); + + var tmpSpanLink = new SpanLink(spans[2].Context); + spans[1].AddSpanLink(tmpSpanLink); tmpSpanLink.AddAttribute("attribute1", "value1"); tmpSpanLink.AddAttribute("attribute2", "value2"); - spans[1].AddSpanLink(spans[0]); + + spans[1].AddSpanLink(new SpanLink(spans[0].Context)); foreach (var span in spans) { diff --git a/tracer/test/Datadog.Trace.Tests/SpanLinksTests.cs b/tracer/test/Datadog.Trace.Tests/SpanLinksTests.cs index 830c02f2fe2b..513b5352777b 100644 --- a/tracer/test/Datadog.Trace.Tests/SpanLinksTests.cs +++ b/tracer/test/Datadog.Trace.Tests/SpanLinksTests.cs @@ -28,31 +28,36 @@ public SpanLinksTests() } [Fact] - public void AddLink_InCloseSpan() + public void AddLink_AfterSpanFinished_IsNoOp() { var parentScope = (Scope)_tracer.StartActive("Parent"); var childScope = (Scope)_tracer.StartActive("Child"); var parentSpan = parentScope.Span; var childSpan = childScope.Span; + var spanLink = new SpanLink(parentSpan.Context); childSpan.Finish(); - Assert.Null(childSpan.AddSpanLink(parentSpan)); + + childSpan.AddSpanLink(spanLink); + childSpan.SpanLinks.Should().BeNullOrEmpty(); } [Fact] - public void AddAttribute_Succeeds_AfterSpanFinished() + public void AddAttribute_BeforeSpanFinished_IsSuccessful() { var parentScope = (Scope)_tracer.StartActive("Parent"); var childScope = (Scope)_tracer.StartActive("Child"); var parentSpan = parentScope.Span; var childSpan = childScope.Span; - var spanLink = childSpan.AddSpanLink(parentSpan); - childSpan.Finish(); + var spanLink = new SpanLink(parentSpan.Context); + + childSpan.AddSpanLink(spanLink); spanLink.AddAttribute("key", "value"); spanLink.Attributes.Should().BeEquivalentTo(new[] { new KeyValuePair("key", "value") }); + childSpan.Finish(); } } } From 2bfad3df0131b3b092bc5b2c8f40908e2ec212ee Mon Sep 17 00:00:00 2001 From: Zach Montoya Date: Fri, 22 Nov 2024 13:33:53 -0800 Subject: [PATCH 3/4] Updates to make the SpanLink API more similar to the Activity API: - Rename the method from AddSpanLink to AddLink (this also mirrors the suggestion from OpenTelemetry Tracing API spec) - Return the Span object instead of returning nothing --- tracer/src/Datadog.Trace/Activity/OtlpHelpers.cs | 2 +- tracer/src/Datadog.Trace/Span.cs | 7 ++++--- .../Agent/MessagePack/SpanMessagePackFormatterTests.cs | 6 +++--- tracer/test/Datadog.Trace.Tests/SpanLinksTests.cs | 4 ++-- 4 files changed, 10 insertions(+), 9 deletions(-) diff --git a/tracer/src/Datadog.Trace/Activity/OtlpHelpers.cs b/tracer/src/Datadog.Trace/Activity/OtlpHelpers.cs index f95801dc5405..27958ce5f711 100644 --- a/tracer/src/Datadog.Trace/Activity/OtlpHelpers.cs +++ b/tracer/src/Datadog.Trace/Activity/OtlpHelpers.cs @@ -255,7 +255,7 @@ private static void ExtractActivityLinks(Span span, IActivity5? activity spanContext.PropagatedTags = traceTags; var spanLink = new SpanLink(spanContext); - span.AddSpanLink(spanLink); + span.AddLink(spanLink); if (duckLink.Tags is not null) { diff --git a/tracer/src/Datadog.Trace/Span.cs b/tracer/src/Datadog.Trace/Span.cs index e4b0a12fe00a..2c17e3c88886 100644 --- a/tracer/src/Datadog.Trace/Span.cs +++ b/tracer/src/Datadog.Trace/Span.cs @@ -539,16 +539,17 @@ private void WriteCloseDebugMessage() /// Adds a SpanLink to the current Span if the Span is active. /// /// The SpanLink to add - internal void AddSpanLink(SpanLink spanLink) + internal Span AddLink(SpanLink spanLink) { if (IsFinished) { - Log.Warning("AddSpanLink should not be called after the span was closed"); - return; + Log.Warning("AddLink should not be called after the span was closed"); + return this; } SpanLinks ??= new List(); SpanLinks.Add(spanLink); + return this; } } } diff --git a/tracer/test/Datadog.Trace.Tests/Agent/MessagePack/SpanMessagePackFormatterTests.cs b/tracer/test/Datadog.Trace.Tests/Agent/MessagePack/SpanMessagePackFormatterTests.cs index 4660675dbcbe..2ed0d18d7335 100644 --- a/tracer/test/Datadog.Trace.Tests/Agent/MessagePack/SpanMessagePackFormatterTests.cs +++ b/tracer/test/Datadog.Trace.Tests/Agent/MessagePack/SpanMessagePackFormatterTests.cs @@ -141,14 +141,14 @@ public void SpanLink_Tag_Serialization() new("pair", "false"), new("arbitrary", "56709") }; - spans[0].AddSpanLink(new SpanLink(spans[1].Context, attributesToAdd)); + spans[0].AddLink(new SpanLink(spans[1].Context, attributesToAdd)); var tmpSpanLink = new SpanLink(spans[2].Context); - spans[1].AddSpanLink(tmpSpanLink); + spans[1].AddLink(tmpSpanLink); tmpSpanLink.AddAttribute("attribute1", "value1"); tmpSpanLink.AddAttribute("attribute2", "value2"); - spans[1].AddSpanLink(new SpanLink(spans[0].Context)); + spans[1].AddLink(new SpanLink(spans[0].Context)); foreach (var span in spans) { diff --git a/tracer/test/Datadog.Trace.Tests/SpanLinksTests.cs b/tracer/test/Datadog.Trace.Tests/SpanLinksTests.cs index 513b5352777b..55aeb08bbe0f 100644 --- a/tracer/test/Datadog.Trace.Tests/SpanLinksTests.cs +++ b/tracer/test/Datadog.Trace.Tests/SpanLinksTests.cs @@ -39,7 +39,7 @@ public void AddLink_AfterSpanFinished_IsNoOp() childSpan.Finish(); - childSpan.AddSpanLink(spanLink); + childSpan.AddLink(spanLink); childSpan.SpanLinks.Should().BeNullOrEmpty(); } @@ -53,7 +53,7 @@ public void AddAttribute_BeforeSpanFinished_IsSuccessful() var childSpan = childScope.Span; var spanLink = new SpanLink(parentSpan.Context); - childSpan.AddSpanLink(spanLink); + childSpan.AddLink(spanLink); spanLink.AddAttribute("key", "value"); spanLink.Attributes.Should().BeEquivalentTo(new[] { new KeyValuePair("key", "value") }); From 4cfb37f686dff7638d8a1bce63c77b4d6ae6e051 Mon Sep 17 00:00:00 2001 From: Zach Montoya Date: Fri, 22 Nov 2024 14:09:34 -0800 Subject: [PATCH 4/4] Add small doc comment for new Span return --- tracer/src/Datadog.Trace/Span.cs | 1 + 1 file changed, 1 insertion(+) diff --git a/tracer/src/Datadog.Trace/Span.cs b/tracer/src/Datadog.Trace/Span.cs index 2c17e3c88886..89cec0e664c7 100644 --- a/tracer/src/Datadog.Trace/Span.cs +++ b/tracer/src/Datadog.Trace/Span.cs @@ -539,6 +539,7 @@ private void WriteCloseDebugMessage() /// Adds a SpanLink to the current Span if the Span is active. /// /// The SpanLink to add + /// This span to allow method chaining. internal Span AddLink(SpanLink spanLink) { if (IsFinished)