Avoid creation of empty CopyOnWriteArrayList for span links #10822
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -51,7 +51,7 @@ | |
| * <p>Spans are created by the {@link CoreTracer#buildSpan}. This implementation adds some features | ||
| * according to the DD agent. | ||
| */ | ||
| public class DDSpan implements AgentSpan, CoreSpan<DDSpan>, SpanLinkAccessor, AttachableWrapper { | ||
| private static final Logger log = LoggerFactory.getLogger(DDSpan.class); | ||
|
|
||
| static DDSpan create( | ||
|
|
@@ -116,7 +116,8 @@ static DDSpan create( | |
| */ | ||
| private volatile int longRunningVersion = 0; | ||
|
|
||
| private static final List<AgentSpanLink> EMPTY = Collections.emptyList(); | ||
| protected volatile List<AgentSpanLink> links; | ||
|
|
||
| /** | ||
| * Spans should be constructed using the builder, not by calling the constructor directly. | ||
|
|
@@ -144,7 +145,7 @@ private DDSpan( | |
| context.getTraceCollector().touch(); // external clock: explicitly update lastReferenced | ||
| } | ||
|
|
||
| this.links = links == null || links.isEmpty() ? EMPTY : new CopyOnWriteArrayList<>(links); | ||
| } | ||
|
|
||
| public boolean isFinished() { | ||
|
|
@@ -758,12 +759,12 @@ public CharSequence getType() { | |
|
|
||
| @Override | ||
| public void processServiceTags() { | ||
| context.earlyProcessTags(this); | ||
|
Contributor
Author
There was a problem hiding this comment. this DDSpan is now a SpanLinkAccessor / WritableSpanLinks
Contributor
Author
There was a problem hiding this comment. Updated - now just an AppendableSpanLinks (previously WritableSpanLinks) |
||
| } | ||
|
|
||
| @Override | ||
| public void processTagsAndBaggage(final MetadataConsumer consumer) { | ||
| context.processTagsAndBaggage(consumer, longRunningVersion, this); | ||
|
Contributor
Author
There was a problem hiding this comment. this DDSpan is now a SpanLinkAccessor / WritableSpanLinks
Contributor
Author
There was a problem hiding this comment. Updated - now just an AppendableSpanLinks (previously WritableSpanLinks) |
||
| } | ||
|
|
||
| @Override | ||
|
|
@@ -876,11 +877,47 @@ public void setLongRunningVersion(int longRunningVersion) { | |
| public TraceConfig traceConfig() { | ||
| return context.getTraceCollector().getTraceConfig(); | ||
| } | ||
|
|
||
| @Override | ||
| public List<? extends AgentSpanLink> getLinks() { | ||
|
Contributor
Author
There was a problem hiding this comment. I don't entirely like that I had to add this public method, but I took care to make the signature ? extends AgentSpanLink to discourage writing to the list directly.
Contributor
There was a problem hiding this comment. Same, I don't like exposing the links. It was a strong design constraints when I added span links. Additionally, the Javadoc for this method states the collection should be an immutable collection where the return collection is mutable. That's even worst if we want to prevent to 1. show the internal span links implementation 2. contributors to modify span links outside DDSpan. I have open a Slack discussion about fixing the root design issue before trying to unlocking more public API.
Contributor
Author
There was a problem hiding this comment. Yes, I didn't add an immutable wrapper that would negate the some of the gains from this change. For now, I think the best option might be to just drop the
Contributor
There was a problem hiding this comment.
That would indeed "solve" the point I raised here #10822 (comment) |
||
| return this.links; | ||
| } | ||
|
|
||
| @Override | ||
| public void addLink(AgentSpanLink 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)); | ||
| } | ||
| } | ||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,9 @@ | ||
| package datadog.trace.core; | ||
|
|
||
| import datadog.trace.bootstrap.instrumentation.api.AgentSpanLink; | ||
| import datadog.trace.bootstrap.instrumentation.api.WritableSpanLinks; | ||
| import java.util.List; | ||
|
|
||
| public interface SpanLinkAccessor extends WritableSpanLinks { | ||
|
dougqh marked this conversation as resolved.
Outdated
|
||
| public List<? extends AgentSpanLink> getLinks(); | ||
|
dougqh marked this conversation as resolved.
Outdated
|
||
| } | ||
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Few ideas here:
dd-trace-java/dd-trace-core/src/main/java/datadog/trace/core/tagprocessor/SpanPointersProcessor.java
Line 42 in d9df78f
Does it mean it can skip span links and use tags directly?
addSpanLinkmethod asConsumer<SpanLink>. This will avoid exposing the span link implementation and we can keep itnulluntil there is effectively span link stored.Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
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.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
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.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SpanContextwas supposed to describe "a span context", not to hold the span data model.We should be able to use
SpanContextas 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, notSpanContext.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 :)
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
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
SpanContextintoSpanContents.I have a bit of crazy idea to recycle
SpanContents, but I want to do so while maintainingSpanobject identity. In other words, I'd always create a newSpan, but theSpanContentsinside the span would be reused.There was a problem hiding this comment.
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.