Initialize Brave Zipkin tracing for Dropwizard servers and JAX RS clients #235

schlosna · 2016-09-21T19:24:17Z

Introduces a Tracer wrapping Brave to produce Zipkin traces and spans to the "tracing" slf4j logger.

Typically a Dropwizard server should call com.palantir.remoting1.servers.DropwizardServers#configure as part of startup initialization, which will take care of registering the Brave singleton tracer in com.palantir.remoting1.servers.Tracers#activeTracer as a Jersey server interceptor.

JAX RS clients created via com.palantir.remoting1.jaxrs.JaxRsClient#builder(com.palantir.remoting1.clients.ClientConfig) will inherit the active tracer by default, or can provide their own via ClientConfig.

Similarly the Tracer allows for initiating local traces with its API.

Addresses #115

…ents Introduces a Tracer wrapping Brave to produce Zipkin traces and spans to the "tracing" slf4j logger. Typically a Dropwizard server should call `com.palantir.remoting1.servers.DropwizardServers#configure` as part of startup initialization, which will take care of registering the Brave singleton tracer in `com.palantir.remoting1.servers.Tracers#activeTracer` as a Jersey server interceptor. JAX RS clients created via `com.palantir.remoting1.jaxrs.JaxRsClient# builder(com.palantir.remoting1.clients.ClientConfig)` will inherit the active tracer by default, or can provide their own via `ClientConfig`. Similarly the Tracer allows for initiating local traces with its API.

schlosna · 2016-09-21T19:24:53Z

@uschi2000 @markelliot @clockfort for SA

schlosna · 2016-09-21T19:31:59Z

We still need to fix the span collection plumbing, sampling configuration, and a few more things, but I'm considering those for a followup PR for now.

uschi2000 · 2016-09-22T12:22:54Z

before I start reading the code changes: what's the high-level goal of this change?

schlosna · 2016-09-22T14:01:15Z

Rob, the high level goal is to provide a single unified Brave instance to Dropwizard based applications so that they can do local tracing, and inherit the proper incoming server trace & span as top level local spans' parents.

uschi2000 · 2016-09-22T14:21:05Z

Got it, thanks

On Thu, Sep 22, 2016 at 4:01 PM David Schlosnagle notifications@github.com
wrote:

Rob, the high level goal is to provide a single unified Brave instance to
Dropwizard based applications so that they can do local tracing, and
inherit the proper incoming server trace & span as top level local spans'
parents.

—
You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub
#235 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AGOdwUkEThpDiCIig_Ncln9a8SaAS5xRks5qsoosgaJpZM4KDLsA
.

schlosna · 2016-09-22T21:02:30Z

This will require some tweaking as http-remoting is shading some Brave components which causes the following issue when using this:

Exception in thread "main" java.lang.NoSuchMethodError: com.palantir.remoting1.servers.BraveTracer.defaultClock()Lcom/github/kristofa/brave/AnnotationSubmitter$Clock;
  at com.palantir.remoting1.servers.DropwizardTracingFilters.createBraveTracer(DropwizardTracingFilters.java:99)
  at com.palantir.remoting1.servers.DropwizardTracingFilters.getOrCreateBraveTracer(DropwizardTracingFilters.java:82)
  at com.palantir.remoting1.servers.DropwizardTracingFilters.registerTracers(DropwizardTracingFilters.java:63)
  at com.palantir.remoting1.servers.DropwizardServers.configure(DropwizardServers.java:50)

Might also be worth separating the tracing initialization from the other aspects in DropwizardServers as the tracing needs to be setup first before anything creates JAX RS clients or nothing will be traced.

uschi2000 · 2016-09-23T12:09:56Z

Hmm, I find this all very hard to understand :( There's a strange mixture between static state (e.g., Tracers.getActiveTracer()) and injected state (e.g., ClientConfig#setTracer). David, can we brainstorm a little in person to find a simpler solution?

(Or maybe I'm just not getting the point...)

uschi2000 · 2016-09-23T11:35:28Z

dropwizard-servers/src/main/java/com/palantir/remoting1/servers/DropwizardTracingFilters.java

@@ -57,32 +60,45 @@ private DropwizardTracingFilters() {}
     * TODO(rfink) Is there a more stable way to retrieve IP/Port information?
     */
    static void registerTracers(Environment environment, Configuration config, String tracerName) {
-        ServerTracer serverTracer = getServerTracer(extractIp(config), extractPort(config), tracerName);
+        final BraveTracer tracer = getOrCreateBraveTracer(config, tracerName);


final isn't normally used in this codebase

uschi2000 · 2016-09-23T11:38:10Z

ext/brave-extensions/src/main/java/com/palantir/remoting1/servers/BraveContext.java

+    private final Brave brave;
+    private final SpanId spanId;
+
+    @SuppressWarnings("WeakerAccess") // public API


this is to suppress the intelliJ hint?

Yes, this suppresses an IntelliJ warning since this is unused in this project

uschi2000 · 2016-09-23T11:40:42Z

ext/brave-extensions/src/test/java/com/palantir/remoting1/servers/BraveTracerTest.java

+    @Test
+    public void traceCallable() throws Exception {
+        @SuppressWarnings("unchecked") // yay generic type erasure
+        Callable<String> mockCallable = mock(Callable.class);


can get around this with annotation-based mocking:

@Mock private Foo<Bar> foo;

uschi2000 · 2016-09-23T11:42:15Z

http-clients-api/src/main/java/com/palantir/remoting1/servers/Tracer.java

+    /**
+     * Begins a trace context for the specified component and operation.
+     *
+     * @param component component name


uschi2000 · 2016-09-23T11:42:19Z

http-clients-api/src/main/java/com/palantir/remoting1/servers/Tracer.java

+     * @param component component name
+     * @param operation operation name
+     *
+     * @return new trace context


uschi2000 · 2016-09-23T11:43:16Z

http-clients-api/src/main/java/com/palantir/remoting1/servers/Tracers.java

+/**
+ * Utility methods for tracing components and operations.
+ */
+@SuppressWarnings("WeakerAccess")


not a fan of this

uschi2000

Still struggling with the semi-coupled nature of our Tracers wrapper and the Brave implementation. Is there a particular reason we need to wrap Brave?

uschi2000 · 2016-09-28T18:03:54Z

http-clients-api/src/main/java/com/palantir/remoting1/servers/Tracer.java

+     */
+    TraceContext begin(String component, String operation);
+
+    Class<? extends TraceContext> getContextClass();


why do we need this?

schlosna · 2016-09-28T18:26:18Z

I'd be fine with exposing Brave directly to both servers (and possibly clients) for use as needed for local tracing -- my main goal here is to get the inheritable state shared across server, client, and local tracers, as well as have a consistent mechanism for configuring the Brave instance to sample & collect spans, so that we can have the configuration injected via service discovery.

The main concern for exposing Brave here is that we're tying ourselves to a concrete API. I don't have strong objections to that as I think we can handle breaks and incompatibility as needed for our own http-remoting consumers.

Another option might be to use the opentracing-api from
https://github.com/opentracing/opentracing-java and the brave-opentracing bridge from https://github.com/openzipkin/brave-opentracing . That route might lose some of the access to Brave niceties, but I haven't looked into that route quite yet.

uschi2000 · 2016-09-28T18:31:25Z

The current PR feels like a limbo with some parts wrapped in our own API
and some parts using Brave directly.

On Wed, Sep 28, 2016 at 7:26 PM David Schlosnagle notifications@github.com
wrote:

I'd be fine with exposing Brave directly to both servers (and possibly
clients) for use as needed for local tracing -- my main goal here is to get
the inheritable state shared across server, client, and local tracers, as
well as have a consistent mechanism for configuring the Brave instance to
sample & collect spans, so that we can have the configuration injected via
service discovery.

The main concern for exposing Brave here is that we're tying ourselves to
a concrete API. I don't have strong objections to that as I think we can
handle breaks and incompatibility as needed for our own http-remoting
consumers.

Another option might be to use the opentracing-api from
https://github.com/opentracing/opentracing-java and the brave-opentracing
bridge from https://github.com/openzipkin/brave-opentracing . That route
might lose some of the access to Brave niceties, but I haven't looked into
that route quite yet.

—
You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub
#235 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AGOdwXOTWaG4-kQoEE8XMjNl4AJG_IjEks5qurFKgaJpZM4KDLsA
.

uschi2000 · 2016-09-28T18:31:35Z

(Sorry, I have to run now.)

On Wed, Sep 28, 2016 at 7:31 PM Robert Fink rf@robertfink.de wrote:

The current PR feels like a limbo with some parts wrapped in our own API
and some parts using Brave directly.

On Wed, Sep 28, 2016 at 7:26 PM David Schlosnagle <
notifications@github.com> wrote:

I'd be fine with exposing Brave directly to both servers (and possibly
clients) for use as needed for local tracing -- my main goal here is to get
the inheritable state shared across server, client, and local tracers, as
well as have a consistent mechanism for configuring the Brave instance to
sample & collect spans, so that we can have the configuration injected via
service discovery.

The main concern for exposing Brave here is that we're tying ourselves to
a concrete API. I don't have strong objections to that as I think we can
handle breaks and incompatibility as needed for our own http-remoting
consumers.

Another option might be to use the opentracing-api from
https://github.com/opentracing/opentracing-java and the brave-opentracing
bridge from https://github.com/openzipkin/brave-opentracing . That route
might lose some of the access to Brave niceties, but I haven't looked into
that route quite yet.

—
You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub
#235 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AGOdwXOTWaG4-kQoEE8XMjNl4AJG_IjEks5qurFKgaJpZM4KDLsA
.

markelliot · 2016-09-30T13:22:39Z

I think the challenge with this PR is that it's not a full-measure for wrapping Brave. My basic stance (and I don't feel strongly about which of these to choose) is that we need to either:

Just use Brave as-is and live with its semantics
Use our own tracing shim (which is less code than this PR and will have precisely the semantics we want) + write an adapter from that shim to integrate with Zipkin

If I had to choose, I'd prefer 2.

schlosna · 2016-09-30T13:30:55Z

@markelliot I don't really want to wrap Brave, I'd rather inject it into all of the places that we want to do tracing, but a lot of the http-remoting APIs currently prevent that and make this more difficult than it needs to be -- e.g. we need to push the trace config into the JaxRs ClientConfig so that tracing there knows what to trace and where to push it. This is made more difficult by the shading done by http-remoting and dependency limitations, which is how I ended up with this PR for the time being.

markelliot · 2016-09-30T16:23:55Z

Not sure I grok what you're saying – we own all of the JaxRs code, so we could in theory make this simpler?

schlosna · 2016-10-02T20:24:03Z

I have a cleanup that I think is cleaner -- it will add brace-core and brace-okhttp dependency for http-clients-api.
I will push later tonight or tomorrow once I increase test coverage.

uschi2000 · 2016-10-03T00:02:54Z

Cheers David

On Sun, Oct 2, 2016, 13:24 David Schlosnagle notifications@github.com
wrote:

I have a cleanup that I think is cleaner -- it will add brace-core and
brace-okhttp dependency for http-clients-api.
I will push later tonight or tomorrow once I increase test coverage.

—
You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub
#235 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AGOdwa5xxDcG49fk8lmWKYMBlOckGBh2ks5qwBLkgaJpZM4KDLsA
.

schlosna · 2016-10-11T17:47:28Z

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Initialize Brave Zipkin tracing for Dropwizard servers and JAX RS clients #235

Initialize Brave Zipkin tracing for Dropwizard servers and JAX RS clients #235

schlosna commented Sep 21, 2016 •

edited

schlosna commented Sep 21, 2016 •

edited

schlosna commented Sep 21, 2016

uschi2000 commented Sep 22, 2016

schlosna commented Sep 22, 2016

uschi2000 commented Sep 22, 2016

schlosna commented Sep 22, 2016 •

edited

uschi2000 commented Sep 23, 2016 •

edited

uschi2000 Sep 23, 2016

uschi2000 Sep 23, 2016

schlosna Sep 28, 2016

uschi2000 Sep 23, 2016

uschi2000 Sep 23, 2016

uschi2000 Sep 23, 2016

uschi2000 Sep 23, 2016

uschi2000 left a comment

uschi2000 Sep 28, 2016

schlosna commented Sep 28, 2016

uschi2000 commented Sep 28, 2016

uschi2000 commented Sep 28, 2016

markelliot commented Sep 30, 2016

schlosna commented Sep 30, 2016

markelliot commented Sep 30, 2016

schlosna commented Oct 2, 2016

uschi2000 commented Oct 3, 2016

schlosna commented Oct 11, 2016

Initialize Brave Zipkin tracing for Dropwizard servers and JAX RS clients #235

Initialize Brave Zipkin tracing for Dropwizard servers and JAX RS clients #235

Conversation

schlosna commented Sep 21, 2016 • edited

schlosna commented Sep 21, 2016 • edited

schlosna commented Sep 21, 2016

uschi2000 commented Sep 22, 2016

schlosna commented Sep 22, 2016

uschi2000 commented Sep 22, 2016

schlosna commented Sep 22, 2016 • edited

uschi2000 commented Sep 23, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

uschi2000 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

schlosna commented Sep 28, 2016

uschi2000 commented Sep 28, 2016

uschi2000 commented Sep 28, 2016

markelliot commented Sep 30, 2016

schlosna commented Sep 30, 2016

markelliot commented Sep 30, 2016

schlosna commented Oct 2, 2016

uschi2000 commented Oct 3, 2016

schlosna commented Oct 11, 2016

schlosna commented Sep 21, 2016 •

edited

schlosna commented Sep 21, 2016 •

edited

schlosna commented Sep 22, 2016 •

edited

uschi2000 commented Sep 23, 2016 •

edited