Brave 3.0.0

[kristofa]

While brave works quite good I think it is time to rework some parts and to remove some legacy code. Here is a list of improvements I'm thinking about doing:

1. Introduce brave server abstraction similar as the client abstraction @srapp implemented ( #27 ). Just like having the client abstraction having the server abstraction that hides ServerTracer will make it less effort to integrate and improve consistency. ClientTracer / ServerTracer can still be used for non http integrations (eg database).
2. Update documentation on how to use brave client / server abstractions.
3. Get rid of complexity of dealing with span names. Currently when integrating with http frameworks we use the path part of the URL as the span name. This resulted in inconsistencies ( #32 ) between client/server and lots of effort to clean paths that contain variable content ( #33 ). Instead I would like to follow the approach of Zipkin / Finagle which uses the http method as span name (GET, PUT, POST,...) and adds the url path unchanged as a binary annotation to the span.
4. Compatibility with Finagle. Make sure mixing Finagle / Zipkin and Brave services works. Previous point is also related to this.
5. Upgrade to Java 7. Java 6 is not supported anymore and Java 7 is getting pretty old already. Require at least Java 7.

These are the main topics I can think of now. Those changes will change the preferred way of integrating with Brave and the visualization of traces in zipkin-web that's why version 3.0.0

[kristofa]

Feel free to give your input. @joeslice , @henrikno , @K-Jo , @srapp , @wjam , @eirslett , @michaelsembwever , @hzhaofb , @ryantenney.

[eirslett]

1. Are you sure you want to use HTTP as the "default", instead of having brave-http-client, brave-http-server etc.? We run a lot of Thrift microservices where we don't need/want HTTP dependencies.
2. Great! The more documentation, the better!
3. Sounds good!
4. That's great! One problem is that you can't mix Finagle clients and Brave clients within the same application - but I guess the most important aspect is that different services within the platform should be able to communicate/trace independent of Zipkin client implementation.
5. Good idea.

Sounds like there will be no breaking changes (to the public API). That's good :-) Since we have a few Brave calls from userland code, not just in commons libraries.

[joeslice]

Awesome, looking forward to seeing these changes. These all sound reasonable @kristofa . I would add to the list of nice updates:

• Refactor the ZipkinSpanCollector and SpanProcessingThread. The queuing in this class could be rethought a bit. A few adjustments I'd like to make in 3.0: 1. The threading logic and queueing logic can be rethought to be a bit more smooth in a low-load-followed-by-spike scenario, and not be as latent in the general case. 2. Combining the worker with the thread manager may be a bit clearer. 3. Extracting a super class with much of the coordination would be useful in the case we want a different implementation of the actual reporting protocol (we have built one over HTTP instead of Thrift for example). 4. The configuration would need to be changed a bit to support this refactoring.
• Throughout brave, it would be nice to be working with a set of Brave-specific domain objects (like a com.github.kristofa.brave.domain.Span) instead of the Thrift generated classes. Transforming to a Thrift class at the last possible moment means that we can enhance the domain, and use it in different ways without being constrained to the Thrift domain.
• Span naming logic: as you've mentioned it would be nice to understand all the places (before and after a request have started processing) that we would want to make a decision about span names, and support them all by design.

Thanks again for all your work on this library. I'm sure we at @bazaarvoice will be contributing to the 3.0+ efforts. If you choose to create issues for each of these things, and mark some as contribution-wanted, I bet you'll get help from several groups...

[srapp]

This looks like a sweet list of changes @kristofa . I would add that there is value in having more specific span names, especially for filtering traces and for understanding dense traces with many different calls to the same service. At the very least, introducing that logic into a shared module so that it can be shared across different implementations could help alleviate the complexity (or at least contain it), but I'd like to hear others' use cases as well.

Also, +1 to what @joeslice posted. I'd love to help and contribute.

[kristofa]

Thanks for the input guys!

@eirslett 1. I actually am thinking of renaming current brave-client to brave-http-client as you mention and introduce a brave-http-server counterpart. Because the current client is indeed focused on http. This should at least make all http integrations conform to each other and reduce code duplication. You will still be able to use Client/ServerTracer directly for custom integration.

@joeslice I'd be happy to hear about your ideas on improving ZipkinSpanCollector. With the reporting protocol you mean that you don't send thrift to the Twitter collector but you have build your own collector and send over http?

@srapp My idea was to make the default implementation simple. I'll think about making it pluggable so you might swap with your own implementation.

I hope to start with the implementation in a few weeks.

[joeslice]

I'd be happy to hear about your ideas on improving ZipkinSpanCollector.

I'll submit a patch with some ideas for ZipkinSpanCollector.

With the reporting protocol you mean that you don't send thrift to the Twitter collector but you have build your own collector and send over http?

Yes, we built a web service that accepts span data via HTTP. This worked better for us than thrift because of the stateless connections, and the ability to use load balancers, etc.

[michaelsembwever]

1. I agree with Eirik's concern here. As the microservices trend becomes more popular so will the appeal of zipkin, and along with it people should/will become more conscientious to the unnecessarily large transitive dependencies trees all too often getting sucked into codebases through compile time libraries like brave. Any effort to minimise such transitive dependencies will always be welcome (whether it through separate artifacts or optional dependencies).
2. If there are to be no/little breaking changes, and you're thinking of bugfix maintain 2.x for a while, why not update to Java8 and remove Guava? (Guava, along with thrift, scala, spring, and logging frameworks, are typical pain-points that are nice, if possible, to avoid having in transitive dependency trees).

[fedor57]

Kristof,

thanx, let me add a little of my humble opinion on span/service names topic. I really don't think that the approach to simplify naming that you decided for 3.0 is the right thing to do. There are the following facts:

• product requirements for Zipkin infrastructure users are more important than complexity issues you experience. For example in my company only I know something about those issues and Zipkin internals, but the other guys who will be using the interface will ask me "Why? Why we get that meaningless names and cannot filter? Can you fix it now?". What should I answer? "No, never, because this fix will not be right thing to do and your existing infrastructure is ill from the beginning because of bad url naming conventions."
• you shouldn't follow Twitter guys here. Actually they create an internal tool but don't think about deployment in a different hetegoreneous environments. They just don't care much, it is simplier for them to have a standart url naming and deal with it internally. By the same reason they don't care about IPv6 now.
• at the same time looks like Brave is one of the main efforts to bring Zipkin to the very different companies, so you should care ;)
• from my tests I don't see a big problems of having different service names on client and server for the same span. There is some doubling in filters but it works fine. But I haven't tested for span names yet.
• service always knows better about it's own naming. The only reason to write something else for "cs" on client side besides ids and timing is that client does not know if server has Zipkin instrumentation. If it has from my point of view - the server naming should always overwrite everything that is know from the client side.
• so no real meaning for consistency on server or client side for service/span names. It's just the way to make sure the some information will be available. I think that is the original idea in Dapper. If in Zipkin code server info does not overwrite client one, it should be fixed inside Zipkin.
• a correspondence between urls and service/span names is a very specific thing. Some of the deployment would like to use host name to derive service name, some have url prefixes. The way to extract names is very similar to url routing for typical Web MVC application. If those non-trivial rules exist why those shouldn't be reflected in the Zipkin tracing? The main idea - trivial mapping everywhere is not right if you want to deploy Zipkin in different companies
• from my point of view there must be 1) default url-based procedures 2) regex and other matching rules in config 3) plugins to derive service/span names. And no restrictions for server/client names match.

[fedor57]

Also I think the following additions can make Brave more usable in different environments:

Some ways to report in annotations:

• hostname before SLB - as seen from the client (almost the same as http.request annotation)
• IPv6 of the server
• hostname of the server

Also as I guess there are some optimizations in reporting Endpoints, so those are not send for every span. Do you think that adding 3 mentioned annotations to every span will increase the traffic significantly because they will not be optimized? Perhaps it's too hard to implement all this on Brave client side efficiently, so some changes to Brave implementation should be done.

[kristofa]

@fedor57 lots of comments, thanks :)

Just an additional remark on simplifying span names. What I would do is make the default implementation for HTTP integration more simple by using the http method as span name (GET, PUT, POST,...) but add a binary annotation (key = http.uri) containing the request path without query params as value. I don't want to do this because I want to explicitly mimic what Twitter does with Zipkin but because I have found it to work well and I like simple things that work well :) It is what Finagle does when using HTTP and I have been looking at traces generated like this for a while now.

If we find the right abstractions you should be able to configure your own implementation for changing span names the way you want. I'll take this into account.

[codefromthecrypt]

not sure if universally desired, but I'll timebox a stab at removing guava. wish me luck

[codefromthecrypt]

here's guava rip: https://github.com/kristofa/brave/pull/72

side-note: I also don't think the apache commons thing is pulling its weight. We'd be better off with no core deps outside thrift imho.

[schrepfler]

Can we get a RC published to maven central so we can play a bit?

[kristofa]

@schrepfler Yes, certainly. I'll try to publish a RC still this week.

[kristofa]

@schrepfler and others: brave 3.0.0-alpha-1 is out! Big thanks for the contributions by @adriancole !

[kristofa]

What's left to do before hitting brave 3.0.0 besides testing:

1. Finalize work on brave-http. Build new abstractions which build a small http specific layer on top of the new client/server abstractions in brave-core (ClientRequestInterceptor, ClientResponseInterceptor, ServerRequestInterceptor and ServerResponseInterceptor). This work has started in http-client-server-abstraction branch.
2. Integrate work finished in 1. into different http implementations (resteasy, jersey,...). This will introduce more uniformity and remove code duplication.
3. Integrate the brave-core client/server abstractions in the non http integrations.
4. Update documentation. Especially about the usage of the new Client/Server abstractions.
5. Fix issue #66

[schrepfler]

I think there's a space for a servlet api module as well, it will open integration to things like Riverbed/Brocade ADC/SteelApp or apps which don't user jersey.

[codefromthecrypt]

I'm going to spike making core as dep-free as possible, ideally none except bare-bones thrift.

Part of this will be analyzing the generated code we are using and probably some more maven refactoring.

Ideally, brave 3 could be considered in dependency sensitive environments, such as Android, alternate JVM languages or rpc libraries (ex grpc, rxnetty etc), without folks having to shade us. That's my goal.

-A

[kristofa]

@adriancole I saw your pr #77. Will have a closer look at it soon! So far I focused mainly on back-end systems and didn't give any attention on making brave work well on for example Android and slimming down the dependencies to ideally be dep free... but I agree that this has huge potential for adoption and is a good thing to include in brave 3.0.0

[schrepfler]

I noticed we use getLocalAddr and getLocalPort on the ServletTraceFilter https://github.com/kristofa/brave/blob/194a26616893e2a70e9792277002de61dfefc066/brave-jersey/src/main/java/com/github/kristofa/brave/jersey/ServletTraceFilter.java

As these are Servlet 2.4, is there a chance some other method can be used so that we can be support even 2.2?

[kristofa]

@schrepfler To fix #66 I would like to change the way in which we determine the ip address for Endpoint. So I would wait with the change to ServletTraceFilter. That code might become obsolete in future.

[noroute]

@fedor57 Concerning span names, there is the pseudonymization use case (privacy regulations!) which would make pluggable span names very useful.

[kristofa]

@eirslett @joeslice @srapp @fedor57 @noroute @adriancole @schrepfler @michaelsembwever All the planned changes for brave 3.0.0 (and more) have been implemented and I just released a release candidate: brave 3.0.0-rc-1 which should become available in Maven Central shortly.

Please test it out and give your final feedback. I plan to make the final release in the week starting 14th of September. Cheers!

[kristofa]

Brave 3.0.0 released. Closing.