Skip to content

Commit c72b86f

Browse files
JacekLachJacekLach
committed
Use top span id as the local id
1 parent 20aae2c commit c72b86f

File tree

6 files changed

+59
-78
lines changed

6 files changed

+59
-78
lines changed

‎tracing-jersey/src/main/java/com/palantir/tracing/jersey/TraceEnrichingFilter.java‎

Lines changed: 4 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -46,7 +46,7 @@ public final class TraceEnrichingFilter implements ContainerRequestFilter, Conta
4646
* This is the name of the trace id property we set on {@link ContainerRequestContext}.
4747
*/
4848
public static final String TRACE_ID_PROPERTY_NAME = "com.palantir.tracing.traceId";
49-
public static final String LOCAL_TRACE_ID_PROPERTY_NAME = "com.palantir.tracing.localTraceId";
49+
public static final String TOP_SPAN_ID_PROPERTY_NAME = "com.palantir.tracing.topSpanId";
5050
public static final String SAMPLED_PROPERTY_NAME = "com.palantir.tracing.sampled";
5151

5252
@Context
@@ -72,8 +72,7 @@ public void filter(ContainerRequestContext requestContext) throws IOException {
7272
Tracer.initTrace(getObservabilityFromHeader(requestContext), Tracers.randomId());
7373
Tracer.fastStartSpan(operation, SpanType.SERVER_INCOMING);
7474
} else {
75-
// propagate the traceid from request headers, but create a new local trace id to disambiguate
76-
Tracer.initTrace(getObservabilityFromHeader(requestContext), traceId, Tracers.randomId());
75+
Tracer.initTrace(getObservabilityFromHeader(requestContext), traceId);
7776
if (spanId == null) {
7877
Tracer.fastStartSpan(operation, SpanType.SERVER_INCOMING);
7978
} else {
@@ -84,8 +83,8 @@ public void filter(ContainerRequestContext requestContext) throws IOException {
8483

8584
// Give asynchronous downstream handlers access to the trace id
8685
requestContext.setProperty(TRACE_ID_PROPERTY_NAME, Tracer.getTraceId());
87-
Tracer.getLocalTraceId().ifPresent(localId ->
88-
requestContext.setProperty(LOCAL_TRACE_ID_PROPERTY_NAME, localId));
86+
Tracer.getTopSpanId().ifPresent(localId ->
87+
requestContext.setProperty(TOP_SPAN_ID_PROPERTY_NAME, localId));
8988
requestContext.setProperty(SAMPLED_PROPERTY_NAME, Tracer.isTraceObservable());
9089
}
9190

‎tracing-jersey/src/test/java/com/palantir/tracing/jersey/TraceEnrichingFilterTest.java‎

Lines changed: 9 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -119,7 +119,7 @@ public void testTraceState_withHeaderUsesTraceId() {
119119

120120
@Test
121121
public void testTraceState_withHeaderUsesTraceIdWithDifferentLocalIds() {
122-
Response response = target.path("/local-trace").request()
122+
Response response = target.path("/top-span").request()
123123
.header(TraceHttpHeaders.TRACE_ID, "traceId")
124124
.header(TraceHttpHeaders.PARENT_SPAN_ID, "parentSpanId")
125125
.header(TraceHttpHeaders.SPAN_ID, "spanId")
@@ -128,13 +128,13 @@ public void testTraceState_withHeaderUsesTraceIdWithDifferentLocalIds() {
128128
assertThat(response.getHeaderString(TraceHttpHeaders.PARENT_SPAN_ID)).isNull();
129129
assertThat(response.getHeaderString(TraceHttpHeaders.SPAN_ID)).isNull();
130130
verify(observer).consume(spanCaptor.capture());
131-
assertThat(spanCaptor.getValue().getOperation()).isEqualTo("Jersey: GET /local-trace");
131+
assertThat(spanCaptor.getValue().getOperation()).isEqualTo("Jersey: GET /top-span");
132132

133133
String firstLocalTrace = response.readEntity(String.class);
134134
assertThat(firstLocalTrace).isNotEmpty();
135135

136136
// make the query again
137-
Response otherResponse = target.path("/local-trace").request()
137+
Response otherResponse = target.path("/top-span").request()
138138
.header(TraceHttpHeaders.TRACE_ID, "traceId")
139139
.header(TraceHttpHeaders.PARENT_SPAN_ID, "parentSpanId")
140140
.header(TraceHttpHeaders.SPAN_ID, "spanId")
@@ -185,17 +185,6 @@ public void testTraceState_withoutRequestHeadersGeneratesValidTraceResponseHeade
185185
assertThat(spanCaptor.getValue().getOperation()).isEqualTo("Jersey: GET /trace");
186186
}
187187

188-
@Test
189-
public void testTraceState_withoutRequestHeadersDoesNotGenerateLocalTrace() {
190-
Response response = target.path("/local-trace").request().get();
191-
assertThat(response.getHeaderString(TraceHttpHeaders.TRACE_ID)).isNotNull();
192-
assertThat(response.getHeaderString(TraceHttpHeaders.PARENT_SPAN_ID)).isNull();
193-
assertThat(response.getHeaderString(TraceHttpHeaders.SPAN_ID)).isNull();
194-
verify(observer).consume(spanCaptor.capture());
195-
assertThat(spanCaptor.getValue().getOperation()).isEqualTo("Jersey: GET /local-trace");
196-
assertThat(response.readEntity(String.class)).isNullOrEmpty();
197-
}
198-
199188
@Test
200189
public void testTraceState_withoutRequestHeadersGeneratesValidTraceResponseHeadersWhenFailing() {
201190
Response response = target.path("/failing-trace").request().get();
@@ -259,6 +248,8 @@ public void testFilter_setsMdcIfTraceIdHeaderIsPresent() throws Exception {
259248
TraceEnrichingFilter.INSTANCE.filter(request);
260249
assertThat(MDC.get(Tracers.TRACE_ID_KEY)).isEqualTo("traceId");
261250
verify(request).setProperty(TraceEnrichingFilter.TRACE_ID_PROPERTY_NAME, "traceId");
251+
// the top span id is randomly generated
252+
verify(request).setProperty(eq(TraceEnrichingFilter.TOP_SPAN_ID_PROPERTY_NAME), anyString());
262253
// Note: this will be set to a random value; we want to check whether the value is being set
263254
verify(request).setProperty(eq(TraceEnrichingFilter.SAMPLED_PROPERTY_NAME), anyBoolean());
264255
}
@@ -304,8 +295,8 @@ public void getFailingTraceOperation() {
304295
}
305296

306297
@Override
307-
public String getLocalTraceId() {
308-
return Tracer.getLocalTraceId().orElse(null);
298+
public String getTopSpanId() {
299+
return Tracer.getTopSpanId().get();
309300
}
310301

311302
@Override
@@ -343,8 +334,8 @@ public interface TracingTestService {
343334
void getFailingTraceOperation();
344335

345336
@GET
346-
@Path("/local-trace")
347-
String getLocalTraceId();
337+
@Path("/top-span")
338+
String getTopSpanId();
348339

349340
@GET
350341
@Path("/failing-streaming-trace")

‎tracing-undertow/src/main/java/com/palantir/tracing/undertow/TracedOperationHandler.java‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -100,8 +100,7 @@ private String initializeTrace(HttpServerExchange exchange) {
100100

101101
/** Initializes trace state given a trace-id header from the client. */
102102
private void initializeTraceFromExisting(HeaderMap headers, String traceId) {
103-
// propagate the traceid from request headers, but create a new local trace id to disambiguate
104-
Tracer.initTrace(getObservabilityFromHeader(headers), traceId, Tracers.randomId());
103+
Tracer.initTrace(getObservabilityFromHeader(headers), traceId);
105104
String spanId = headers.getFirst(SPAN_ID); // nullable
106105
if (spanId == null) {
107106
Tracer.fastStartSpan(operation, SpanType.SERVER_INCOMING);

‎tracing-undertow/src/test/java/com/palantir/tracing/undertow/TracedOperationHandlerTest.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -110,7 +110,7 @@ public void whenTraceIsInHeader_usesGivenTraceIdwithDifferentLocalIds() throws E
110110
TracedOperationHandler traceSettingHandler =
111111
new TracedOperationHandler(exc -> {
112112
traceIdValue.set(Tracer.getTraceId());
113-
localTraceIdValue.set(Tracer.getLocalTraceId().orElse(null));
113+
localTraceIdValue.set(Tracer.getTopSpanId().orElse(null));
114114

115115
}, "GET /traced");
116116
traceSettingHandler.handleRequest(exchange);

‎tracing/src/main/java/com/palantir/tracing/Trace.java‎

Lines changed: 42 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -29,7 +29,6 @@
2929
import java.util.ArrayDeque;
3030
import java.util.Deque;
3131
import java.util.Optional;
32-
import javax.annotation.Nullable;
3332

3433
/**
3534
* Represents a trace as an ordered list of non-completed spans. Supports adding and removing of spans. This class is
@@ -45,13 +44,10 @@
4544
public abstract class Trace {
4645

4746
private final String traceId;
48-
@Nullable
49-
private final String localTraceId;
5047

51-
private Trace(String traceId, @Nullable String localTraceId) {
48+
private Trace(String traceId) {
5249
checkArgument(!Strings.isNullOrEmpty(traceId), "traceId must be non-empty");
5350
this.traceId = traceId;
54-
this.localTraceId = localTraceId;
5551
}
5652

5753
/**
@@ -143,42 +139,33 @@ final String getTraceId() {
143139
/**
144140
* The globally unique non-empty identifier for this call trace within a service (or another locality context).
145141
*
146-
* While {@link #getTraceId()} is expected to be propagated across RPC calls, localTraceId distinguishes between
147-
* two concurrent RPC calls made to a service with the same traceid.
142+
* While {@link #getTraceId()} is expected to be propagated across RPC calls, the top span id distinguishes
143+
* between two concurrent RPC calls made to a service with the same traceid.
148144
*/
149-
final Optional<String> getLocalTraceId() {
150-
// XXX: should this be equal to traceId if localTraceId is not set?
151-
return Optional.ofNullable(localTraceId);
152-
}
145+
abstract Optional<String> getTopSpanId();
153146

154147
abstract Optional<String> getOriginatingSpanId();
155148

156149
/** Returns a copy of this Trace which can be independently mutated. */
157150
abstract Trace deepCopy();
158151

159152
static Trace of(boolean isObservable, String traceId) {
160-
return isObservable ? new Sampled(traceId, null) : new Unsampled(traceId, null);
161-
}
162-
163-
static Trace of(boolean isObservable, String traceId, String localTraceId) {
164-
checkArgument(!Strings.isNullOrEmpty(localTraceId), "localTraceId must be non-empty");
165-
return isObservable ? new Sampled(traceId, localTraceId) : new Unsampled(traceId, localTraceId);
153+
return isObservable ? new Sampled(traceId) : new Unsampled(traceId);
166154
}
167155

168156
private static final class Sampled extends Trace {
169157

170158
private final Deque<OpenSpan> stack;
171159

172-
private Sampled(ArrayDeque<OpenSpan> stack, String traceId, @Nullable String localTraceId) {
173-
super(traceId, localTraceId);
160+
private Sampled(ArrayDeque<OpenSpan> stack, String traceId) {
161+
super(traceId);
174162
this.stack = stack;
175163
}
176164

177-
private Sampled(String traceId, @Nullable String localTraceId) {
178-
this(new ArrayDeque<>(), traceId, localTraceId);
165+
private Sampled(String traceId) {
166+
this(new ArrayDeque<>(), traceId);
179167
}
180168

181-
182169
@Override
183170
@SuppressWarnings("ResultOfMethodCallIgnored") // Sampled traces cannot optimize this path
184171
void fastStartSpan(String operation, String parentSpanId, SpanType type) {
@@ -216,6 +203,14 @@ boolean isObservable() {
216203
return true;
217204
}
218205

206+
@Override
207+
Optional<String> getTopSpanId() {
208+
if (stack.isEmpty()) {
209+
return Optional.empty();
210+
}
211+
return Optional.of(stack.peekLast().getSpanId());
212+
}
213+
219214
@Override
220215
Optional<String> getOriginatingSpanId() {
221216
if (stack.isEmpty()) {
@@ -226,7 +221,7 @@ Optional<String> getOriginatingSpanId() {
226221

227222
@Override
228223
Trace deepCopy() {
229-
return new Sampled(new ArrayDeque<>(stack), getTraceId(), getLocalTraceId().orElse(null));
224+
return new Sampled(new ArrayDeque<>(stack), getTraceId());
230225
}
231226

232227
@Override
@@ -242,35 +237,41 @@ private static final class Unsampled extends Trace {
242237
*/
243238
private int numberOfSpans;
244239
private Optional<String> originatingSpanId = Optional.empty();
240+
private Optional<String> topSpanId = Optional.empty();
245241

246-
private Unsampled(int numberOfSpans, String traceId, @Nullable String localTraceId) {
247-
super(traceId, localTraceId);
242+
private Unsampled(int numberOfSpans, String traceId) {
243+
super(traceId);
248244
this.numberOfSpans = numberOfSpans;
249245
validateNumberOfSpans();
250246
}
251247

252-
private Unsampled(String traceId, @Nullable String localTraceId) {
253-
this(0, traceId, localTraceId);
248+
private Unsampled(String traceId) {
249+
this(0, traceId);
254250
}
255251

256252
@Override
257253
void fastStartSpan(String _operation, String parentSpanId, SpanType _type) {
258-
startSpan(Optional.of(parentSpanId));
254+
if (numberOfSpans == 0) {
255+
originatingSpanId = Optional.of(parentSpanId);
256+
topSpanId = Optional.of(Tracers.randomId());
257+
}
258+
numberOfSpans++;
259259
}
260260

261261
@Override
262262
void fastStartSpan(String _operation, SpanType _type) {
263+
if (numberOfSpans == 0) {
264+
topSpanId = Optional.of(Tracers.randomId());
265+
}
263266
numberOfSpans++;
264267
}
265268

266269
@Override
267270
protected void push(OpenSpan span) {
268-
startSpan(span.getParentSpanId());
269-
}
270-
271-
private void startSpan(Optional<String> parentSpanId) {
272271
if (numberOfSpans == 0) {
273-
originatingSpanId = parentSpanId;
272+
// TODO: shouldn't this be span.getOriginatingSpanId?
273+
originatingSpanId = span.getParentSpanId();
274+
topSpanId = Optional.of(span.getSpanId());
274275
}
275276
numberOfSpans++;
276277
}
@@ -288,6 +289,7 @@ Optional<OpenSpan> pop() {
288289
}
289290
if (numberOfSpans == 0) {
290291
originatingSpanId = Optional.empty();
292+
topSpanId = Optional.empty();
291293
}
292294
return Optional.empty();
293295
}
@@ -303,14 +305,20 @@ boolean isObservable() {
303305
return false;
304306
}
305307

308+
@Override
309+
Optional<String> getTopSpanId() {
310+
return topSpanId;
311+
}
312+
306313
@Override
307314
Optional<String> getOriginatingSpanId() {
308315
return originatingSpanId;
309316
}
310317

311318
@Override
312319
Trace deepCopy() {
313-
return new Unsampled(numberOfSpans, getTraceId(), getLocalTraceId().orElse(null));
320+
// TODO: shouldn't this preserve originatingSpanId / topSpanId?
321+
return new Unsampled(numberOfSpans, getTraceId());
314322
}
315323

316324
/** Internal validation, this should never fail because {@link #pop()} only decrements positive values. */

‎tracing/src/main/java/com/palantir/tracing/Tracer.java‎

Lines changed: 2 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -78,15 +78,6 @@ private static Trace createTrace(Observability observability, String traceId) {
7878
return Trace.of(observable, traceId);
7979
}
8080

81-
/**
82-
* Creates a new localized trace, but does not set it as the current trace.
83-
*/
84-
private static Trace createTrace(Observability observability, String traceId, String localTraceId) {
85-
checkArgument(!Strings.isNullOrEmpty(traceId), "traceId must be non-empty");
86-
boolean observable = shouldObserve(observability);
87-
return Trace.of(observable, traceId, localTraceId);
88-
}
89-
9081
private static boolean shouldObserve(Observability observability) {
9182
switch (observability) {
9283
case SAMPLE:
@@ -148,13 +139,6 @@ public static void initTrace(Observability observability, String traceId) {
148139
setTrace(createTrace(observability, traceId));
149140
}
150141

151-
/**
152-
* Initializes the current thread's trace, erasing any previously accrued open spans.
153-
*/
154-
public static void initTrace(Observability observability, String traceId, String localTraceId) {
155-
setTrace(createTrace(observability, traceId, localTraceId));
156-
}
157-
158142
/**
159143
* Opens a new span for this thread's call trace, labeled with the provided operation and parent span. Only allowed
160144
* when the current trace is empty.
@@ -500,8 +484,8 @@ public static String getTraceId() {
500484
}
501485

502486
/** Returns the globally unique identifier for this thread's trace specific to this call. */
503-
public static Optional<String> getLocalTraceId() {
504-
return checkNotNull(currentTrace.get(), "There is no trace").getLocalTraceId();
487+
public static Optional<String> getTopSpanId() {
488+
return checkNotNull(currentTrace.get(), "There is no trace").getTopSpanId();
505489
}
506490

507491
/** Clears the current trace id and returns it if present. */

0 commit comments

Comments
 (0)