Skip to content
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

Fix #177: Add APIs for Supplier<String> operation names #262

Closed
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions changelog/@unreleased/pr-262.v2.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
type: improvement
improvement:
description: Add Supplier<String> operation name APIs for lazy operation name evaluation
links:
- https://github.com/palantir/tracing-java/pull/262
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
import com.palantir.tracing.api.TraceHttpHeaders;
import java.io.IOException;
import java.util.Optional;
import java.util.function.Supplier;
import javax.annotation.Priority;
import javax.ws.rs.container.ContainerRequestContext;
import javax.ws.rs.container.ContainerRequestFilter;
Expand Down Expand Up @@ -55,12 +56,10 @@ public final class TraceEnrichingFilter implements ContainerRequestFilter, Conta
// Handles incoming request
@Override
public void filter(ContainerRequestContext requestContext) throws IOException {
String path = Optional.ofNullable(uriInfo)
Supplier<String> operation = () -> "Jersey: " + requestContext.getMethod() + " " + Optional.ofNullable(uriInfo)
.map(ExtendedUriInfo::getMatchedModelResource)
.map(Resource::getPath)
.orElse("(unknown)");

String operation = "Jersey: " + requestContext.getMethod() + " " + path;
// The following strings are all nullable
String traceId = requestContext.getHeaderString(TraceHttpHeaders.TRACE_ID);
String spanId = requestContext.getHeaderString(TraceHttpHeaders.SPAN_ID);
Expand Down
19 changes: 19 additions & 0 deletions tracing/src/main/java/com/palantir/tracing/CloseableTracer.java
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
package com.palantir.tracing;

import com.palantir.tracing.api.SpanType;
import java.util.function.Supplier;

/**
* Wraps the {@link Tracer} methods in a closeable resource to enable the usage of the try-with-resources pattern.
Expand All @@ -39,6 +40,13 @@ public static CloseableTracer startSpan(String operation) {
return startSpan(operation, SpanType.LOCAL);
}

/**
* Opens a new {@link SpanType#LOCAL LOCAL} span for this thread's call trace, labeled with the provided operation.
*/
public static CloseableTracer startSpan(Supplier<String> operation) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm worried that clients might choose to use this instead of the normal one for plain string concatenations which are not that expensive, thereby potentially slowing down hot paths.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Today hot paths are slow depending on the length of the http path.

return startSpan(operation, SpanType.LOCAL);
}

/**
* Opens a new span for this thread's call trace with the provided {@link SpanType},
* labeled with the provided operation.
Expand All @@ -50,6 +58,17 @@ public static CloseableTracer startSpan(String operation, SpanType spanType) {
return INSTANCE;
}

/**
* Opens a new span for this thread's call trace with the provided {@link SpanType},
* labeled with the provided operation.
*
* If you need to a span that may complete on another thread, use {@link DetachedSpan#start} instead.
*/
public static CloseableTracer startSpan(Supplier<String> operation, SpanType spanType) {
Tracer.fastStartSpan(operation, spanType);
return INSTANCE;
}

@Override
public void close() {
Tracer.fastCompleteSpan();
Expand Down
46 changes: 46 additions & 0 deletions tracing/src/main/java/com/palantir/tracing/DetachedSpan.java
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@

import com.google.errorprone.annotations.MustBeClosed;
import com.palantir.tracing.api.SpanType;
import java.util.function.Supplier;
import javax.annotation.CheckReturnValue;

/** Span which is not bound to thread state, and can be completed on any other thread. */
Expand All @@ -38,6 +39,12 @@ static DetachedSpan start(String operation) {
return start(operation, SpanType.LOCAL);
}

/** Equivalent to {@link #start(String)} with a lazy supplier for the operation name. */
@CheckReturnValue
static DetachedSpan start(Supplier<String> operation) {
return start(operation, SpanType.LOCAL);
}

/**
* Marks the beginning of a span, which you can {@link #complete} on any other thread.
*
Expand All @@ -48,13 +55,26 @@ static DetachedSpan start(String operation, SpanType type) {
return Tracer.detachInternal(operation, type);
}

/** Equivalent to {@link #start(String, SpanType)} with a lazy supplier for the operation name. */
@CheckReturnValue
static DetachedSpan start(Supplier<String> operation, SpanType type) {
return Tracer.detachInternal(operation, type);
}

/**
* Equivalent to {@link Tracer#startSpan(String, SpanType)}, but using this {@link DetachedSpan}
* as the parent instead of thread state.
*/
@MustBeClosed
CloseableSpan childSpan(String operationName, SpanType type);

/**
* Equivalent to {@link Tracer#fastStartSpan(Supplier, SpanType)}, but using this {@link DetachedSpan}
* as the parent instead of thread state.
*/
@MustBeClosed
CloseableSpan childSpan(Supplier<String> operationName, SpanType type);

/**
* Equivalent to {@link Tracer#startSpan(String)}, but using this {@link DetachedSpan} as the parent instead
* of thread state.
Expand All @@ -72,15 +92,32 @@ default CloseableSpan completeAndStartChild(String operationName, SpanType type)
return child;
}

@MustBeClosed
@SuppressWarnings("MustBeClosedChecker")
default CloseableSpan completeAndStartChild(Supplier<String> operationName, SpanType type) {
CloseableSpan child = childSpan(operationName, type);
complete();
return child;
}

@MustBeClosed
default CloseableSpan completeAndStartChild(String operationName) {
return completeAndStartChild(operationName, SpanType.LOCAL);
}

@MustBeClosed
default CloseableSpan completeAndStartChild(Supplier<String> operationName) {
return completeAndStartChild(operationName, SpanType.LOCAL);
}

/** Starts a child {@link DetachedSpan} using this instance as the parent. */
@CheckReturnValue
DetachedSpan childDetachedSpan(String operation, SpanType type);

/** Starts a child {@link DetachedSpan} using this instance as the parent. */
@CheckReturnValue
DetachedSpan childDetachedSpan(Supplier<String> operation, SpanType type);

/**
* Starts a child {@link DetachedSpan} using this instance as the parent.
* Equivalent to {@link #childDetachedSpan(String, SpanType)} using {@link SpanType#LOCAL}.
Expand All @@ -90,6 +127,15 @@ default DetachedSpan childDetachedSpan(String operation) {
return childDetachedSpan(operation, SpanType.LOCAL);
}

/**
* Starts a child {@link DetachedSpan} using this instance as the parent.
* Equivalent to {@link #childDetachedSpan(Supplier, SpanType)} using {@link SpanType#LOCAL}.
*/
@CheckReturnValue
default DetachedSpan childDetachedSpan(Supplier<String> operation) {
return childDetachedSpan(operation, SpanType.LOCAL);
}

/**
* Completes this span. After complete is invoked, other methods are not expected to produce spans, but
* they must not throw either in order to avoid confusing failures.
Expand Down
31 changes: 31 additions & 0 deletions tracing/src/main/java/com/palantir/tracing/Trace.java
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@
import java.util.ArrayDeque;
import java.util.Deque;
import java.util.Optional;
import java.util.function.Supplier;

/**
* Represents a trace as an ordered list of non-completed spans. Supports adding and removing of spans. This class is
Expand Down Expand Up @@ -115,6 +116,16 @@ private static <T> Optional<T> orElse(Optional<T> left, Optional<T> right) {
*/
abstract void fastStartSpan(String operation, SpanType type);

/**
* Like {@link #startSpan(String, String, SpanType)}, but does not return an {@link OpenSpan}.
*/
abstract void fastStartSpan(Supplier<String> operation, String parentSpanId, SpanType type);

/**
* Like {@link #startSpan(String, SpanType)}, but does not return an {@link OpenSpan}.
*/
abstract void fastStartSpan(Supplier<String> operation, SpanType type);

protected abstract void push(OpenSpan span);

abstract Optional<OpenSpan> top();
Expand Down Expand Up @@ -170,6 +181,16 @@ void fastStartSpan(String operation, SpanType type) {
startSpan(operation, type);
}

@Override
void fastStartSpan(Supplier<String> operation, String parentSpanId, SpanType type) {
fastStartSpan(operation.get(), parentSpanId, type);
}

@Override
void fastStartSpan(Supplier<String> operation, SpanType type) {
fastStartSpan(operation.get(), type);
}

@Override
protected void push(OpenSpan span) {
stack.push(span);
Expand Down Expand Up @@ -242,6 +263,16 @@ void fastStartSpan(String operation, SpanType type) {
numberOfSpans++;
}

@Override
void fastStartSpan(Supplier<String> operation, String parentSpanId, SpanType type) {
numberOfSpans++;
}

@Override
void fastStartSpan(Supplier<String> operation, SpanType type) {
numberOfSpans++;
}

@Override
protected void push(OpenSpan span) {
startSpan(span.getParentSpanId());
Expand Down
57 changes: 57 additions & 0 deletions tracing/src/main/java/com/palantir/tracing/Tracer.java
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@
import java.util.Optional;
import java.util.concurrent.atomic.AtomicBoolean;
import java.util.function.Consumer;
import java.util.function.Supplier;
import javax.annotation.Nullable;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
Expand Down Expand Up @@ -188,6 +189,27 @@ public static void fastStartSpan(String operation) {
fastStartSpan(operation, SpanType.LOCAL);
}

/**
* Like {@link #startSpan(String, String, SpanType)}, but does not return an {@link OpenSpan}.
*/
public static void fastStartSpan(Supplier<String> operation, String parentSpanId, SpanType type) {
getOrCreateCurrentTrace().fastStartSpan(operation, parentSpanId, type);
}

/**
* Like {@link #startSpan(String, SpanType)}, but does not return an {@link OpenSpan}.
*/
public static void fastStartSpan(Supplier<String> operation, SpanType type) {
getOrCreateCurrentTrace().fastStartSpan(operation, type);
}

/**
* Like {@link #startSpan(String)}, but does not return an {@link OpenSpan}.
*/
public static void fastStartSpan(Supplier<String> operation) {
fastStartSpan(operation, SpanType.LOCAL);
}

/**
* Like {@link #startSpan(String, SpanType)}, but does not set or modify tracing thread state.
* This is an internal utility that should not be called directly outside of {@link DetachedSpan}.
Expand All @@ -203,6 +225,17 @@ static DetachedSpan detachInternal(String operation, SpanType type) {
: new UnsampledDetachedSpan(traceId);
}

static DetachedSpan detachInternal(Supplier<String> operation, SpanType type) {
Trace maybeCurrentTrace = currentTrace.get();
String traceId = maybeCurrentTrace != null
? maybeCurrentTrace.getTraceId() : Tracers.randomId();
boolean sampled = maybeCurrentTrace != null
? maybeCurrentTrace.isObservable() : sampler.sample();
return sampled
? new SampledDetachedSpan(operation.get(), type, traceId, getParentSpanId(maybeCurrentTrace))
: new UnsampledDetachedSpan(traceId);
}

private static Optional<String> getParentSpanId(@Nullable Trace trace) {
if (trace != null) {
Optional<OpenSpan> maybeOpenSpan = trace.top();
Expand Down Expand Up @@ -240,12 +273,23 @@ public CloseableSpan childSpan(String operationName, SpanType type) {
return TraceRestoringCloseableSpan.of(maybeCurrentTrace);
}

@Override
@MustBeClosed
public CloseableSpan childSpan(Supplier<String> operationName, SpanType type) {
return childSpan(operationName.get(), type);
}

@Override
public DetachedSpan childDetachedSpan(String operation, SpanType type) {
warnIfCompleted("startDetachedSpan");
return new SampledDetachedSpan(operation, type, traceId, Optional.of(openSpan.getSpanId()));
}

@Override
public DetachedSpan childDetachedSpan(Supplier<String> operation, SpanType type) {
return childDetachedSpan(operation.get(), type);
}

@Override
public void complete() {
if (completed.compareAndSet(false, true)) {
Expand Down Expand Up @@ -284,11 +328,24 @@ public CloseableSpan childSpan(String operationName, SpanType type) {
return TraceRestoringCloseableSpan.of(maybeCurrentTrace);
}

@Override
public CloseableSpan childSpan(Supplier<String> operationName, SpanType type) {
Trace maybeCurrentTrace = currentTrace.get();
setTrace(Trace.of(false, traceId));
Tracer.fastStartSpan(operationName, type);
return TraceRestoringCloseableSpan.of(maybeCurrentTrace);
}

@Override
public DetachedSpan childDetachedSpan(String operation, SpanType type) {
return this;
}

@Override
public DetachedSpan childDetachedSpan(Supplier<String> operation, SpanType type) {
return this;
}

@Override
public void complete() {
// nop
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,16 @@ public void startsAndClosesSpan() {
assertThat(Tracer.getAndClearTrace().top()).isEmpty();
}

@Test
public void startsAndClosesSpan_supplier() {
try (CloseableTracer tracer = CloseableTracer.startSpan(() -> "foo")) {
OpenSpan openSpan = Tracer.copyTrace().get().top().get();
assertThat(openSpan.getOperation()).isEqualTo("foo");
assertThat(openSpan.type()).isEqualTo(SpanType.LOCAL);
}
assertThat(Tracer.getAndClearTrace().top()).isEmpty();
}

@Test
public void startsAndClosesSpanWithType() {
try (CloseableTracer tracer = CloseableTracer.startSpan("foo", SpanType.CLIENT_OUTGOING)) {
Expand All @@ -53,4 +63,14 @@ public void startsAndClosesSpanWithType() {
}
assertThat(Tracer.getAndClearTrace().top()).isEmpty();
}

@Test
public void startsAndClosesSpanWithType_supplier() {
try (CloseableTracer tracer = CloseableTracer.startSpan(() -> "foo", SpanType.CLIENT_OUTGOING)) {
OpenSpan openSpan = Tracer.copyTrace().get().top().get();
assertThat(openSpan.getOperation()).isEqualTo("foo");
assertThat(openSpan.type()).isEqualTo(SpanType.CLIENT_OUTGOING);
}
assertThat(Tracer.getAndClearTrace().top()).isEmpty();
}
}
10 changes: 10 additions & 0 deletions tracing/src/test/java/com/palantir/tracing/TracerTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -270,6 +270,16 @@ public void testFastCompleteSpan() {
assertThat(spanCaptor.getValue().getOperation()).isEqualTo(operation);
}

@Test
public void testFastCompleteSpan_supplier() {
Tracer.subscribe("1", observer1);
String operation = "operation";
Tracer.fastStartSpan(() -> operation);
Tracer.fastCompleteSpan();
verify(observer1).consume(spanCaptor.capture());
assertThat(spanCaptor.getValue().getOperation()).isEqualTo(operation);
}

@Test
public void testFastCompleteSpanWithMetadata() {
Tracer.subscribe("1", observer1);
Expand Down