-
Notifications
You must be signed in to change notification settings - Fork 331
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.
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
base: master
Are you sure you want to change the base?
Changes from all commits
40fc417
34fddd2
43a60d5
4b7e5ef
9410a6f
f453110
8aa187e
9dd37e5
17a0044
9c0745a
ca95b42
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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(); | ||
| 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 ? new CopyOnWriteArrayList<>() : new CopyOnWriteArrayList<>(links); | ||
| this.links = links == null || links.isEmpty() ? EMPTY : new CopyOnWriteArrayList<>(links); | ||
| } | ||
|
|
||
| public boolean isFinished() { | ||
|
|
@@ -764,12 +765,12 @@ public CharSequence getType() { | |
|
|
||
| @Override | ||
| public void processServiceTags() { | ||
| context.earlyProcessTags(links); | ||
| context.earlyProcessTags(this); | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this DDSpan is now a SpanLinkAccessor / WritableSpanLinks
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Updated - now just an AppendableSpanLinks (previously WritableSpanLinks) |
||
| } | ||
|
|
||
| @Override | ||
| public void processTagsAndBaggage(final MetadataConsumer consumer) { | ||
| context.processTagsAndBaggage(consumer, longRunningVersion, links); | ||
| context.processTagsAndBaggage(consumer, longRunningVersion, this); | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this DDSpan is now a SpanLinkAccessor / WritableSpanLinks
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Updated - now just an AppendableSpanLinks (previously WritableSpanLinks) |
||
| } | ||
|
|
||
| @Override | ||
|
|
@@ -883,10 +884,44 @@ public TraceConfig traceConfig() { | |
| return context.getTraceCollector().getTraceConfig(); | ||
| } | ||
|
|
||
| List<? extends AgentSpanLink> getLinks() { | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've kept this accessor, but it is now only package visible. |
||
| return this.links; | ||
| } | ||
|
|
||
| @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)); | ||
| } | ||
| } | ||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -24,7 +24,7 @@ | |||||
| import datadog.trace.api.sampling.PrioritySampling; | ||||||
| import datadog.trace.api.sampling.SamplingMechanism; | ||||||
| import datadog.trace.bootstrap.instrumentation.api.AgentSpanContext; | ||||||
| import datadog.trace.bootstrap.instrumentation.api.AgentSpanLink; | ||||||
| import datadog.trace.bootstrap.instrumentation.api.AppendableSpanLinks; | ||||||
| import datadog.trace.bootstrap.instrumentation.api.Baggage; | ||||||
| import datadog.trace.bootstrap.instrumentation.api.ProfilerContext; | ||||||
| import datadog.trace.bootstrap.instrumentation.api.ProfilingContextIntegration; | ||||||
|
|
@@ -1087,19 +1087,23 @@ public <T> void setMetaStruct(final String field, final T value) { | |||||
| } | ||||||
| } | ||||||
|
|
||||||
| public void earlyProcessTags(List<AgentSpanLink> links) { | ||||||
| void earlyProcessTags(AppendableSpanLinks links) { | ||||||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Reduced the visibility of earlyProcessTags and processTagsAndBaggage |
||||||
| synchronized (unsafeTags) { | ||||||
| TagsPostProcessorFactory.eagerProcessor().processTags(unsafeTags, this, links); | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
| public void processTagsAndBaggage( | ||||||
| final MetadataConsumer consumer, int longRunningVersion, List<AgentSpanLink> links) { | ||||||
| void processTagsAndBaggage( | ||||||
| final MetadataConsumer consumer, int longRunningVersion, DDSpan restrictedSpan) { | ||||||
| // NOTE: The span is passed for the sole purpose of allowing updating & reading of the span links | ||||||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The name |
||||||
| // This is a compromise to avoid... | ||||||
| // - creating an extra wrapper object that would create significant allocation | ||||||
| // - implementing an interface to read the spans that require making the read method public | ||||||
| synchronized (unsafeTags) { | ||||||
| // Tags | ||||||
| TagsPostProcessorFactory.lazyProcessor().processTags(unsafeTags, this, links); | ||||||
| TagsPostProcessorFactory.lazyProcessor().processTags(unsafeTags, this, restrictedSpan); | ||||||
|
|
||||||
| String linksTag = DDSpanLink.toTag(links); | ||||||
| String linksTag = DDSpanLink.toTag(restrictedSpan.getLinks()); | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🎯 suggestion: We don't even need to introduce
Suggested change
|
||||||
| if (linksTag != null) { | ||||||
| unsafeTags.put(SPAN_LINKS, linksTag); | ||||||
| } | ||||||
|
|
||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -69,7 +69,7 @@ public static SpanLink from(ExtractedContext context, SpanAttributes attributes) | |
| * @param links The span link collection to encode. | ||
| * @return The encoded tag value, {@code null} if no links. | ||
| */ | ||
| public static String toTag(List<AgentSpanLink> links) { | ||
| public static String toTag(List<? extends AgentSpanLink> links) { | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Updated to accommodate return from SpanLinkAccessor.getLinks |
||
| if (links == null || links.isEmpty()) { | ||
| return null; | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,23 +1,10 @@ | ||
| package datadog.trace.core.tagprocessor; | ||
|
|
||
| import datadog.trace.api.TagMap; | ||
| import datadog.trace.bootstrap.instrumentation.api.AgentSpanLink; | ||
| import datadog.trace.bootstrap.instrumentation.api.AppendableSpanLinks; | ||
| import datadog.trace.core.DDSpanContext; | ||
| import java.util.List; | ||
| import java.util.Map; | ||
|
|
||
| public abstract class TagsPostProcessor { | ||
| /* | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Got rid of the old processTags which I had kept around for test migration. |
||
| * DQH - For testing purposes only | ||
| */ | ||
| @Deprecated | ||
| final Map<String, Object> processTags( | ||
| Map<String, Object> unsafeTags, DDSpanContext context, List<AgentSpanLink> links) { | ||
| TagMap map = TagMap.fromMap(unsafeTags); | ||
| this.processTags(map, context, links); | ||
| return map; | ||
| } | ||
|
|
||
| public abstract void processTags( | ||
| TagMap unsafeTags, DDSpanContext spanContext, List<AgentSpanLink> spanLinks); | ||
| TagMap unsafeTags, DDSpanContext spanContext, AppendableSpanLinks spanLinks); | ||
| } | ||
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.