Skip to content

Commit 183c78b

Browse files
bantonssonbantonsson
committed
Change tags map to a protected normal HashMap
1 parent 4174006 commit 183c78b

File tree

4 files changed

+65
-37
lines changed

4 files changed

+65
-37
lines changed

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

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,6 @@
99
import java.io.PrintWriter;
1010
import java.io.StringWriter;
1111
import java.lang.ref.WeakReference;
12-
import java.util.Collections;
1312
import java.util.Map;
1413
import java.util.concurrent.TimeUnit;
1514
import java.util.concurrent.atomic.AtomicLong;
@@ -373,7 +372,8 @@ public String getSpanType() {
373372

374373
@Override
375374
public Map<String, Object> getTags() {
376-
return Collections.unmodifiableMap(context.getTags());
375+
// This is an imutable copy of the tags
376+
return context.getTags();
377377
}
378378

379379
public String getType() {

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

Lines changed: 36 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -1,11 +1,13 @@
11
package datadog.trace.core;
22

3+
import com.google.common.collect.ImmutableMap;
34
import datadog.trace.api.DDId;
45
import datadog.trace.api.DDTags;
56
import datadog.trace.api.sampling.PrioritySampling;
67
import datadog.trace.bootstrap.instrumentation.api.AgentSpan;
78
import datadog.trace.core.taginterceptor.AbstractTagInterceptor;
89
import java.util.Collections;
10+
import java.util.HashMap;
911
import java.util.List;
1012
import java.util.Map;
1113
import java.util.TreeMap;
@@ -45,7 +47,7 @@ public class DDSpanContext implements AgentSpan.Context {
4547
private final DDId parentId;
4648

4749
/** Tags are associated to the current span, they will not propagate to the children span */
48-
private final Map<String, Object> tags;
50+
private final Map<String, Object> unsafe_tags;
4951

5052
/** The service name is required, otherwise the span are dropped by the agent */
5153
private volatile String serviceName;
@@ -109,7 +111,7 @@ public DDSpanContext(
109111
// Three is the magic number from the tags below that we set at the end,
110112
// and "* 4 / 3" is to make sure that we don't resize immediately
111113
int capacity = ((tagsSize <= 0 ? 3 : tagsSize + 3) * 4) / 3;
112-
this.tags = new ConcurrentHashMap<>(capacity);
114+
this.unsafe_tags = new HashMap<>(capacity);
113115

114116
this.serviceNameMappings = serviceNameMappings;
115117
setServiceName(serviceName);
@@ -124,12 +126,12 @@ public DDSpanContext(
124126
}
125127

126128
if (origin != null) {
127-
this.tags.put(ORIGIN_KEY, origin);
129+
this.unsafe_tags.put(ORIGIN_KEY, origin);
128130
}
129131
// Additional Metadata
130132
Thread current = Thread.currentThread();
131-
this.tags.put(DDTags.THREAD_NAME, current.getName());
132-
this.tags.put(DDTags.THREAD_ID, current.getId());
133+
this.unsafe_tags.put(DDTags.THREAD_NAME, current.getName());
134+
this.unsafe_tags.put(DDTags.THREAD_ID, current.getId());
133135
}
134136

135137
@Override
@@ -164,7 +166,7 @@ public boolean isResourceNameSet() {
164166
}
165167

166168
public boolean hasResourceName() {
167-
return isResourceNameSet() || tags.containsKey(DDTags.RESOURCE_NAME);
169+
return isResourceNameSet() || getTag(DDTags.RESOURCE_NAME) != null;
168170
}
169171

170172
public void setResourceName(final CharSequence resourceName) {
@@ -322,17 +324,17 @@ public void setMetric(final String key, final Number value) {
322324
* @param value the value of the tag. tags with null values are ignored.
323325
*/
324326
public void setTag(final String tag, final Object value) {
325-
synchronized (tags) {
327+
synchronized (unsafe_tags) {
326328
internalSetTag(tag, value);
327329
}
328330
}
329331

330332
void setAllTags(final Map<String, ? extends Object> map) {
331-
if (map == null) {
333+
if (map == null || map.isEmpty()) {
332334
return;
333335
}
334336

335-
synchronized (tags) {
337+
synchronized (unsafe_tags) {
336338
for (final Map.Entry<String, ? extends Object> tag : map.entrySet()) {
337339
internalSetTag(tag.getKey(), tag.getValue());
338340
}
@@ -341,7 +343,7 @@ void setAllTags(final Map<String, ? extends Object> map) {
341343

342344
private void internalSetTag(final String tag, final Object value) {
343345
if (value == null || (value instanceof String && ((String) value).isEmpty())) {
344-
tags.remove(tag);
346+
unsafe_tags.remove(tag);
345347
return;
346348
}
347349

@@ -363,20 +365,37 @@ private void internalSetTag(final String tag, final Object value) {
363365
}
364366

365367
if (addTag) {
366-
tags.put(tag, value);
368+
unsafe_tags.put(tag, value);
367369
}
368370
}
369371

370372
Object getTag(final String key) {
371-
return tags.get(key);
373+
synchronized (unsafe_tags) {
374+
return unsafe_tags.get(key);
375+
}
372376
}
373377

374378
Object getAndRemoveTag(final String tag) {
375-
return tags.remove(tag);
379+
synchronized (unsafe_tags) {
380+
return unsafe_tags.remove(tag);
381+
}
376382
}
377383

378384
public Map<String, Object> getTags() {
379-
return tags;
385+
synchronized (unsafe_tags) {
386+
return ImmutableMap.copyOf(unsafe_tags);
387+
}
388+
}
389+
390+
public abstract static class TagsAndBaggageConsumer<E extends Exception> {
391+
public abstract void accept(Map<String, Object> tags, Map<String, String> baggage) throws E;
392+
}
393+
394+
public <E extends Exception> void processTagsAndBaggage(TagsAndBaggageConsumer<E> consumer)
395+
throws E {
396+
synchronized (unsafe_tags) {
397+
consumer.accept(unsafe_tags, baggageItems);
398+
}
380399
}
381400

382401
@Override
@@ -401,7 +420,9 @@ public String toString() {
401420
s.append(" *errored*");
402421
}
403422

404-
s.append(" tags=").append(new TreeMap<>(tags));
423+
synchronized (unsafe_tags) {
424+
s.append(" tags=").append(new TreeMap<>(unsafe_tags));
425+
}
405426
return s.toString();
406427
}
407428
}

‎dd-trace-core/src/main/java/datadog/trace/core/serialization/FormatWriter.java‎

Lines changed: 19 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@
1515

1616
import datadog.trace.api.DDId;
1717
import datadog.trace.core.DDSpan;
18+
import datadog.trace.core.DDSpanContext;
1819
import datadog.trace.core.StringTables;
1920
import java.io.IOException;
2021
import java.nio.charset.StandardCharsets;
@@ -95,18 +96,24 @@ public void writeNumberMap(
9596

9697
public void writeMeta(final DDSpan span, final DEST destination) throws IOException {
9798
writeKey(META, destination);
98-
Map<String, String> baggage = span.context().getBaggageItems();
99-
Map<String, Object> tags = span.context().getTags();
100-
writeMapHeader(baggage.size() + tags.size(), destination);
101-
for (Map.Entry<String, String> entry : baggage.entrySet()) {
102-
// tags and baggage may intersect, but tags take priority
103-
if (!tags.containsKey(entry.getKey())) {
104-
writeTag(stringToBytes(entry.getKey()), entry.getValue(), destination);
105-
}
106-
}
107-
for (Map.Entry<String, Object> entry : tags.entrySet()) {
108-
writeObjectAsString(stringToBytes(entry.getKey()), entry.getValue(), destination);
109-
}
99+
span.context()
100+
.processTagsAndBaggage(
101+
new DDSpanContext.TagsAndBaggageConsumer<IOException>() {
102+
@Override
103+
public void accept(Map<String, Object> tags, Map<String, String> baggage)
104+
throws IOException {
105+
writeMapHeader(baggage.size() + tags.size(), destination);
106+
for (Map.Entry<String, String> entry : baggage.entrySet()) {
107+
// tags and baggage may intersect, but tags take priority
108+
if (!tags.containsKey(entry.getKey())) {
109+
writeTag(stringToBytes(entry.getKey()), entry.getValue(), destination);
110+
}
111+
}
112+
for (Map.Entry<String, Object> entry : tags.entrySet()) {
113+
writeObjectAsString(stringToBytes(entry.getKey()), entry.getValue(), destination);
114+
}
115+
}
116+
});
110117
writeMapFooter(destination);
111118
}
112119

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

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -90,8 +90,8 @@ class CoreSpanBuilderTest extends DDSpecification {
9090
context.getServiceName() == expectedService
9191
context.getSpanType() == expectedType
9292

93-
context.tags[THREAD_NAME] == Thread.currentThread().getName()
94-
context.tags[THREAD_ID] == Thread.currentThread().getId()
93+
context.getTag(THREAD_NAME) == Thread.currentThread().getName()
94+
context.getTag(THREAD_ID) == Thread.currentThread().getId()
9595
}
9696

9797
def "setting #name should remove"() {
@@ -302,9 +302,9 @@ class CoreSpanBuilderTest extends DDSpecification {
302302
span.samplingPriority == extractedContext.samplingPriority
303303
span.context().origin == extractedContext.origin
304304
span.context().baggageItems == extractedContext.baggage
305-
span.context().@tags == extractedContext.tags + [(RUNTIME_ID_TAG) : config.getRuntimeId(),
306-
(LANGUAGE_TAG_KEY): LANGUAGE_TAG_VALUE,
307-
(THREAD_NAME) : thread.name, (THREAD_ID): thread.id]
305+
span.context().tags == extractedContext.tags + [(RUNTIME_ID_TAG) : config.getRuntimeId(),
306+
(LANGUAGE_TAG_KEY): LANGUAGE_TAG_VALUE,
307+
(THREAD_NAME) : thread.name, (THREAD_ID): thread.id]
308308

309309
where:
310310
extractedContext | _
@@ -323,9 +323,9 @@ class CoreSpanBuilderTest extends DDSpecification {
323323
span.samplingPriority == null
324324
span.context().origin == tagContext.origin
325325
span.context().baggageItems == [:]
326-
span.context().@tags == tagContext.tags + [(RUNTIME_ID_TAG) : config.getRuntimeId(),
327-
(LANGUAGE_TAG_KEY): LANGUAGE_TAG_VALUE,
328-
(THREAD_NAME) : thread.name, (THREAD_ID): thread.id]
326+
span.context().tags == tagContext.tags + [(RUNTIME_ID_TAG) : config.getRuntimeId(),
327+
(LANGUAGE_TAG_KEY): LANGUAGE_TAG_VALUE,
328+
(THREAD_NAME) : thread.name, (THREAD_ID): thread.id]
329329

330330
where:
331331
tagContext | _

0 commit comments

Comments
 (0)