Skip to content

Avoid creation of empty CopyOnWriteArrayList for span links #10822

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

Merged
gh-worker-dd-mergequeue-cf854d merged 13 commits into from
Mar 30, 2026
Merged

Avoid creation of empty CopyOnWriteArrayList for span links #10822

Changes from 1 commit
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
40fc417
Avoid creation of empty CopyOnWriteArrayList for span links
dougqh Mar 12, 2026
34fddd2
Merge branch 'master' into dougqh/span-links-allocation
dougqh Mar 24, 2026
43a60d5
Introducing WritableSpanLinks & SpanLinkAccessor
dougqh Mar 24, 2026
4b7e5ef
Merge branch 'dougqh/span-links-allocation' of github.com:DataDog/dd-…
dougqh Mar 24, 2026
9410a6f
spotless
dougqh Mar 25, 2026
f453110
Addressing review comments
dougqh Mar 25, 2026
8aa187e
Remove vestigial processTags(Map, DDSpanContext, List) method from Ta…
dougqh Mar 26, 2026
9dd37e5
spotless
dougqh Mar 26, 2026
17a0044
codeNarcTest - fixing typo
dougqh Mar 26, 2026
9c0745a
Merge branch 'master' into dougqh/span-links-allocation
dougqh Mar 26, 2026
ca95b42
Restricting API access
dougqh Mar 27, 2026
8580496
spotless
dougqh Mar 27, 2026
d145c52
Merge branch 'master' into dougqh/span-links-allocation
dougqh Mar 30, 2026
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
40 changes: 36 additions & 4 deletions dd-trace-core/src/main/java/datadog/trace/core/DDSpan.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -116,7 +116,8 @@ static DDSpan create(
*/
private volatile int longRunningVersion = 0;

protected final List<AgentSpanLink> links;
private static final List<AgentSpanLink> EMPTY = Collections.emptyList();
Copy link
Copy Markdown
Contributor

@amarziali amarziali Mar 12, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

There is an issue with SpanPointersProcessor. In fact TagsPostProcessors manipulate links directly and adding something to an emptyList will cause an UnsupportedOperationException. What can be done, is to use an empty non threadsafe list when calling tag processors if the original is empty and eventually batch adding the list at the end if the processors added something

Copy link
Copy Markdown
Contributor

@PerfectSlayer PerfectSlayer Mar 12, 2026

Choose a reason for hiding this comment

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

Few ideas here:

  1. There is a TODO from Doug about the processor that mutates the span links:

dd-trace-java/dd-trace-core/src/main/java/datadog/trace/core/tagprocessor/SpanPointersProcessor.java

Line 42 in d9df78f

// DQH - TODO - There's a lot room to optimize this using TagMap's capabilities

Does it mean it can skip span links and use tags directly?

  1. As processors only add span links, we can replace the span links collection by a method reference to the addSpanLink method as Consumer<SpanLink>. This will avoid exposing the span link implementation and we can keep it null until there is effectively span link stored.

Copy link
Copy Markdown
Contributor Author

@dougqh dougqh Mar 12, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

That's a good catch.
I do think we need to improve the encapsulation; otherwise, these changes are going to be hard to make.

As for my prior TODO, TagMap has a bunch of methods for removeAndGetEntry, getString, stringValue, etc that could be used in the various *spanPointer methods. In theory, they could simplify the code significantly.

Copy link
Copy Markdown
Contributor Author

@dougqh dougqh Mar 12, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

On a related note, I'm curious why spanLinks was put on DDSpan rather than DDSpanContext.
It seems like it would be more consistent to put on DDSpanContext and then we wouldn't have this issue.

I also think we might be pool DDSpanContext in the future, so I'd like to avoid adding more to DDSpan.

Copy link
Copy Markdown
Contributor

@PerfectSlayer PerfectSlayer Mar 13, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

I'm curious why spanLinks was put on DDSpan rather than DDSpanContext.

SpanContext was supposed to describe "a span context", not to hold the span data model.
We should be able to use SpanContext as reference descriptor for spans, but they end up holding way too much data.
So when I add to implement span links, I made sure to leave them in Span, not SpanContext.

I had a look at its Javadoc and it basically should only be IDs (trace + span), sampling, a bit of (W3C) trace state and baggage, nothing else :)

Copy link
Copy Markdown
Contributor Author

@dougqh dougqh Mar 24, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

Okay, that makes sense. I think that maybe we should repurpose SpanContext into SpanContents.

I have a bit of crazy idea to recycle SpanContents, but I want to do so while maintaining Span object identity. In other words, I'd always create a new Span, but the SpanContents inside the span would be reused.

Copy link
Copy Markdown
Contributor Author

@dougqh dougqh Mar 25, 2026

Choose a reason for hiding this comment

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

I've addressed the TagPostProcessor interaction by introducing two new interfaces: WritableSpanLinks and SpanLinkAccessor.

WritableSpanLinks just provides the ability to addLink, it is passed to TagPostProcessor. SpanLinkAccessor extends WritableSpanLinks and also provides a way to get the links.

DDSpan now implements SpanLinkAccessor. I picked this design, so I could avoid a capturing lambda around the span.

protected volatile List<AgentSpanLink> links;

/**
* Spans should be constructed using the builder, not by calling the constructor directly.
Expand Down Expand Up @@ -144,7 +145,7 @@ private DDSpan(
context.getTraceCollector().touch(); // external clock: explicitly update lastReferenced
}

this.links = links == null ? new CopyOnWriteArrayList<>() : new CopyOnWriteArrayList<>(links);
this.links = links == null || links.isEmpty() ? EMPTY : new CopyOnWriteArrayList<>(links);
}

public boolean isFinished() {
Expand Down Expand Up @@ -879,8 +880,39 @@ public TraceConfig traceConfig() {

@Override
public void addLink(AgentSpanLink link) {
if (link != null) {
this.links.add(link);
if (link == null) {
return;
}

// If links are initially null / empty, then the shared placeholder List EMPTY is used.
// Bacause EMPTY is shared, EMPTY is safe for reading, but not for writing.
// On write - if links is the EMPTY placeholder, then need to create a CopyOnWriteArrayList
// owned by this DDSpan

// Creation of the CopyOnWriteArrayList is done via double checking locking using volatile &
// synchronized

// If before or inside the synchronized block, links no longer points to EMPTY,
// then this thread or another thread has already handled the list construction,
// so just add to the list

// If links still points to EMPTY inside the synchronized block, then construct a new
// CopyOnWriteArrayList
// containing the newly added link

List<AgentSpanLink> links = this.links;
if (links != EMPTY) {
links.add(link);
return;
}

synchronized (this) {
links = this.links;
if (links != EMPTY) {
links.add(link);
} else {
this.links = new CopyOnWriteArrayList<>(Collections.singletonList(link));
}
}
}

Expand Down
Loading