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

Open
dougqh wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Avoid creation of empty CopyOnWriteArrayList for span links #10822

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
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
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
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
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
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 :)

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