Skip to content

Commit 701d1e5

Browse files
dougqhdougqh
authored
Add primitive setters to DDSpanContext to take advantage of TagMap (#10018)
* Extracting static helper * Extract static version of buildSpanContext * Switch to singleSpanBuilder * Extracting static version of startSpan * spotless * Reorganizing link & parent resolution Previous static extraction caused a test to fail because links logic relied on having a side effect on the builder instance's List<AgentSpanLink> * spotless * Primitive setters for DDSpanContext Adding primitive setters to DDSpanContext to take advantage of TagMap primitive storage Using precheck of tagInterceptor.needsIntercept to avoid boxing while calling tagInterceptor.interceptTag * Update DDSpanContext.java * Fixing silly oversight of having duplicate methods taking a long * Removing boolean return * Fixed missing negation in setBox - causing some tests to fail * Silencing Mock checking To simplify the code, I pulled CoreTracer.getTagInterceptor() into a member variable of DDSpanContext Unfortunately, that caused Mock call checks to fail, so needed to update the PendingTraceBufferTest * spotless * Add setter for float
1 parent 90c739d commit 701d1e5

File tree

6 files changed

+165
-8
lines changed

6 files changed

+165
-8
lines changed

‎dd-java-agent/instrumentation-testing/src/main/groovy/datadog/trace/agent/test/TrackingSpanDecorator.groovy‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -134,6 +134,11 @@ class TrackingSpanDecorator implements AgentSpan {
134134
return delegate.setTag(key, value)
135135
}
136136

137+
@Override
138+
AgentSpan setTag(String key, float value) {
139+
return delegate.setTag(key, value)
140+
}
141+
137142
@Override
138143
AgentSpan setTag(String key, double value) {
139144
return delegate.setTag(key, value)

‎dd-trace-core/src/main/java/datadog/trace/core/DDSpan.java‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -432,6 +432,12 @@ public DDSpan setTag(final String tag, final long value) {
432432
return this;
433433
}
434434

435+
@Override
436+
public DDSpan setTag(final String tag, final float value) {
437+
context.setTag(tag, value);
438+
return this;
439+
}
440+
435441
@Override
436442
public DDSpan setTag(final String tag, final double value) {
437443
context.setTag(tag, value);

‎dd-trace-core/src/main/java/datadog/trace/core/DDSpanContext.java‎

Lines changed: 133 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -76,6 +76,8 @@ public class DDSpanContext
7676
/** The collection of all span related to this one */
7777
private final TraceCollector traceCollector;
7878

79+
private final TagInterceptor tagInterceptor;
80+
7981
/** Baggage is associated with the whole trace and shared with other spans */
8082
private volatile Map<String, String> baggageItems;
8183

@@ -330,6 +332,7 @@ public DDSpanContext(
330332

331333
assert traceCollector != null;
332334
this.traceCollector = traceCollector;
335+
this.tagInterceptor = this.traceCollector.getTracer().getTagInterceptor();
333336

334337
assert traceId != null;
335338
this.traceId = traceId;
@@ -766,9 +769,122 @@ public void setTag(final String tag, final Object value) {
766769
synchronized (unsafeTags) {
767770
unsafeTags.remove(tag);
768771
}
769-
} else if (!traceCollector.getTracer().getTagInterceptor().interceptTag(this, tag, value)) {
772+
} else if (!tagInterceptor.interceptTag(this, tag, value)) {
773+
synchronized (unsafeTags) {
774+
unsafeTags.set(tag, value);
775+
}
776+
}
777+
}
778+
779+
public void setTag(final String tag, final String value) {
780+
if (null == tag) {
781+
return;
782+
}
783+
if (null == value) {
784+
synchronized (unsafeTags) {
785+
unsafeTags.remove(tag);
786+
}
787+
} else if (!tagInterceptor.interceptTag(this, tag, value)) {
788+
synchronized (unsafeTags) {
789+
unsafeTags.set(tag, value);
790+
}
791+
}
792+
}
793+
794+
/*
795+
* Uses to determine if there's an opportunity to avoid primitve boxing.
796+
* If the underlying map doesn't support efficient primitives, then boxing is used.
797+
* If the tag may be intercepted, then boxing is also used.
798+
*/
799+
private boolean precheckIntercept(String tag) {
800+
// Usually only a single instanceof TagMap will be loaded,
801+
// so isOptimized is turned into a direct call and then inlines to a constant
802+
// Since isOptimized just returns a constant - doesn't require synchronization
803+
return !unsafeTags.isOptimized() || tagInterceptor.needsIntercept(tag);
804+
}
805+
806+
/*
807+
* Used when precheckIntercept determines that boxing is unavoidable
808+
*
809+
* Either because the tagInterceptor needs to be fully checked (which requires boxing)
810+
* In that case, a box has already been created so it makes sense to pass the box
811+
* onto TagMap, since optimized TagMap will cache the box
812+
*
813+
* -- OR --
814+
*
815+
* The TagMap isn't optimized and will need to box the primitive regardless of
816+
* tag interception
817+
*/
818+
private void setBox(String tag, Object box) {
819+
if (!tagInterceptor.interceptTag(this, tag, box)) {
820+
synchronized (unsafeTags) {
821+
unsafeTags.set(tag, box);
822+
}
823+
}
824+
}
825+
826+
public void setTag(final String tag, final boolean value) {
827+
if (null == tag) {
828+
return;
829+
}
830+
if (precheckIntercept(tag)) {
831+
this.setBox(tag, value);
832+
} else {
770833
synchronized (unsafeTags) {
771-
unsafeSetTag(tag, value);
834+
unsafeTags.set(tag, value);
835+
}
836+
}
837+
}
838+
839+
public void setTag(final String tag, final int value) {
840+
if (null == tag) {
841+
return;
842+
}
843+
if (precheckIntercept(tag)) {
844+
this.setBox(tag, value);
845+
} else {
846+
synchronized (unsafeTags) {
847+
unsafeTags.set(tag, value);
848+
}
849+
}
850+
}
851+
852+
public void setTag(final String tag, final long value) {
853+
if (null == tag) {
854+
return;
855+
}
856+
// check needsIntercept first to avoid unnecessary boxing
857+
boolean intercepted =
858+
tagInterceptor.needsIntercept(tag) && tagInterceptor.interceptTag(this, tag, value);
859+
if (!intercepted) {
860+
synchronized (unsafeTags) {
861+
unsafeTags.set(tag, value);
862+
}
863+
}
864+
}
865+
866+
public void setTag(final String tag, final float value) {
867+
if (null == tag) {
868+
return;
869+
}
870+
if (precheckIntercept(tag)) {
871+
this.setBox(tag, value);
872+
} else {
873+
synchronized (unsafeTags) {
874+
unsafeTags.set(tag, value);
875+
}
876+
}
877+
}
878+
879+
public void setTag(final String tag, final double value) {
880+
if (null == tag) {
881+
return;
882+
}
883+
if (precheckIntercept(tag)) {
884+
this.setBox(tag, value);
885+
} else {
886+
synchronized (unsafeTags) {
887+
unsafeTags.set(tag, value);
772888
}
773889
}
774890
}
@@ -789,12 +905,11 @@ void setAllTags(final TagMap map, boolean needsIntercept) {
789905
// to avoid using a capturing lambda
790906
map.forEach(
791907
this,
792-
traceCollector.getTracer().getTagInterceptor(),
793-
(ctx, tagInterceptor, tagEntry) -> {
908+
(ctx, tagEntry) -> {
794909
String tag = tagEntry.tag();
795910
Object value = tagEntry.objectValue();
796911

797-
if (!tagInterceptor.interceptTag(ctx, tag, value)) {
912+
if (!ctx.tagInterceptor.interceptTag(ctx, tag, value)) {
798913
ctx.unsafeTags.set(tagEntry);
799914
}
800915
});
@@ -809,7 +924,6 @@ void setAllTags(final TagMap.Ledger ledger) {
809924
return;
810925
}
811926

812-
TagInterceptor tagInterceptor = traceCollector.getTracer().getTagInterceptor();
813927
synchronized (unsafeTags) {
814928
for (final TagMap.EntryChange entryChange : ledger) {
815929
if (entryChange.isRemoval()) {
@@ -834,7 +948,6 @@ void setAllTags(final Map<String, ?> map) {
834948
} else if (map instanceof TagMap) {
835949
setAllTags((TagMap) map);
836950
} else if (!map.isEmpty()) {
837-
TagInterceptor tagInterceptor = traceCollector.getTracer().getTagInterceptor();
838951
synchronized (unsafeTags) {
839952
for (final Map.Entry<String, ?> tag : map.entrySet()) {
840953
if (!tagInterceptor.interceptTag(this, tag.getKey(), tag.getValue())) {
@@ -846,7 +959,19 @@ void setAllTags(final Map<String, ?> map) {
846959
}
847960

848961
void unsafeSetTag(final String tag, final Object value) {
849-
unsafeTags.put(tag, value);
962+
unsafeTags.set(tag, value);
963+
}
964+
965+
void unsafeSetTag(final String tag, final CharSequence value) {
966+
unsafeTags.set(tag, value);
967+
}
968+
969+
void unsafeSetTag(final String tag, final byte value) {
970+
unsafeTags.set(tag, value);
971+
}
972+
973+
void unsafeSetTag(final String tag, final double value) {
974+
unsafeTags.set(tag, value);
850975
}
851976

852977
Object getTag(final String key) {

‎dd-trace-core/src/test/groovy/datadog/trace/core/PendingTraceBufferTest.groovy‎

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -104,6 +104,7 @@ class PendingTraceBufferTest extends DDSpecification {
104104
1 * bufferSpy.longRunningSpansEnabled()
105105
1 * bufferSpy.enqueue(trace)
106106
_ * tracer.getPartialFlushMinSpans() >> 10
107+
_ * tracer.getTagInterceptor()
107108
1 * tracer.getTimeWithNanoTicks(_)
108109
1 * tracer.onRootSpanPublished(span)
109110
0 * _
@@ -137,6 +138,7 @@ class PendingTraceBufferTest extends DDSpecification {
137138
1 * bufferSpy.enqueue(trace)
138139
_ * bufferSpy.longRunningSpansEnabled()
139140
_ * tracer.getPartialFlushMinSpans() >> 10
141+
_ * tracer.getTagInterceptor()
140142
1 * tracer.getTimeWithNanoTicks(_)
141143
1 * tracer.onRootSpanPublished(parent)
142144
0 * _
@@ -151,6 +153,7 @@ class PendingTraceBufferTest extends DDSpecification {
151153
1 * tracer.write({ it.size() == 2 })
152154
1 * tracer.writeTimer() >> Monitoring.DISABLED.newTimer("")
153155
_ * tracer.getPartialFlushMinSpans() >> 10
156+
_ * tracer.getTagInterceptor()
154157
1 * tracer.getTimeWithNanoTicks(_)
155158
0 * _
156159
}
@@ -168,6 +171,7 @@ class PendingTraceBufferTest extends DDSpecification {
168171

169172
then:
170173
_ * tracer.getPartialFlushMinSpans() >> 10
174+
_ * tracer.getTagInterceptor()
171175
_ * traceConfig.getServiceMapping() >> [:]
172176
_ * tracer.getTimeWithNanoTicks(_)
173177
1 * tracer.writeTimer() >> Monitoring.DISABLED.newTimer("")
@@ -194,6 +198,7 @@ class PendingTraceBufferTest extends DDSpecification {
194198
buffer.queue.capacity() * bufferSpy.enqueue(_)
195199
_ * bufferSpy.longRunningSpansEnabled()
196200
_ * tracer.getPartialFlushMinSpans() >> 10
201+
_ * tracer.getTagInterceptor()
197202
_ * traceConfig.getServiceMapping() >> [:]
198203
_ * tracer.getTimeWithNanoTicks(_)
199204
buffer.queue.capacity() * tracer.onRootSpanPublished(_)
@@ -210,6 +215,7 @@ class PendingTraceBufferTest extends DDSpecification {
210215
_ * bufferSpy.longRunningSpansEnabled()
211216
1 * tracer.write({ it.size() == 1 })
212217
_ * tracer.getPartialFlushMinSpans() >> 10
218+
_ * tracer.getTagInterceptor()
213219
_ * traceConfig.getServiceMapping() >> [:]
214220
2 * tracer.getTimeWithNanoTicks(_)
215221
1 * tracer.onRootSpanPublished(_)
@@ -232,6 +238,7 @@ class PendingTraceBufferTest extends DDSpecification {
232238
buffer.queue.capacity() * bufferSpy.enqueue(_)
233239
_ * bufferSpy.longRunningSpansEnabled()
234240
_ * tracer.getPartialFlushMinSpans() >> 10
241+
_ * tracer.getTagInterceptor()
235242
_ * traceConfig.getServiceMapping() >> [:]
236243
_ * tracer.getTimeWithNanoTicks(_)
237244
buffer.queue.capacity() * tracer.onRootSpanPublished(_)
@@ -250,6 +257,7 @@ class PendingTraceBufferTest extends DDSpecification {
250257
_ * bufferSpy.longRunningSpansEnabled()
251258
0 * tracer.write({ it.size() == 1 })
252259
_ * tracer.getPartialFlushMinSpans() >> 10
260+
_ * tracer.getTagInterceptor()
253261
_ * traceConfig.getServiceMapping() >> [:]
254262
_ * tracer.getTimeWithNanoTicks(_)
255263
1 * tracer.onRootSpanPublished(_)
@@ -279,6 +287,7 @@ class PendingTraceBufferTest extends DDSpecification {
279287
_ * bufferSpy.longRunningSpansEnabled()
280288
1 * bufferSpy.enqueue(trace)
281289
_ * tracer.getPartialFlushMinSpans() >> 10
290+
_ * tracer.getTagInterceptor()
282291
1 * tracer.getTimeWithNanoTicks(_)
283292
1 * tracer.onRootSpanPublished(parent)
284293
0 * _
@@ -307,6 +316,7 @@ class PendingTraceBufferTest extends DDSpecification {
307316
latch.countDown()
308317
}
309318
_ * tracer.getPartialFlushMinSpans() >> 10
319+
_ * tracer.getTagInterceptor()
310320
0 * _
311321
}
312322

@@ -333,6 +343,7 @@ class PendingTraceBufferTest extends DDSpecification {
333343
parentLatch.countDown()
334344
}
335345
_ * tracer.getPartialFlushMinSpans() >> 10
346+
_ * tracer.getTagInterceptor()
336347
1 * tracer.getTimeWithNanoTicks(_)
337348
1 * tracer.onRootSpanPublished(parent)
338349
0 * _
@@ -349,6 +360,7 @@ class PendingTraceBufferTest extends DDSpecification {
349360
1 * bufferSpy.enqueue(trace)
350361
_ * bufferSpy.longRunningSpansEnabled()
351362
_ * tracer.getPartialFlushMinSpans() >> 10
363+
_ * tracer.getTagInterceptor()
352364
1 * tracer.writeTimer() >> Monitoring.DISABLED.newTimer("")
353365
1 * tracer.write({ it.size() == 1 }) >> {
354366
childLatch.countDown()
@@ -428,6 +440,7 @@ class PendingTraceBufferTest extends DDSpecification {
428440
1 * tracer.writeTimer() >> Monitoring.DISABLED.newTimer("")
429441
1 * tracer.write({ it.size() == 1 })
430442
1 * tracer.getPartialFlushMinSpans() >> 10000
443+
_ * tracer.getTagInterceptor()
431444
1 * traceConfig.getServiceMapping() >> [:]
432445
2 * tracer.getTimeWithNanoTicks(_)
433446
1 * tracer.onRootSpanPublished(_)
@@ -444,6 +457,7 @@ class PendingTraceBufferTest extends DDSpecification {
444457
buffer.queue.capacity() * bufferSpy.enqueue(_)
445458
_ * bufferSpy.longRunningSpansEnabled()
446459
_ * tracer.getPartialFlushMinSpans() >> 10000
460+
_ * tracer.getTagInterceptor()
447461
_ * traceConfig.getServiceMapping() >> [:]
448462
_ * tracer.getTimeWithNanoTicks(_)
449463
0 * _

‎internal-api/src/main/java/datadog/trace/bootstrap/instrumentation/api/AgentSpan.java‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -66,6 +66,8 @@ default boolean isValid() {
6666

6767
AgentSpan setTag(String key, long value);
6868

69+
AgentSpan setTag(String key, float value);
70+
6971
AgentSpan setTag(String key, double value);
7072

7173
@Override

‎internal-api/src/main/java/datadog/trace/bootstrap/instrumentation/api/ImmutableSpan.java‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,11 @@ public AgentSpan setTag(String key, long value) {
2626
return this;
2727
}
2828

29+
@Override
30+
public AgentSpan setTag(String key, float value) {
31+
return this;
32+
}
33+
2934
@Override
3035
public AgentSpan setTag(String key, double value) {
3136
return this;

0 commit comments

Comments
 (0)