Removes guava dependency

[codefromthecrypt]

Guava leads to library conflicts and interoperability problems. By
removing the guava dep, we can more safely be placed in core libraries.

Instead of forcing users to use or convert out of Guava's optional, we
now use nullable. Nullable is trivial to convert to an optional type in
the caller's favorite functional library or language.

[kristofa]

@adriancole Thanks for doing this! I was actually planning on removing guava and apache commons soon. Did you notice that a few days ago I did some commits which are merged to master that upgrade Java source and target to 1.8?

Because we are using Java 1.8 now I would propose to migrate to Java 8 Optional instead of going with null values. Do you think you could do this instead?

I think lots of users will be thankful that you tackle removing guava.

[codefromthecrypt]

@kristofa Thanks for the quick feedback. Here's my unacclimated 2p on the Java 8 question.

We should be cautious with using java Optional (and in fact consider moving source back to target 1.7). Few who use non-java languages or alternate functional libraries will be able to leverage java 8 Optional for some time, and it will be more of a chore to them than nullable. For example, in order to use brave, they may have to upgrade the root of their dependency tree!

Unless the source level of java 8 is strategic to us, I think it best to revert it to 7 (and add animal sniffer which I can help with). We can still build/test/run with JDK 8, but I think sticking on java 7 for some time will help adoption. Android (even if not explicitly targeted) is ruled out with Java 8, so are scala 2.10 users who aren't using finagle. Some of our integrations could move to java 8, but I think the core should stick to java 7 source level.

[michaelsembwever]

Removing guava is a fantastic move!

Sticking with Java7 also gets my vote. Being a compile-time library that falls in the "infrastructure components" category for any microservices platform, that is where the value of brave/zipkin is in having it everywhere in such a platform, would mean expecting often hundreds of services and applications to all be Java8 compatible before brave-3 can be considered an option.

When is Apache commons a problem? ( libthrift is already dependent on commons-lang3-3.1 )

[codefromthecrypt]

@michaelsembwever thx for the feedback. I would like us to be able to not maintain 2 forks in order to progress brave-3 as well!

wrt commons and libthrift:

I feel like libthrift is just an implementation of thrift, and I'd rather decouple our use of commons from our use of libthrift. For example, I've come very close to writing square wire thrift integration a couple times now :) I've also thought about what ifs for protobuf as opposed to only thrift. Practically speaking, I concede this is mostly pie-in-sky and we'd likely continue to have this as transitive for the foreseeable future.. That said, even if that's the case, if we don't commons directly, we don't need to track it vs what libthrift uses. One less thing to deal with.

wrt commons on its own:

I've no specific beef with apache commons except that it is a dependency that doesn't add much value (ex. our use of it would be maybe <100loc to copy or reproduce). If we have no apache commons dep, we have no need to track it and neither would our callers. It is a benign dep, I admit, but again, rather not think about it at all.

[kristofa]

Thanks for the input @adriancole and @michaelsembwever. You convinced me, I'll downgrade to Java 7. I'll have to rework some code I wrote earlier this week though which use Java 8 Optional.

I also think it should not be too difficult to remove apache commons dependency from brave itself. As far as I can remember we use HashCodeBuilder, EqualsBuilder and Validate. Those can be replaced by Java 7 Objects static utility methods.

[codefromthecrypt]

Whoot! So is this mergable? Regardless, I can raise another PR to downgrade and add animal sniffer.

[kristofa]

It is a little sad that we make the code less safe by removing the usage of Optional. But the biggest issue with guava is it is not backwards compatible across releases... so lets bite the bullet...

I never used animal sniffer Maven plugin before. Looks useful. Feel free to submit a pr :)

The code in which I introduced Java 8 Optional this week is the new client/server abstraction code that lives in brave-impl (ClientRequestInterceptor, ClientRequestAdapter, ServerRequestInterceptor, TraceData classes). The server abstraction should remove code duplication across different implementations when we integrate it. The client abstraction should replace what lives in brave-client and makes it more generic, less http specific.

[codefromthecrypt]

Thanks for the pointers!

@kristofa @michaelsembwever

Here's my plan with internal maintenance. All of which I can do all the work except review on. I understand all of which also have implications on in-flight work.

• pull request to animal sniff - this is important for the main source tree only, and we can override on modules we feel are safe to constrain to JRE 8. For example, we could still write tests using lambdas if we want, while still facilitating core portability.
• pull request to auto-value - We can handle the validation/toString/equals/hashCode case with no dependency or boilerplate by using auto-value. AutoValue writes classes (ex. that check null) as opposed to using reflection. Its annotations are source retention, so there's literally no dep leak to consumers.
• (optional - at some later point) pull request to style all the things - our xml and source files have different style and it is distracting. they look very close to google style, which is well documented. something to consider.

I understand that while I'm quite familiar and comfortable with above, they may not be universally liked. That said, I appreciate keeping an open mind. If such pull requests don't end up acceptable.. well it was worth a shot!