Skip to content

Commit 292356e

Browse files
marcingrzejszczakmarcingrzejszczak
authored
Not creating scopes when resetting scope on trace handler (#214)
without this change, we're using the `tracer.withSpan(null)` to reset the thread local, however the better alternative is to use the `CurrentTraceContext.maybeScope` mechanism. with this change we're changing the way the `TracingObservationHandler` is clearing scopes. We also needed to improve the testing tracer to support these changes fixes gh-213
1 parent 504266a commit 292356e

File tree

11 files changed

+307
-70
lines changed

11 files changed

+307
-70
lines changed

‎micrometer-tracing-bridges/micrometer-tracing-bridge-brave/src/test/java/io/micrometer/tracing/brave/bridge/BraveTracingApiTests.java

+61-5
Original file line numberDiff line numberDiff line change
@@ -24,14 +24,24 @@
2424
import brave.propagation.StrictCurrentTraceContext;
2525
import brave.sampler.Sampler;
2626
import brave.test.TestSpanHandler;
27+
import io.micrometer.context.ContextAccessor;
28+
import io.micrometer.context.ContextRegistry;
29+
import io.micrometer.context.ContextSnapshot;
30+
import io.micrometer.observation.Observation;
31+
import io.micrometer.observation.ObservationRegistry;
32+
import io.micrometer.observation.contextpropagation.ObservationThreadLocalAccessor;
2733
import io.micrometer.tracing.*;
34+
import io.micrometer.tracing.handler.DefaultTracingObservationHandler;
2835
import org.junit.jupiter.api.AfterEach;
2936
import org.junit.jupiter.api.Test;
3037

38+
import java.util.Map;
3139
import java.util.concurrent.ExecutorService;
3240
import java.util.concurrent.Executors;
41+
import java.util.function.Predicate;
3342

3443
import static org.assertj.core.api.BDDAssertions.then;
44+
import static org.assertj.core.api.BDDAssertions.thenNoException;
3545

3646
/**
3747
* Sources for tracing-api.adoc
@@ -166,21 +176,20 @@ void should_work_with_baggage() {
166176
Baggage baggageForSpanInScopeTwo = tracer.createBaggage("from_span_in_scope 2").set("value 2");
167177

168178
try (BaggageInScope baggage = baggageForSpanInScopeOne.makeCurrent()) {
169-
then(baggageForSpanInScopeOne.get()).as("[In scope] Baggage 1").isEqualTo("value 1");
179+
then(baggage.get()).as("[In scope] Baggage 1").isEqualTo("value 1");
170180
then(tracer.getBaggage("from_span_in_scope 1").get()).as("[In scope] Baggage 1").isEqualTo("value 1");
171181
}
172182

173183
try (BaggageInScope baggage = baggageForSpanInScopeTwo.makeCurrent()) {
174-
then(baggageForSpanInScopeTwo.get()).as("[In scope] Baggage 2").isEqualTo("value 2");
184+
then(baggage.get()).as("[In scope] Baggage 2").isEqualTo("value 2");
175185
then(tracer.getBaggage("from_span_in_scope 2").get()).as("[In scope] Baggage 2").isEqualTo("value 2");
176186
}
177187
}
178188

179189
// Assuming that you have a handle to the span
180190
Baggage baggageForExplicitSpan = tracer.createBaggage("from_span").set(span.context(), "value 3");
181191
try (BaggageInScope baggage = baggageForExplicitSpan.makeCurrent()) {
182-
then(baggageForExplicitSpan.get(span.context())).as("[Span passed explicitly] Baggage 3")
183-
.isEqualTo("value 3");
192+
then(baggage.get(span.context())).as("[Span passed explicitly] Baggage 3").isEqualTo("value 3");
184193
then(tracer.getBaggage("from_span").get(span.context())).as("[Span passed explicitly] Baggage 3")
185194
.isEqualTo("value 3");
186195
}
@@ -191,7 +200,7 @@ void should_work_with_baggage() {
191200
// When there's no span in scope, there will never be any baggage - even if you
192201
// make it current
193202
try (BaggageInScope baggage = baggageFour.makeCurrent()) {
194-
then(baggageFour.get()).as("[Out of span scope] Baggage 1").isNull();
203+
then(baggage.get()).as("[Out of span scope] Baggage 1").isNull();
195204
then(tracer.getBaggage("from_span_in_scope 1").get()).as("[Out of span scope] Baggage 1").isNull();
196205
}
197206
then(tracer.getBaggage("from_span_in_scope 1").get()).as("[Out of scope] Baggage 1").isNull();
@@ -203,4 +212,51 @@ void should_work_with_baggage() {
203212
.isEqualTo("value 3");
204213
}
205214

215+
@Test
216+
void should_not_break_on_scopes() {
217+
ObservationRegistry registry = ObservationRegistry.create();
218+
registry.observationConfig().observationHandler(new DefaultTracingObservationHandler(tracer));
219+
220+
ContextRegistry.getInstance().registerContextAccessor(new ContextAccessor<Observation, Observation>() {
221+
@Override
222+
public Class<? extends Observation> readableType() {
223+
return Observation.class;
224+
}
225+
226+
@Override
227+
public void readValues(Observation sourceContext, Predicate<Object> keyPredicate,
228+
Map<Object, Object> readValues) {
229+
readValues.put(ObservationThreadLocalAccessor.KEY, sourceContext);
230+
}
231+
232+
@Override
233+
public <T> T readValue(Observation sourceContext, Object key) {
234+
return (T) sourceContext;
235+
}
236+
237+
@Override
238+
public Class<? extends Observation> writeableType() {
239+
return Observation.class;
240+
}
241+
242+
@Override
243+
public Observation writeValues(Map<Object, Object> valuesToWrite, Observation targetContext) {
244+
return (Observation) valuesToWrite.get(ObservationThreadLocalAccessor.KEY);
245+
}
246+
});
247+
248+
Observation obs0 = Observation.createNotStarted("observation-0", registry);
249+
Observation obs1 = Observation.createNotStarted("observation-1", registry);
250+
251+
thenNoException().isThrownBy(() -> {
252+
try (Observation.Scope scope = obs0.start().openScope()) {
253+
try (Observation.Scope scope2 = obs1.start().openScope()) {
254+
try (ContextSnapshot.Scope scope3 = ContextSnapshot.setAllThreadLocalsFrom(obs1)) {
255+
// do sth here
256+
}
257+
}
258+
}
259+
});
260+
}
261+
206262
}

‎micrometer-tracing-bridges/micrometer-tracing-bridge-otel/src/test/java/io/micrometer/tracing/otel/bridge/OtelTracingApiTests.java

+57
Original file line numberDiff line numberDiff line change
@@ -15,10 +15,17 @@
1515
*/
1616
package io.micrometer.tracing.otel.bridge;
1717

18+
import io.micrometer.context.ContextAccessor;
19+
import io.micrometer.context.ContextRegistry;
20+
import io.micrometer.context.ContextSnapshot;
21+
import io.micrometer.observation.Observation;
22+
import io.micrometer.observation.ObservationRegistry;
23+
import io.micrometer.observation.contextpropagation.ObservationThreadLocalAccessor;
1824
import io.micrometer.tracing.Baggage;
1925
import io.micrometer.tracing.BaggageInScope;
2026
import io.micrometer.tracing.Span;
2127
import io.micrometer.tracing.Tracer;
28+
import io.micrometer.tracing.handler.DefaultTracingObservationHandler;
2229
import io.opentelemetry.context.propagation.ContextPropagators;
2330
import io.opentelemetry.extension.trace.propagation.B3Propagator;
2431
import io.opentelemetry.sdk.OpenTelemetrySdk;
@@ -30,11 +37,14 @@
3037
import org.junit.jupiter.api.Test;
3138

3239
import java.util.Collections;
40+
import java.util.Map;
3341
import java.util.concurrent.ExecutorService;
3442
import java.util.concurrent.Executors;
43+
import java.util.function.Predicate;
3544

3645
import static io.opentelemetry.sdk.trace.samplers.Sampler.alwaysOn;
3746
import static org.assertj.core.api.BDDAssertions.then;
47+
import static org.assertj.core.api.BDDAssertions.thenNoException;
3848

3949
class OtelTracingApiTests {
4050

@@ -213,4 +223,51 @@ void should_work_with_baggage() {
213223
.isEqualTo("value 3");
214224
}
215225

226+
@Test
227+
void should_not_break_on_scopes() {
228+
ObservationRegistry registry = ObservationRegistry.create();
229+
registry.observationConfig().observationHandler(new DefaultTracingObservationHandler(tracer));
230+
231+
ContextRegistry.getInstance().registerContextAccessor(new ContextAccessor<Observation, Observation>() {
232+
@Override
233+
public Class<? extends Observation> readableType() {
234+
return Observation.class;
235+
}
236+
237+
@Override
238+
public void readValues(Observation sourceContext, Predicate<Object> keyPredicate,
239+
Map<Object, Object> readValues) {
240+
readValues.put(ObservationThreadLocalAccessor.KEY, sourceContext);
241+
}
242+
243+
@Override
244+
public <T> T readValue(Observation sourceContext, Object key) {
245+
return (T) sourceContext;
246+
}
247+
248+
@Override
249+
public Class<? extends Observation> writeableType() {
250+
return Observation.class;
251+
}
252+
253+
@Override
254+
public Observation writeValues(Map<Object, Object> valuesToWrite, Observation targetContext) {
255+
return (Observation) valuesToWrite.get(ObservationThreadLocalAccessor.KEY);
256+
}
257+
});
258+
259+
Observation obs0 = Observation.createNotStarted("observation-0", registry);
260+
Observation obs1 = Observation.createNotStarted("observation-1", registry);
261+
262+
thenNoException().isThrownBy(() -> {
263+
try (Observation.Scope scope = obs0.start().openScope()) {
264+
try (Observation.Scope scope2 = obs1.start().openScope()) {
265+
try (ContextSnapshot.Scope scope3 = ContextSnapshot.setAllThreadLocalsFrom(obs1)) {
266+
// do sth here
267+
}
268+
}
269+
}
270+
});
271+
}
272+
216273
}

‎micrometer-tracing-tests/micrometer-tracing-test/src/main/java/io/micrometer/tracing/test/simple/SimpleCurrentTraceContext.java

+35-15
Original file line numberDiff line numberDiff line change
@@ -15,15 +15,15 @@
1515
*/
1616
package io.micrometer.tracing.test.simple;
1717

18+
import io.micrometer.tracing.CurrentTraceContext;
19+
import io.micrometer.tracing.Span;
20+
import io.micrometer.tracing.TraceContext;
21+
1822
import java.util.Objects;
1923
import java.util.concurrent.Callable;
2024
import java.util.concurrent.Executor;
2125
import java.util.concurrent.ExecutorService;
2226

23-
import io.micrometer.tracing.CurrentTraceContext;
24-
import io.micrometer.tracing.Span;
25-
import io.micrometer.tracing.TraceContext;
26-
2727
/**
2828
* A test implementation of a current trace context.
2929
*
@@ -53,22 +53,18 @@ public TraceContext context() {
5353

5454
@Override
5555
public Scope newScope(TraceContext context) {
56-
Span span = Objects.requireNonNull(SimpleSpanAndScope.getSpanForTraceContext(context),
57-
"You must create a span with this context before");
58-
SimpleSpanInScope inScope = this.simpleTracer.withSpan(span);
59-
return inScope::close;
56+
SimpleSpan previous = SimpleTracer.getCurrentSpan();
57+
SimpleTracer.setCurrentSpan(context);
58+
return previous != null ? new RevertToPreviousScope(previous) : new RevertToNullScope();
6059
}
6160

6261
@Override
6362
public Scope maybeScope(TraceContext context) {
64-
Span span = Objects.requireNonNull(SimpleSpanAndScope.getSpanForTraceContext(context),
65-
"You must create a span with this context before");
66-
if (this.simpleTracer.currentSpan() == span) {
67-
return () -> {
68-
69-
};
63+
SimpleSpan current = SimpleTracer.getCurrentSpan();
64+
if (Objects.equals(current != null ? current.context() : current, context)) {
65+
return Scope.NOOP;
7066
}
71-
return newScope(span.context());
67+
return newScope(context);
7268
}
7369

7470
@Override
@@ -91,4 +87,28 @@ public ExecutorService wrap(ExecutorService delegate) {
9187
return delegate;
9288
}
9389

90+
private static final class RevertToNullScope implements Scope {
91+
92+
@Override
93+
public void close() {
94+
SimpleTracer.resetCurrentSpan();
95+
}
96+
97+
}
98+
99+
private static final class RevertToPreviousScope implements Scope {
100+
101+
final SimpleSpan previous;
102+
103+
RevertToPreviousScope(SimpleSpan previous) {
104+
this.previous = previous;
105+
}
106+
107+
@Override
108+
public void close() {
109+
SimpleTracer.setCurrentSpan(this.previous);
110+
}
111+
112+
}
113+
94114
}

‎micrometer-tracing-tests/micrometer-tracing-test/src/main/java/io/micrometer/tracing/test/simple/SimpleSpan.java

+1-1
Original file line numberDiff line numberDiff line change
@@ -65,7 +65,7 @@ public class SimpleSpan implements Span, FinishedSpan {
6565
* Creates a new instance of {@link SimpleSpan}.
6666
*/
6767
public SimpleSpan() {
68-
SimpleSpanAndScope.bindSpanToTraceContext(context(), this);
68+
SimpleTracer.bindSpanToTraceContext(context(), this);
6969
}
7070

7171
@Override

‎micrometer-tracing-tests/micrometer-tracing-test/src/main/java/io/micrometer/tracing/test/simple/SimpleSpanAndScope.java

+3-25
Original file line numberDiff line numberDiff line change
@@ -15,16 +15,14 @@
1515
*/
1616
package io.micrometer.tracing.test.simple;
1717

18-
import java.util.Map;
19-
import java.util.Objects;
20-
import java.util.concurrent.ConcurrentHashMap;
21-
2218
import io.micrometer.common.lang.Nullable;
2319
import io.micrometer.tracing.Span;
2420
import io.micrometer.tracing.SpanAndScope;
2521
import io.micrometer.tracing.TraceContext;
2622
import io.micrometer.tracing.Tracer;
2723

24+
import java.util.Objects;
25+
2826
/**
2927
* Container object for {@link Span} and its corresponding {@link Tracer.SpanInScope}.
3028
*
@@ -34,8 +32,6 @@
3432
*/
3533
public class SimpleSpanAndScope extends SpanAndScope {
3634

37-
private static final Map<TraceContext, Span> traceContextToSpans = new ConcurrentHashMap<>();
38-
3935
private final TraceContext traceContext;
4036

4137
/**
@@ -54,7 +50,7 @@ public SimpleSpanAndScope(Span span, @Nullable Tracer.SpanInScope scope) {
5450
* @param scope scope
5551
*/
5652
public SimpleSpanAndScope(TraceContext traceContext, @Nullable Tracer.SpanInScope scope) {
57-
super(Objects.requireNonNull(traceContextToSpans.get(traceContext),
53+
super(Objects.requireNonNull(SimpleTracer.getSpanForTraceContext(traceContext),
5854
"You must create a span with this context before"), scope);
5955
this.traceContext = traceContext;
6056
}
@@ -67,22 +63,4 @@ public TraceContext getTraceContext() {
6763
return traceContext;
6864
}
6965

70-
/**
71-
* Binds the given {@link Span} to the given {@link TraceContext}.
72-
* @param traceContext the traceContext to use to bind this span to
73-
* @param span the span that needs to be bounded to the traceContext
74-
*/
75-
static void bindSpanToTraceContext(TraceContext traceContext, Span span) {
76-
traceContextToSpans.put(traceContext, span);
77-
}
78-
79-
/**
80-
* Returns the {@link Span} that is bounded to the given {@link TraceContext}.
81-
* @param traceContext the traceContext to use to fetch the span
82-
* @return the span that is bounded to the given traceContext (null if none)
83-
*/
84-
static Span getSpanForTraceContext(TraceContext traceContext) {
85-
return traceContextToSpans.get(traceContext);
86-
}
87-
8866
}

0 commit comments

Comments
 (0)