TracingFilter sets the "brave.Span" attribute and reads "http.route"

This sets up the baseline for integration over `TracingFilter` via
* Setting the "brave.Span" attribute to reduce dependence on current trace context
* Reading the "http.route" attribute possibly set downstream
* Better documentation and small fixes

instrumentation/http-tests/src/main/java/brave/test/http/ITHttp.java

@@ -90,7 +90,7 @@ public abstract class ITHttp {
    * to read them on the main thread, we use a concurrent queue. As some implementations report
    * after a response is sent, we use a blocking queue to prevent race conditions in tests.
    */
-  BlockingQueue<Span> spans = new LinkedBlockingQueue<>();
+  protected BlockingQueue<Span> spans = new LinkedBlockingQueue<>();
 
   /** Call this to block until a span was reported */
   protected Span takeSpan() throws InterruptedException {
@@ -131,7 +131,7 @@ protected Span takeSpan() throws InterruptedException {
     }
   };
 
-  Tracing.Builder tracingBuilder(Sampler sampler) {
+  protected Tracing.Builder tracingBuilder(Sampler sampler) {
     return Tracing.newBuilder()
         .spanReporter(s -> {
           // make sure the context was cleared prior to finish.. no leaks!

instrumentation/http-tests/src/main/java/brave/test/http/ITHttpServer.java

@@ -316,7 +316,6 @@ public <Req> void request(HttpAdapter<Req, ?> adapter, Req req, SpanCustomizer c
 
     // verify normal tags
     assertThat(span.tags())
-        .hasSize(2)
         .containsEntry("http.method", "OPTIONS")
         .containsEntry("http.path", "/");
 

instrumentation/http-tests/src/main/java/brave/test/http/ITServlet25Container.java

@@ -4,8 +4,6 @@
 import brave.http.HttpTracing;
 import brave.propagation.ExtraFieldPropagation;
 import java.io.IOException;
-import java.util.EnumSet;
-import javax.servlet.DispatcherType;
 import javax.servlet.Filter;
 import javax.servlet.FilterChain;
 import javax.servlet.FilterConfig;
@@ -20,6 +18,7 @@
 import org.eclipse.jetty.servlet.ServletContextHandler;
 import org.eclipse.jetty.servlet.ServletHolder;
 import org.junit.Test;
+import zipkin2.Span;
 
 import static org.assertj.core.api.Assertions.assertThat;
 
@@ -64,6 +63,27 @@ protected void doGet(HttpServletRequest req, HttpServletResponse resp) throws IO
     }
   }
 
+  Filter delegate;
+
+  class DelegatingFilter implements Filter {
+
+    @Override public void init(FilterConfig filterConfig) throws ServletException {
+    }
+
+    @Override
+    public void doFilter(ServletRequest request, ServletResponse response, FilterChain chain)
+        throws IOException, ServletException {
+      if (delegate == null) {
+        chain.doFilter(request, response);
+      } else {
+        delegate.doFilter(request, response, chain);
+      }
+    }
+
+    @Override public void destroy() {
+    }
+  }
+
   // copies the header to the response
   Filter userFilter = new Filter() {
     @Override public void init(FilterConfig filterConfig) {
@@ -82,6 +102,8 @@ public void doFilter(ServletRequest request, ServletResponse response, FilterCha
   };
 
   @Test public void currentSpanVisibleToOtherFilters() throws Exception {
+    delegate = userFilter;
+
     String path = "/foo";
 
     Request request = new Request.Builder().url(url(path))
@@ -95,6 +117,65 @@ public void doFilter(ServletRequest request, ServletResponse response, FilterCha
     takeSpan();
   }
 
+  // Shows how a framework can layer on "http.route" logic
+  Filter customHttpRoute = new Filter() {
+    @Override public void init(FilterConfig filterConfig) {
+    }
+
+    @Override
+    public void doFilter(ServletRequest request, ServletResponse response, FilterChain chain)
+        throws IOException, ServletException {
+      request.setAttribute("http.route", ((HttpServletRequest) request).getRequestURI());
+      chain.doFilter(request, response);
+    }
+
+    @Override public void destroy() {
+    }
+  };
+
+  /**
+   * Shows that by adding the request attribute "http.route" a layered framework can influence
+   * any derived from the route, including the span name.
+   */
+  @Test public void canSetCustomRoute() throws Exception {
+    delegate = customHttpRoute;
+
+    get("/foo");
+
+    Span span = takeSpan();
+    assertThat(span.name())
+        .isEqualTo("get /foo");
+  }
+
+  Filter customHook = new Filter() {
+    @Override public void init(FilterConfig filterConfig) {
+    }
+
+    @Override
+    public void doFilter(ServletRequest request, ServletResponse response, FilterChain chain)
+        throws IOException, ServletException {
+      ((brave.Span) request.getAttribute("brave.Span")).tag("foo", "bar");
+      chain.doFilter(request, response);
+    }
+
+    @Override public void destroy() {
+    }
+  };
+
+  /**
+   * Shows that a framework can directly use the "brave.Span" rather than relying on the
+   * current span.
+   */
+  @Test public void canUseSpanAttribute() throws Exception {
+    delegate = customHook;
+
+    get("/foo");
+
+    Span span = takeSpan();
+    assertThat(span.tags())
+        .containsEntry("foo", "bar");
+  }
+
   @Override
   public void init(ServletContextHandler handler) {
     // add servlets for the test resource
@@ -106,14 +187,13 @@ public void init(ServletContextHandler handler) {
     handler.addServlet(new ServletHolder(new ExceptionServlet()), "/exception");
 
     // add the trace filter
-    handler.getServletContext()
-        .addFilter("tracingFilter", newTracingFilter())
-        .addMappingForUrlPatterns(EnumSet.allOf(DispatcherType.class), true, "/*");
-
-    handler.getServletContext()
-        .addFilter("userFilter", userFilter)
-        .addMappingForUrlPatterns(EnumSet.of(DispatcherType.REQUEST), true, "/*");
+    addFilter(handler, newTracingFilter());
+    // add a holder for test filters
+    addFilter(handler, new DelegatingFilter());
   }
 
   protected abstract Filter newTracingFilter();
+
+  // abstract because filter registration types were not introduced until servlet 3.0
+  protected abstract void addFilter(ServletContextHandler handler, Filter filter);
 }

instrumentation/servlet/README.md

@@ -4,53 +4,85 @@ This module contains a tracing filter for Servlet 2.5+ (including Async).
 reports Zipkin how long each request takes, along with relevant tags
 like the http url.
 
+## Configuration
 To enable tracing, you need to add the `TracingFilter`.
 
-Here's a Servlet 3+ example:
+### Servlet 3+
+When configuring, make sure this is set for all paths, all dispatcher
+types and matches after other filters. Otherwise, you may miss spans or
+leak unfinished asynchronous spans.
+
+Here's an example Servlet 3+ listener
 ```java
 public class TracingServletContextListener implements ServletContextListener {
+  Sender sender = OkHttpSender.create("http://127.0.0.1:9411/api/v2/spans");
+  AsyncReporter<Span> spanReporter = AsyncReporter.create(sender);
+  Tracing tracing = Tracing.newBuilder()
+        .localServiceName("my-service-name")
+        .spanReporter(spanReporter).build();
 
   @Override
   public void contextInitialized(ServletContextEvent servletContextEvent) {
-    Tracing tracing = Tracing.newBuilder().localServiceName("myservicename").build();
     servletContextEvent
         .getServletContext()
-        .addFilter("TracingFilter", TracingFilter.create(tracing))
+        .addFilter("tracingFilter", TracingFilter.create(tracing))
         .addMappingForUrlPatterns(EnumSet.allOf(DispatcherType.class), true, "/*");
   }
 
   @Override
   public void contextDestroyed(ServletContextEvent servletContextEvent) {
+    try {
+      tracing.close(); // disables Tracing.current()
+      spanReporter.close(); // stops reporting thread and flushes data
+      sender.close(); // closes any transport resources
+    } catch (IOException e) {
+      // do something real
+    }
   }
 }
 ```
 
+### Servlet 2.5
+
 In Servlet 2.5, there's no `ServletContextListener`. Besides using tools
-like Spring or Guice, you can make a decorating `javax.servlet.Filter`
+like Spring or Guice, you can make a delegating `javax.servlet.Filter`
 that configures tracing, and add that to your web.xml.
+```xml
+  <filter>
+    <filter-name>tracingFilter</filter-name>
+    <filter-class>com.myco.DelegatingTracingFilter</filter-class>
+  </filter>
+
+  <filter-mapping>
+    <filter-name>tracingFilter</filter-name>
+    <url-pattern>/*</url-pattern>
+  </filter-mapping>
+```
+
+Here's an example implementation:
 ```java
-public class ServletContextTracingFilter implements Filter {
+public class DelegatingTracingFilter implements Filter {
+  Sender sender = OkHttpSender.create("http://127.0.0.1:9411/api/v2/spans");
+  AsyncReporter<Span> spanReporter = AsyncReporter.create(sender);
+  Tracing tracing = Tracing.newBuilder()
+        .localServiceName("my-service-name")
+        .spanReporter(spanReporter).build();
+  Filter delegate = TracingFilter.create(tracing);
 
   @Override
   public void doFilter(ServletRequest request, ServletResponse response, FilterChain chain)
       throws IOException, ServletException {
-    Filter currentTracingFilter =
-        (Filter) request.getServletContext().getAttribute(TracingFilter.class.getName());
-    if (currentTracingFilter == null) {
-      chain.doFilter(request, response);
-    } else {
-      currentTracingFilter.doFilter(request, response, chain);
-    }
-  }
-
-  @Override
-  public void init(FilterConfig filterConfig) {
-    Tracing tracing = Tracing.newBuilder().localServiceName("myservicename").build();
-    Filter tracingFilter = TracingFilter.create(tracing);
-    filterConfig.getServletContext().setAttribute(TracingFilter.class.getName(), tracingFilter);
+    delegate.doFilter(request, response, chain);
   }
 
   @Override public void destroy() {
+    try {
+      tracing.close(); // disables Tracing.current()
+      spanReporter.close(); // stops reporting thread and flushes data
+      sender.close(); // closes any transport resources
+    } catch (IOException e) {
+      // do something real
+    }
   }
 }
 ```

instrumentation/servlet/src/it/servlet25/src/test/java/brave/servlet25/ITTracingFilter.java

@@ -1,12 +1,20 @@
 package brave.servlet25;
 
+import brave.test.http.ITServlet25Container;
 import brave.servlet.TracingFilter;
+import java.util.EnumSet;
 import javax.servlet.Filter;
-import brave.test.http.ITServlet25Container;
+import org.eclipse.jetty.server.DispatcherType;
+import org.eclipse.jetty.servlet.FilterHolder;
+import org.eclipse.jetty.servlet.ServletContextHandler;
 
 public class ITTracingFilter extends ITServlet25Container {
 
   @Override protected Filter newTracingFilter() {
     return TracingFilter.create(httpTracing);
   }
+
+  @Override protected void addFilter(ServletContextHandler handler, Filter filter) {
+    handler.addFilter(new FilterHolder(filter), "/*", EnumSet.allOf(DispatcherType.class));
+  }
 }

instrumentation/servlet/src/main/java/brave/servlet/HttpServletAdapter.java

@@ -3,11 +3,25 @@
 import brave.http.HttpServerAdapter;
 import javax.servlet.http.HttpServletRequest;
 import javax.servlet.http.HttpServletResponse;
+import javax.servlet.http.HttpServletResponseWrapper;
 import zipkin2.Endpoint;
 
 /** This can also parse the remote IP of the client. */
 // public for others like sparkjava to use
 public class HttpServletAdapter extends HttpServerAdapter<HttpServletRequest, HttpServletResponse> {
+
+  /**
+   * Looks for the {@link HttpServletRequest#setAttribute(String, Object) request attribute}
+   * "http.route". When present, returns a response wrapper that this adapter can use to parse it.
+   */
+  // not static so that this can be overridden by implementations as needed.
+  public HttpServletResponse adaptResponse(HttpServletRequest req, HttpServletResponse resp) {
+    String httpRoute = (String) req.getAttribute("http.route");
+    return httpRoute != null
+        ? new DecoratedHttpServletResponse(resp, req.getMethod(), httpRoute)
+        : resp;
+  }
+
   final ServletRuntime servlet = ServletRuntime.get();
 
   /**
@@ -42,7 +56,39 @@ public class HttpServletAdapter extends HttpServerAdapter<HttpServletRequest, Ht
     return request.getHeader(name);
   }
 
+  /**
+   * When applied to {@link #adaptResponse(HttpServletRequest, HttpServletResponse)}, returns the
+   * {@link HttpServletRequest#getMethod() request method}.
+   */
+  @Override public String methodFromResponse(HttpServletResponse response) {
+    if (response instanceof DecoratedHttpServletResponse) {
+      return ((DecoratedHttpServletResponse) response).method;
+    }
+    return null;
+  }
+
+  /**
+   * When applied to {@link #adaptResponse(HttpServletRequest, HttpServletResponse)}, returns the
+   * {@link HttpServletRequest#getAttribute(String) request attribute} "http.route".
+   */
+  @Override public String route(HttpServletResponse response) {
+    if (response instanceof DecoratedHttpServletResponse) {
+      return ((DecoratedHttpServletResponse) response).httpRoute;
+    }
+    return null;
+  }
+
   @Override public Integer statusCode(HttpServletResponse response) {
     return servlet.status(response);
   }
+
+  static class DecoratedHttpServletResponse extends HttpServletResponseWrapper {
+    final String method, httpRoute;
+
+    DecoratedHttpServletResponse(HttpServletResponse response, String method, String httpRoute) {
+      super(response);
+      this.method = method;
+      this.httpRoute = httpRoute;
+    }
+  }
 }