Span links improvements and bug fix #3123

nhulston · 2025-01-27T19:42:29Z

What does this PR do?

Implements an AddSpanLinks method
Previously, span links are only serialized under Span[span_links]. This PR serializes span links in the span meta, under Span[meta][_dd.span_links].
Moves span links struct to its own file since regenerating span_links.msgp.go previously results in errors.
Removes the omitempty tag from the all fields in the SpanLink struct.

Motivation

These improvements and bug fix are prerequisites for a new feature I'm working on called 'span pointers', and they will be generally useful to other people in the future. Span pointers will be implemented in a future PR, but I did a similar PR in Java.

Implements the AddSpanLinks method. Although it's only used once right now, I will be using this in my future 'span pointers' PR.
Span links should also be serialized in Meta[_dd.span_links]. See this article. This aligns span links logic with other tracers, and some logic in the agent/frontend expect links to be under the meta.
Regenerating span_links.msgp.go right now results in errors (due to recent additions to the ddtrace.go file), so I moved the span links struct to a separate file to resolve this.
(Bug fix) Removes the omitempty from all fields in the SpanLink struct. Even though these fields are optional, they should still have values of 0 or empty string when serialized by msgp. This is because libdatadog requires these fields to exist when deserializing the span links. The serverless agent uses libdatadog, and this is causing an error and causing any span with span links to be dropped by the serverless agent. This is how span links are serialized in all other tracers, so it's best to just modify dd-trace-go to align with them.

Testing

Added unit tests.

Manual testing shows that the links are serialized correctly in the meta.

Manual testing also showed that spans with span links are not dropped anymore by Bottlecap, the serverless agent. The S3 request span is now visible, but was dropped previously.

Reviewer's Checklist

Changed code has unit tests for its functionality at or near 100% coverage.
System-Tests covering this feature have been added and enabled with the va.b.c-dev version tag.
There is a benchmark for any new code, or changes to existing code.
If this interacts with the agent in a new way, a system test has been added.
Add an appropriate team label so this PR gets put in the right place for the release notes.
Non-trivial go.mod changes, e.g. adding new modules, are reviewed by @DataDog/dd-trace-go-guild.
For internal contributors, a matching PR should be created to the v2-dev branch and reviewed by @DataDog/apm-go.

Unsure? Have a question? Request a review!

datadog-datadog-prod-us1 · 2025-01-27T19:49:02Z

Datadog Report

Branch report: nicholas.hulston/span-link-improvements
Commit report: d582292
Test service: dd-trace-go

✅ 0 Failed, 5225 Passed, 73 Skipped, 2m 9.58s Total Time

nhulston · 2025-01-27T19:51:53Z

ddtrace/tracer/span.go

+	if len(s.SpanLinks) == 0 {
+		return
+	}
+	spanLinkBytes, err := json.Marshal(s.SpanLinks)


A little concerned about performance, but not sure if there's any other way to do this. We have to serialize the links into a JSON string such as "[{\"trace_id\":0,\"trace_id_high\":0,\"span_id\":0,\"attributes\":{\"link.kind\":\"span-pointer\",\"ptr.dir\":\"d\",\"ptr.hash\":\"eb29cb7d923f904f02bd8b3d85e228ed\",\"ptr.kind\":\"aws.s3.object\"},\"tracestate\":\"\",\"flags\":0}]"

You could always add a benchmark for that

It's actually must faster than I expected

cpu: Apple M3 Max BenchmarkSerializeSpanLinksInMeta BenchmarkSerializeSpanLinksInMeta-16 1253884 964.4 ns/op

…tion works properly

pr-commenter · 2025-01-27T20:42:24Z

Benchmarks

Benchmark execution time: 2025-02-03 15:39:15

Comparing candidate commit 43e6edc in PR branch nicholas.hulston/span-link-improvements with baseline commit 0a41ffd in branch main.

Found 0 performance improvements and 2 performance regressions! Performance is the same for 57 metrics, 0 unstable metrics.

scenario:BenchmarkSetTagMetric-24

🟥 execution_time [+3.333ns; +6.047ns] or [+2.746%; +4.982%]

scenario:BenchmarkSetTagString-24

🟥 execution_time [+5.352ns; +7.408ns] or [+4.681%; +6.480%]

ddtrace/tracer/span.go

mtoffl01 · 2025-01-28T16:35:29Z

ddtrace/ddtrace.go

@@ -79,6 +79,9 @@ type Span interface {
 	// item should propagate to all descendant spans, both in- and cross-process.
 	SetBaggageItem(key, val string)

+	// AddSpanLinks appends the given links to the span's span links.
+	AddSpanLinks(spanLinks ...SpanLink)


A few notes:

We already have support for adding span links to a span when you are starting it; do you need this new API because you need to add span links to a span that has already been started? If so, what is the motivation for that? If not, can you use the existing API?

Adding a new method to the existing Span interface is a breaking change. The alternative approach is to introduce a new interface with the new method (with the old interface embedded). You can see what we do for adding spanlinks to the span context here: https://github.com/DataDog/dd-trace-go/pull/2973/files

Yes, I do need to add span links to a span that's already started. In my case, I will need to get some values from an S3/Dynamo response before adding span links, which occurs right before the span is finished.

Out of curiosity, why is this a breaking change? Done in 66d3a67, is that what you were looking for?

Public interfaces are considered "contracts" in Golang. Modifying them is "breaking contract" because that means every type which expects to satisfy this interface must now be updated -- this can impact customer code.

… to the existing span interface (breaking change)

mtoffl01

Looking good, just please add tests (no matter how trivial) to indicate how this API should be used!

ddtrace/mocktracer/mockspan.go

ddtrace/tracer/span.go

mtoffl01 · 2025-02-03T20:55:01Z

ddtrace/tracer/span.go

@@ -514,6 +543,8 @@ func (s *span) Finish(opts ...ddtrace.FinishOption) {
 		}
 	}

+	s.serializeSpanLinksInMeta()


Can you state explicitly why you were not able to rely on json.Marshal and we have to call this custom serializer? You may be correct, I just want to make sure I'm following

The serializeSpanLinksInMeta func does use json.Marshal; I just extracted the method for easier refactoring in the future. We could remove the serializeSpanLinksInMeta() function and put the logic directly in Finish(), wdyt?

mtoffl01 · 2025-02-03T20:55:54Z

ddtrace/tracer/span.go

+	if len(s.SpanLinks) == 0 {
+		return
+	}
+	spanLinkBytes, err := json.Marshal(s.SpanLinks)


You could always add a benchmark for that

… reference

mtoffl01

LGTM. Just make sure to:

update the PR title to contain the package you've changed, ddtrace/tracer. See https://github.com/DataDog/dd-trace-go/blob/main/CONTRIBUTING.md#contributing
update the PR description to reflect your new method name, AddSpanLink (instead of AddSpanLinks 😉 )

nhulston force-pushed the nicholas.hulston/span-link-improvements branch from f7a9058 to a455896 Compare January 27, 2025 19:49

nhulston commented Jan 27, 2025

View reviewed changes

nhulston force-pushed the nicholas.hulston/span-link-improvements branch from a455896 to b081005 Compare January 27, 2025 19:54

nhulston changed the title ~~Implement AddSpanLinks method; serialize span links in meta; fix bug with missing fields in span links~~ Span links improvements and bug fixes Jan 27, 2025

nhulston changed the title ~~Span links improvements and bug fixes~~ Span links improvements and bug fix Jan 27, 2025

Move SpanLink struct to separate span_link.go file so msgp genera…

297ef1f

…tion works properly

nhulston force-pushed the nicholas.hulston/span-link-improvements branch from b081005 to 72d9dbe Compare January 27, 2025 20:07

nhulston added 4 commits January 27, 2025 15:08

remove omitempty from SpanLink struct fields

a3922f8

Create and implement AddSpanLinks method

fd93eb7

use AddSpanLinks method in StartSpan

fbbfdda

serialize span links in meta before finishing span

e1c23c8

nhulston force-pushed the nicholas.hulston/span-link-improvements branch from 72d9dbe to e1c23c8 Compare January 27, 2025 20:08

nhulston marked this pull request as ready for review January 27, 2025 20:40

nhulston requested a review from a team as a code owner January 27, 2025 20:40

mtoffl01 reviewed Jan 28, 2025

View reviewed changes

nhulston added 2 commits January 28, 2025 14:42

Create new interface SpanWithLinks, rather than adding a new method…

66d3a67

… to the existing span interface (breaking change)

log marshal error

10d5ddb

nhulston requested a review from mtoffl01 January 28, 2025 19:45

mtoffl01 reviewed Jan 29, 2025

View reviewed changes

ddtrace/mocktracer/mockspan.go Outdated Show resolved Hide resolved

nhulston added 3 commits January 30, 2025 11:36

remove mockspan impl of AddSpanPointers

e32eee0

add unit tests

2f83d57

Merge branch 'main' into nicholas.hulston/span-link-improvements

a5734e1

This comment was marked as resolved.

Sign in to view

nhulston requested a review from mtoffl01 January 30, 2025 17:24

Merge branch 'main' into nicholas.hulston/span-link-improvements

43e6edc

mtoffl01 reviewed Feb 3, 2025

View reviewed changes

mtoffl01 self-assigned this Feb 3, 2025

AddSpanLink singular instead of slice, because slices are shared by…

b0bdffc

… reference

Benchmark SerializeSpanLinksInMeta

c835acd

nhulston requested a review from a team as a code owner February 5, 2025 15:34

nhulston requested a review from mtoffl01 February 5, 2025 15:37

mtoffl01 approved these changes Feb 5, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Span links improvements and bug fix #3123

Span links improvements and bug fix #3123

nhulston commented Jan 27, 2025 •

edited

Loading

datadog-datadog-prod-us1 bot commented Jan 27, 2025 •

edited

Loading

nhulston Jan 27, 2025

mtoffl01 Feb 3, 2025

nhulston Feb 5, 2025

pr-commenter bot commented Jan 27, 2025 •

edited

Loading

mtoffl01 Jan 28, 2025

nhulston Jan 28, 2025

mtoffl01 Jan 29, 2025 •

edited

Loading

mtoffl01 left a comment

This comment was marked as resolved.

mtoffl01 Feb 3, 2025

nhulston Feb 5, 2025

mtoffl01 Feb 3, 2025

mtoffl01 left a comment •

edited

Loading

Span links improvements and bug fix #3123

Are you sure you want to change the base?

Span links improvements and bug fix #3123

Conversation

nhulston commented Jan 27, 2025 • edited Loading

What does this PR do?

Motivation

Testing

Reviewer's Checklist

datadog-datadog-prod-us1 bot commented Jan 27, 2025 • edited Loading

Datadog Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pr-commenter bot commented Jan 27, 2025 • edited Loading

Benchmarks

scenario:BenchmarkSetTagMetric-24

scenario:BenchmarkSetTagString-24

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mtoffl01 Jan 29, 2025 • edited Loading

Choose a reason for hiding this comment

mtoffl01 left a comment

Choose a reason for hiding this comment

This comment was marked as resolved.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mtoffl01 left a comment • edited Loading

Choose a reason for hiding this comment

nhulston commented Jan 27, 2025 •

edited

Loading

datadog-datadog-prod-us1 bot commented Jan 27, 2025 •

edited

Loading

pr-commenter bot commented Jan 27, 2025 •

edited

Loading

mtoffl01 Jan 29, 2025 •

edited

Loading

mtoffl01 left a comment •

edited

Loading