Make BraveServletFilter customizable

[remmeier]

I started including brave in our projects and works great. However, we faced some issues when trying to customize the behavior a little bit and ended up copy pasting the complete implementation.

BraveServletFilter.doFilter is to large, the creation of ServletHttpServerRequest and HttpServerRequestAdapter should be moved into a dedicated method that would allow subclassing and modifying it. Or maybe even better would be to be able to set a factory. We modified HttpServerResponseAdapter to provide more than just the TraceKeys.HTTP_STATUS_CODE.

I can do a PR for this.

[codefromthecrypt]

I've noticed some limitations, too, but I'd be careful about anchoring too much on the way it currently works, or exposing things that we'd end up needing to support later.

For example, I made OkHttp's completely independent of the abstraction as an interim (because it is hard to tell what is actually portable without multiple implementations). https://github.com/openzipkin/brave/blob/master/brave-okhttp/src/main/java/com/github/kristofa/brave/okhttp/BraveTracingInterceptor.java

Maybe you can share your own version for now? and show which frameworks it works with (specifically which use cases it aims to support?)

[remmeier]

regarding use cases:

• I added a "getPrincpialName" to a custom HttpServerRequest
• a custom ServerRequestAdapter adds some further binary annotations: -- the principal -- values from proprietary request headers (like client id, client version) -- some metrics collected during the execution that are not traced by themselves

Not quite sure where to place those. Implementing ServerRequestAdapter seemed the best thing at first. But as noted above there is no easy way to add them without copy/pasing the implementation.

I could also imagine wrapping ServerRequestInterceptor and ServerResponseInterceptor and apply my changes there. But currently those classes not interfaces and are to replace as well. The advantage here is that it is a global thing. As long as those interceptors are used by the different filters (okhttp, servlet, etc.), I can reconfigure brave in one location.

yes, I started using okhttp as well. Works great. I think it would be good if it follows the http servlet and makes use of the ServerRequestAdapter/ServerResponseAdapter and forwards the data to ServerRequestInterceptor and ServerResponseInterceptor.

I started a PR. I think a ServerAdapterFactory in brave-core could be a good next step instead of having to use subclassing.

[codefromthecrypt]

Best way to figure out if the design is generally helpful or the right approach is to ask others to comment. Remember, others have been using this filter for a while, and some may have made their own for various reasons. Some have gone to different directions. You can use their experience, and it is also nicer to the project to change public types with more than one person in mind.

One way to discover others is to join gitter or look at other issues in this repo or other repos of similar nature and CC those asking similar things if this could help. https://gitter.im/openzipkin/zipkin https://github.com/search?q=BraveServletFilter

here's a few to start: @devinsba @llinder @NegatioN @schlosna

---

The current proposal in #265 is to add the ability to control HttpServerRequestAdapter and HttpServerResponseAdapter via extension or a factory variant, to BraveServletFilter and possibly/probably others. The desire is to use a single class across different instrumentation, possibly later adding a means to get the "raw" type for the library-specific request or response object.

In doing something similar in OkHttp, I noticed that these types aren't written for extension. For this reason, I asked to prove that multiple integrations could work like this before changing the code. For example, it would be a problem to end up with a partial implementation or a flurry of PRs considering we do semantic versioning and cannot roll-back change easily.

Has anyone managed to customize to use a single type to control binary annotation values such as the servlet principle across frameworks? If so, how? If not, have you resorted to library-specific code like OkHttpParser? What designs are you using and what are the pros/cons are you seeing?

[devinsba]

We ended up writing our own servlet filter pretty much for our RPC framework. We use AspectJ on all of our deployments so we wrote our filter as an aspect around the javax.servlet.http.HttpServlet methods. We then have a framework specific Aspect that catches traces on their way through it and adds our framework specific annotations.

We could use a servlet filter that was configurable/extendable to cut down on the amount of custom code we have. We would still need aspects though for some of our stuff so it doesn't buy us a lot on that front.

[codefromthecrypt]

@devinsba thanks for sharing! Is the code that adds annotations custom to an open source framework or an internal one? If shareable, can you send a link? Regardless are you only needing to intercept Servlet, or are there other http library types you need to hook into.

[llinder]

Currently our customizations are mostly around filtering requests to exclude things we don't care about. We could have probably achieved the desired results by using the Brave TraceFilter API for this instead.

Providing custom request/response annotations to every request/response is definitely something that we would find beneficial as well. In fact I just started looking into this last week and will probably go down the route of using custom HttpServerRequestAdapter/HttpServerResponseAdapter.

My preference on API changes are biased towards a builder pattern where factories are provided to the builder rather than extending classes and overriding protected methods. That said I don't have any concrete code to share for discussion at this time.

[devinsba]

@adriancole sorry for the slow response. Came down sick.

Its an internal framework. I can probably paste snippets of the instrumentation though. Our framework is built around the base Servelet methods on the server and UrlConnection for the client.

For our non-old-rpc apps (spring or spring-boot) we planned on using the brave classes that already exist or spring-cloud-sleuth. I've only really had success pushing adoption into the older apps so far though so I haven't run into any road blocks with those.

Grails might be the only major outlier for us but I'm not sure that falls under this issue.

Edit, heres a pared down gist, let me know if you want some more context https://gist.github.com/devinsba/ca0a8bc0f05746e6e900efa6fbe60b85

[codefromthecrypt]

It is confusing and time consuming to work on the same design in different places. I've closed #265 as I noticed no-one was participating there, and this is a change that affects many and will surely cost me a lot of time to support, too.

I'd like to be able to address the feature of adding more tags, and I'd like to see design proposals that actually achieve that without doing more.

I don't like the idea of the interceptor proposed on #265 because it is much wider than what we need to accomplish.

interface Intercept<T>{

  boolean accept(Object object);

  T intercept(T interceptedObject);
}

The above is very loose and even if implementable would require generics to make any sense out of. We are in a very specific problem domain. The new concern here isn't at all unbounded, it is simply adding String -> String binary annotation/tags to a span from a source object.

The key concern above is parsing "servlet.principal" or similar, the value parsed goes into the current span! IOTW, if this is servlet or apache or netty http library, you have some source object that is what the span was created from, and you are asking to scrape more from it than initially designed. If keeping the design as simple as possible, the only interception points of note are when a span is opened or closed... parsing the request, response, or error.

Moving down this line of thinking, we can do it in a couple ways.. either the user of brave asserts the tags they want and some adapter of sorts tries to parse them, or the user gives an implementation which also includes what is needed.

Design 1: user says httpTagsDesired = "http.uri", "servlet.principal", "http.path" and the implementation attempts to provide up those tags Design 2: user says httpTagParser = fooParser, and fooParser includes logic and contract to get "http.uri", "servlet.principal", "http.path" where possible.

I like the fact that we are interested in making features, but we really need to drive them with requirements and least touch possible. In other words, if there's a strong desire to create an abstract interceptor idea that could possibly solve http tags, then that should happen in a fork because I need to use my time to help solve the problem raised and can't afford to work on things that don't benefit most people.

[codefromthecrypt]

was chatting with @cschneider and think we could experiment supporting extensible means to add tag/binary annotations with the following parts:

• some means to declare which tags go on the http client or server span.
• some sort of value parser, which instrumentation packages could implement for common things like headers.
• some sort of a registry, which would map common tag names (like zipkin.TraceKeys.HTTP_HOST) to a value parser.

We could backport the ability to parse all currently used zipkin.TraceKeys.HTTP_* and the ability to parse request arbitrary request headers (which this ticket is minimally about). We'd leave the "servlet principal" thing as a user test case, which shows how to parse a non-standard key (which would need access to a raw library class, like ServletRequest)

We'd then play with making the simplest means to configure additional keys that are either headers or the result of a custom parser. As the additional mappings need to end up in the various http filters, we'd end up needing to either extend public constructors (which I don't like), or retrofitting builders (which sounds better to me).

ex. somehow pass a list/map of "client.id" -> HttpHeader("x-client-id"), "servlet.principal" -> ServletPrincipalParser()

cc @epabst as you were asking earlier about exposing constructors and we'd probably want to make sure that whatever factory/builder/ctor pattern we make considers this.

[codefromthecrypt]

ps the design I mentioned ties into #260 as it implies indirecting value production.

[codefromthecrypt]

I'm working on a mockup of what I mentioned. Ideally, we can accomplish the goal of adding binary annotations aka tags to a span without forcing a common ancestor class among the various interceptors etc.

[codefromthecrypt]

Here's a mock-up

/**
 * Parses binary annotation/tags based for a given input type. Implemented as a list of key and
 * value parsers
 */
public interface TagExtractor<T> {

  /**
   * Builders will typically implement this, so that configuration can be applied generically. For
   * example, all http instrumentation would receive the same configuration callbacks.
   */
  interface Config<T extends Config<T>> {
    /** A key that the user wants to add to a span */
    T addKey(String key);

    /** Overrides behavior of value conversion, or adds support for new ones. */
    T addValueParserFactory(ValueParserFactory factory);
  }

  /**
   * Implement this to teach TagExtractor how to parse a key, potentially across multiple
   * libraries.
   */
  interface ValueParserFactory {
    /**
     * Returns a parser that can handle the input type and the binary annotation key or null if
     * unsupported
     */
    ValueParser<?> create(Class<?> type, String key);
  }

  /** Used to get binary annotation values from an input, such as a request object */
  interface ValueParser<T> {
    @Nullable String parse(T input);
  }

  /**
   * any non-null values parsed from the configured keys are returned
   */
  Collection<KeyValueAnnotation> extractTags(T input);
}

Example integration is something like this...

• instrumentation implements ValueParserFactory for all standard keys
  • additional ValueParserFactory can be added to support custom keys
• instrumentation uses TagExtractor at runtime, ex requestTagExtractor.extractTags(request)
  • these are added as binary annotations just like HttpRequestAdapter.requestAnnotations()

The user's job is to configure a list of strings, the keys they want. ex "http.host", "remote_user". Via TagExtractor, whichever of those keys are supported will end up as tags/binary annotations.

Here's how you might teach instrumentation about "remote_user"

valueParserFactory = (type, key) -> {
  if (!key.equals("remote_user")) return null;

  if (HttpServletRequest.class.isAssignableFrom(type)) {
    return (ValueParser<HttpServletRequest>) request -> request.getRemoteUser();
  } else {
    // TODO: figure out other libraries!
    return null;
  }
};

Note that the lookup is done up-front. In other words, the valueParserFactory isn't used at runtime, rather only the value parsers themselves.

[codefromthecrypt]

I've worked a little on the design above and will probably hack a bit more on it tomorrow. Here's the work in progress https://github.com/openzipkin/brave/pull/276

[codefromthecrypt]

I'm pretty sure the TagExtractor/ValueParser will work. I've weaved it in via builders to every http request and response adapter. I revised the design mockup above, too

[codefromthecrypt]

Added this to standardize how instrumentation are constructed (basically add a builder). After this is in, we have a place to start adding hooks, such as which keys to log. https://github.com/openzipkin/brave/pull/279

[codefromthecrypt]

here's a summary of the design I've in progress. It takes a while to do this since I don't want to merge anything unless it works for all libraries (and ..we've quite a few!)

https://github.com/openzipkin/brave/issues/284

[codefromthecrypt]

here's an example of adding something today (adding client ip).. notice I did end up modifying the servlet adapter although I could have registered another filter to do the same (looking up the context key) https://github.com/openzipkin/brave/pull/295

[codefromthecrypt]

I think it is best to do this work in brave 4 #180

[codefromthecrypt]

@remmeier it took a very long time to get everything in place, but do you mind looking at #350? I think most of your requirements should now be met, specifically via the HttpServerParser type there which allows you to see an underlying request such as servlet.