Skip to content

Commit

Permalink
Adds HttpAdapter.template in support of "http.template" tag
Browse files Browse the repository at this point in the history
  • Loading branch information
Adrian Cole committed Feb 12, 2018
1 parent 9cebbb5 commit b235c51
Show file tree
Hide file tree
Showing 10 changed files with 126 additions and 13 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,9 @@
import brave.SpanCustomizer;
import brave.propagation.ExtraFieldPropagation;
import brave.sampler.Sampler;
import java.util.Arrays;
import java.util.LinkedHashSet;
import java.util.Set;
import okhttp3.OkHttpClient;
import okhttp3.Request;
import okhttp3.Response;
Expand Down Expand Up @@ -212,6 +215,32 @@ public <Req> void request(HttpAdapter<Req, ?> adapter, Req req, SpanCustomizer c
.containsEntry("context.visible", "true");
}

@Test
public void supportsHttpTemplate() throws Exception {
httpTracing = httpTracing.toBuilder().serverParser(new HttpServerParser() {
@Override public <Resp> void response(HttpAdapter<?, Resp> adapter, Resp res, Throwable error,
SpanCustomizer customizer) {
super.response(adapter, res, error, customizer);
String template = adapter.template(res);
if (template != null) customizer.name(template);
}
}).build();
init();

get("/items/1?foo");
get("/items/2?bar");

Span span1 = takeSpan(), span2 = takeSpan();
assertThat(Arrays.asList(span1.tags().get("http.path"), span2.tags().get("http.path")))
.containsExactly("/items/1", "/items/2");

Set<String> templates = new LinkedHashSet<>(Arrays.asList(span1.name(), span2.name()));

assertThat(templates).hasSize(1);
assertThat(templates.iterator().next())
.contains("items");
}

@Test
public void addsStatusCode_badRequest() throws Exception {
try {
Expand Down
20 changes: 20 additions & 0 deletions instrumentation/http/src/main/java/brave/http/HttpAdapter.java
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,26 @@ public abstract class HttpAdapter<Req, Resp> {
*/
@Nullable public abstract String requestHeader(Req request, String name);

/**
* An expression representing an application endpoint, used to group similar requests together.
*
* <p>For example, the template "/products/{key}", would match "/products/1" and "/products/2".
* There is no format required for the encoding, as it is sometimes application defined. The
* important part is that the value namespace is low cardinality.
*
* <p>Conventionally associated with the key "http.template"
*
* <p>Eventhough the template is associated with the request, not the response, this is present
* on the response object. The reasons is that many server implementations process the request
* before they can identify the route route.
*/
// BRAVE5: It isn't possible for a user to easily consume HttpServerAdapter, which is why this
// method, while generally about the server, is pushed up to the HttpAdapter. The signatures for
// sampling and parsing could be changed to make it more convenient.
@Nullable public String template(Resp response) {
return null;
}

/**
* The HTTP status code or null if unreadable.
*
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,11 @@ public final class ContainerAdapter
return request.getHeaderString(name);
}

@Override public String template(ContainerResponseContext response) {
// There's no portable means to get the template eventhough there is a way in jersey2
return null;
}

@Override public Integer statusCode(ContainerResponseContext response) {
return response.getStatus();
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,7 @@

/** This can also parse the remote IP of the client. */
// public for others like sparkjava to use
public final class HttpServletAdapter
extends HttpServerAdapter<HttpServletRequest, HttpServletResponse> {
public class HttpServletAdapter extends HttpServerAdapter<HttpServletRequest, HttpServletResponse> {
final ServletRuntime servlet = ServletRuntime.get();

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,21 +22,20 @@ public static AsyncHandlerInterceptor create(HttpTracing httpTracing) {
return new TracingAsyncHandlerInterceptor(httpTracing);
}

final HandlerInterceptor delegate;
final TracingHandlerInterceptor delegate;

@Autowired TracingAsyncHandlerInterceptor(HttpTracing httpTracing) { // internal
delegate = TracingHandlerInterceptor.create(httpTracing);
delegate = new TracingHandlerInterceptor(httpTracing);
}

@Override
public boolean preHandle(HttpServletRequest request, HttpServletResponse response, Object o)
throws Exception {
public boolean preHandle(HttpServletRequest request, HttpServletResponse response, Object o) {
return delegate.preHandle(request, response, o);
}

@Override
public void afterCompletion(HttpServletRequest request, HttpServletResponse response,
Object o, Exception ex) throws Exception {
public void afterCompletion(HttpServletRequest request, HttpServletResponse response, Object o,
Exception ex) {
delegate.afterCompletion(request, response, o, ex);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,9 @@

/** Tracing interceptor for Spring Web MVC {@link HandlerInterceptor}. */
public final class TracingHandlerInterceptor implements HandlerInterceptor {
// redefined from HandlerMapping.BEST_MATCHING_PATTERN_ATTRIBUTE as doesn't exist until Spring 3
static final String BEST_MATCHING_PATTERN_ATTRIBUTE =
"org.springframework.web.servlet.HandlerMapping.bestMatchingPattern";
static final Propagation.Getter<HttpServletRequest, String> GETTER =
new Propagation.Getter<HttpServletRequest, String>() {
@Override public String get(HttpServletRequest carrier, String key) {
Expand All @@ -37,12 +40,23 @@ public static HandlerInterceptor create(HttpTracing httpTracing) {
}

final Tracer tracer;
final ThreadLocal<Object> currentTemplate;
final HttpServerHandler<HttpServletRequest, HttpServletResponse> handler;
final TraceContext.Extractor<HttpServletRequest> extractor;

@Autowired TracingHandlerInterceptor(HttpTracing httpTracing) { // internal
tracer = httpTracing.tracing().tracer();
handler = HttpServerHandler.create(httpTracing, new HttpServletAdapter());
currentTemplate = new ThreadLocal<>();
handler = HttpServerHandler.create(httpTracing, new HttpServletAdapter() {
@Override public String template(HttpServletResponse response) {
Object result = currentTemplate.get();
return result != null ? result.toString() : null;
}

@Override public String toString() {
return "WebMVCAdapter{}";
}
});
extractor = httpTracing.tracing().propagation().extractor(GETTER);
}

Expand All @@ -59,7 +73,7 @@ public boolean preHandle(HttpServletRequest request, HttpServletResponse respons

@Override
public void postHandle(HttpServletRequest request, HttpServletResponse response, Object handler,
ModelAndView modelAndView) throws Exception {
ModelAndView modelAndView) {
}

@Override
Expand All @@ -68,6 +82,16 @@ public void afterCompletion(HttpServletRequest request, HttpServletResponse resp
Span span = tracer.currentSpan();
if (span == null) return;
((SpanInScope) request.getAttribute(SpanInScope.class.getName())).close();
handler.handleSend(response, ex, span);
Object template = request.getAttribute(BEST_MATCHING_PATTERN_ATTRIBUTE);
if (template == null) { // skip thread-local overhead if there's no attribute
handler.handleSend(response, ex, span);
return;
}
try {
currentTemplate.set(template);
handler.handleSend(response, ex, span);
} finally {
currentTemplate.remove();
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
import org.springframework.http.HttpStatus;
import org.springframework.http.ResponseEntity;
import org.springframework.stereotype.Controller;
import org.springframework.web.bind.annotation.PathVariable;
import org.springframework.web.bind.annotation.RequestMapping;
import org.springframework.web.context.support.AnnotationConfigWebApplicationContext;
import org.springframework.web.servlet.AsyncHandlerInterceptor;
Expand Down Expand Up @@ -69,6 +70,11 @@ public Callable<ResponseEntity<Void>> disconnectAsync() {
throw new IOException();
};
}

@RequestMapping(value = "/items/{itemId}")
public ResponseEntity<Void> items(@PathVariable String itemId) throws IOException {
return new ResponseEntity<>(HttpStatus.OK);
}
}

@Configuration
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
import org.springframework.http.HttpStatus;
import org.springframework.http.ResponseEntity;
import org.springframework.stereotype.Controller;
import org.springframework.web.bind.annotation.PathVariable;
import org.springframework.web.bind.annotation.RequestMapping;
import org.springframework.web.context.support.AnnotationConfigWebApplicationContext;
import org.springframework.web.servlet.DispatcherServlet;
Expand Down Expand Up @@ -69,6 +70,11 @@ public Callable<ResponseEntity<Void>> disconnectAsync() throws IOException {
throw new IOException();
};
}

@RequestMapping(value = "/items/{itemId}")
public ResponseEntity<Void> items(@PathVariable String itemId) throws IOException {
return new ResponseEntity<>(HttpStatus.OK);
}
}

@Configuration
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -36,12 +36,14 @@ final class TracingRoutingContextHandler implements Handler<RoutingContext> {
};

final Tracer tracer;
final ThreadLocal<String> currentTemplate;
final HttpServerHandler<HttpServerRequest, HttpServerResponse> serverHandler;
final TraceContext.Extractor<HttpServerRequest> extractor;

TracingRoutingContextHandler(HttpTracing httpTracing) {
tracer = httpTracing.tracing().tracer();
serverHandler = HttpServerHandler.create(httpTracing, new Adapter());
currentTemplate = new ThreadLocal<>();
serverHandler = HttpServerHandler.create(httpTracing, new Adapter(currentTemplate));
extractor = httpTracing.tracing().propagation().extractor(GETTER);
}

Expand Down Expand Up @@ -80,11 +82,27 @@ class TracingHandler implements Handler<Void> {

@Override public void handle(Void aVoid) {
if (!context.request().isEnded()) return;
serverHandler.handleSend(context.response(), context.failure(), span);
String template = context.currentRoute().getPath();
if (template == null) { // skip thread-local overhead if there's no attribute
serverHandler.handleSend(context.response(), context.failure(), span);
return;
}
try {
currentTemplate.set(template);
serverHandler.handleSend(context.response(), context.failure(), span);
} finally {
currentTemplate.remove();
}
}
}

static final class Adapter extends HttpServerAdapter<HttpServerRequest, HttpServerResponse> {
final ThreadLocal<String> currentTemplate;

Adapter(ThreadLocal<String> currentTemplate) {
this.currentTemplate = currentTemplate;
}

@Override public String method(HttpServerRequest request) {
return request.method().name();
}
Expand All @@ -101,6 +119,10 @@ static final class Adapter extends HttpServerAdapter<HttpServerRequest, HttpServ
return request.headers().get(name);
}

@Override public String template(HttpServerResponse response) {
return currentTemplate.get();
}

@Override public Integer statusCode(HttpServerResponse response) {
return response.getStatusCode();
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,9 @@ public class ITVertxWebTracing extends ITHttpServer {
router.route("/exception").handler(ctx -> {
ctx.fail(new Exception());
});
router.route("/items/:itemId").handler(ctx -> {
ctx.response().end("bar");
});
router.route("/exceptionAsync").handler(ctx -> {
ctx.request().endHandler(v -> ctx.fail(new Exception()));
});
Expand Down

0 comments on commit b235c51

Please sign in to comment.