openzipkin / zipkin

Zipkin is a distributed tracing system
https://zipkin.io/
Apache License 2.0
16.94k stars 3.08k forks source link

Express key interfaces as java interfaces, not scala traits #451

Closed codefromthecrypt closed 5 years ago

codefromthecrypt commented 9 years ago

A number of folks have expressed a desire to have zipkin become more friendly to non-scala implementations. We want integrators to not leave zipkin solely because of scala or finagle.

For example, the brave project could implement interfaces defined in zipkin as opposed to similar ones as zipkin only exposes traits. More recently at Lyft, @jpinner needs to implement a Splunk base SpanStore in java.

By making integration points representable and implementable as dependency-free java types, we can slow the amount of divergence in the ecosystem, and head towards convergence. This issue will track the traits and types folks prefer to be java types, and address modeling, for example what to use instead of scala's Seq.

codefromthecrypt commented 9 years ago

interested parties include at least @jpinner @dsyer @autoletics @kristofa @eirslett @xylus @michaelsembwever and myself. Many of these folks can write scala, but are working in interop scenarios that prefer java. (correct me if I'm wrong folks)

codefromthecrypt commented 9 years ago

@postwait based on your presentation last year, I suspect you are also interested in this.

NiteshKant commented 9 years ago

If this was the case, we would not have written our own custom dapper implementation inside Netflix, so I certainly support the intent here :)

autoletics commented 9 years ago

The Probes Open API approach to API library design would be a good starting point...if not the reference itself. The original API was designed in 2008 and has remained the same except that when I moved from JXInsight (product) to Satoris (technology) I removed the com.jinspired.jxinsight.probes.* from the namespace and replaced it with org.jinspired.probes.*.

http://autoletics.github.io/probes-api/

The library includes a single Java class for bootstrapping the underlying provider implementation, see SPI, and everything else is expressed as an inner interface. Other than the single class there is not dependency on an actual implementation.

The Probes Open API also shows how best to implementation extension points. Please look at Strategy, Interceptor,...

I am going to write an article that shows how easy it is to extend the metering runtime via the Interceptor Extension API to publish all method call spans to the existing Zipkin Collector. Hopefully this would serve as the inspiration though naturally I would prefer for everyone to instead adopt the Probes API as the new trace instrumentation and then to move on focusing on span collectors/stores/ui.

codefromthecrypt commented 9 years ago

@autoletics so I think this isn't the right issue to track a move towards a completely different model, as it scopes this far beyond type conversion. My advice would be to create a separate issue for moving to the probes api, if that's your goal. It would be less distracting and also easier to find in the issues list.

my 2p on where the idea of probes-api convergence:

The idea of a general api for JVM-based tracing implementations is also broader than zipkin itself, as it crossed HTrace, Pinpoint, and other Dapper clones. It may well be the case that all of the dapper clones no longer wish to be dapper clones and instead probes api implementors, but that's still not great to have as a sidebar here. Such a goal would only be implementable if each of the projects had an explicit desire to do that. I'm not sure all are there, yet. How about opening an issue in https://github.com/autoletics/probes-api issues list, to demonstrate the value of converging perhaps with an adapter? That way instead of telling people what's best, they would be able to see that it is best and ask for it. Regardless, the engagement and visibility of the probes-api project would increase as a result, which could only help your aim which is to make this a standard api.

I'm sure you have feedback on my advice, but I'm concerned that this thread will be distracted further. If you don't mind, ping me on any other channel, or even in a probes-api issue, and I'd be happy to follow-up with you. In the mean time, let's try to keep this issue scoped as I don't want people to not want to comment on it at all because it got stuck in a probes-api discussion. Fair?

eirslett commented 9 years ago

Are these the kinds of APIs we're talking about? https://github.com/openzipkin/zipkin/blob/master/zipkin-common/src/main/scala/com/twitter/zipkin/storage/SpanStore.scala

Given this not-Java-friendly API:

trait MyStore
class MyStoreImpl

One easy way to provide Java friendliness for traits, is to make abstract classes for them:

trait MyStore
abstract class AbstractMyStore extends MyStore

And then use that from Java:

class MyStoreImpl extends AbstractMyStore {}

Twitter already does this in a couple of places, e.g. AbstractFunction. In these cases it could be enough to just provide stub abstract classes for Java developers to implement.

One thing I think could be particularly valuable, is to rip out the Zipkin stuff from Finagle, rewrite it more or less exactly from Scala to Java, and then use that code from both Finagle, Brave, and other places. (Basically, a reference implementation without external dependencies)

mosesn commented 9 years ago

+1 for rewriting it in java if you want it to be java-friendly.

jpinner commented 9 years ago

Some of the traits can't be instantiated in Java even if you create abstract classes for them in Scala because of how they mix in closable.

Sent from my iPhone

On Jul 10, 2015, at 3:50 AM, Eirik Sletteberg notifications@github.com wrote:

Are these the kinds of APIs we're talking about? https://github.com/openzipkin/zipkin/blob/master/zipkin-common/src/main/scala/com/twitter/zipkin/storage/SpanStore.scala

Given this not-Java-friendly API:

trait MyStore class MyStoreImpl One easy way to provide Java friendliness for traits, is to make abstract classes for them:

trait MyStore abstract class AbstractMyStore extends MyStore And then use that from Java:

class MyStoreImpl extends AbstractMyStore {} Twitter already does this in a couple of places, e.g. AbstractFunction. In these cases it could be enough to just provide stub abstract classes for Java developers to implement.

One thing I think could be particularly valuable, is to rip out the Zipkin stuff from Finagle, rewrite it more or less exactly from Scala to Java, and then use that code from both Finagle, Brave, and other places. (Basically, a reference implementation without external dependencies)

— Reply to this email directly or view it on GitHub.

eirslett commented 9 years ago

https://github.com/twitter/util/blob/master/util-core/src/main/scala/com/twitter/util/Closable.scala#L11

Maybe this works?

abstract class AbstractMyStore extends AbstractClosable with MyStore

and then in Java

class MyStoreImpl extends AbstractMyStore {}
kristofa commented 9 years ago

We should list all the integration points that we want to define as Java implementations (SpanStore might be one of them).

I also prefer a Java implementation. Another way in which we have tested interoperability with Scala is writing tests in Java.

michaelsembwever commented 9 years ago

I'm in favour of replacing all (possible) traits with Java interfaces. Would have saved me time already. Not even worrying about what's the APIs needing exposing are. Unless I'm mistaken and Java interfaces cause problems?

I'm really not in favour of trying to wrap traits. (traits can extend those Java interfaces if we need to maintain that compatibility).

eirslett commented 9 years ago

None of the stuff inside the openzipkin/zipkin repository (or twitter/zipkin) is actually released as libraries (as far as I know), but only deployed to production directly from the master branch. And it's self-contained. So as long as we refactor across the whole project, backwards compatibility shouldn't be an issue.

codefromthecrypt commented 9 years ago

Good point. This is a hidden advantage for sure. At any rate we could release 1.2 and do whatever we need here (breaks and all) as 2.0.

b commented 9 years ago

This is helpful, but doesn't address the related problem of wanting to avoid having to build and deploy scala components at all. Is there already an issue tracking that? It is something we would like to help with, regardless.

eirslett commented 9 years ago

Lookout has done some work on publishing zipkin builds to bintray; we could let people use them instead og building from source. One of the issues then is how to plug in the various storage backends, runtime? Dynamic linking?

codefromthecrypt commented 9 years ago

This is helpful, but doesn't address the related problem of wanting to avoid having to build and deploy scala components at all. Is there already an issue tracking that? It is something we would like to help with, regardless

Good point, Ben. Please add an issue for a pure java implementation. Via discussions, at least Netflix, Lyft and users of Brave would be interested. You'll also at least get my hands helping.

This issue could be considered a milestone towards minimal dependency zipkin.

codefromthecrypt commented 9 years ago

Lookout has done some work on publishing zipkin builds to bintray; we could let people use them instead og building from source. One of the issues then is how to plug in the various storage backends, runtime? Dynamic linking? There are a number of ways to attack configuration of backends. At first, we can just do the port work. Care to bump the config concern out to its own issue?

codefromthecrypt commented 9 years ago

Actually, maybe it is simpler just to rename this issue to rewrite in java? We can always raise incremental pull requests.. Wdyt?

michaelsembwever commented 9 years ago

rewriting just the traits to interfaces is a small incremental step in a positive direction.

i'm a fan of small atomic tickets. can we open up a new ticket, an epic, for "rewrite (core parts) in java"?

On 16 July 2015 at 16:16, Adrian Cole notifications@github.com wrote:

Actually, maybe it is simpler just to rename this issue to rewrite in java? We can always raise incremental pull requests.. Wdyt?

— Reply to this email directly or view it on GitHub https://github.com/twitter/zipkin/issues/451#issuecomment-121969227.

Mick Semb Wever Scandinavia

The Last Pickle Apache Cassandra Consulting http://www.thelastpickle.com

b commented 9 years ago

I agree, a separate issue is best.

https://github.com/openzipkin/zipkin/issues/463

michaelsembwever commented 9 years ago

Taking a look at this it doesn't look like the first step is replacing traits, since most of them reference other scala classes.

I suspect a right "first step" is to look into rewriting zipkin-common. This then gets us closer to replacing traits with interfaces, and allowing new subprojects in the zipkin server-side stack to be written with just java (which will help attract more developers to the community).

eirslett commented 9 years ago

which will help attract more developers to the community

I'm afraid it could repel some Scala developers though... I can only speak for myself, but I'd much prefer most of (~99 %) the code base to still be Scala, with only a few Java hooks here and there. (Of course with the exception of the instrumentation code that's going into people's business applications, which should be Java-based like Brave.) The difference between rewriting "key interfaces" and "the whole Zipkin domain model" is pretty big, and deciding to rewrite the entire Zipkin core is a different decision altogether, I would say.

eirslett commented 9 years ago

I suggest we make some small changes to the Zipkin server-side code so that it's easier to use both from Java and Scala, without having to rewrite all of it to Java.

michaelsembwever commented 9 years ago

Yes, we want the stack to be friendly to both java and scala implementations. Java implementations of the server-side stack should be free to be entirely java.

Today zipkin-common is the "interface" that plays a large part in gluing together the different stack implementations. Whether it is rewritten completely to java, or made java compatible, i have no strong opinion to.

I agree that we don't need to spend time right now on rewriting existing scala component implementations to the server's stack, where they are isolated, to java. If we have a true polyglot stack then if people want a particular component in java rather than scala they are free to do so, and this will be a addition rather than a rewrite. The question of what developers and the community prefers delegates simply to what is popular and maintained best.

codefromthecrypt commented 9 years ago

Well put!

codefromthecrypt commented 9 years ago

Fwiw, we do have java services at Twitter (ex storage) and of course services inherited via acquisition are rarely Scala.

Here are some points I have encountered in a recent project to make a common model across java and Scala (I attempted to abstract some of our time series processing code used for an alerting system).

The technical issues are daunting, but solvable. Regardless, they need to be solved in an environment that desires the solution.

On converting finagle shared libraries towards java:

On finagle services vs ones written without finagle (aka share IDL not libraries).

On pushing thrift to an extension

Hope this helps.

codefromthecrypt commented 9 years ago

SUMMARY:

463 will address an alternative pure-java backend.

This issue focuses on tactical work to make the existing scala/finagle backend more friendly to java or otherwise non-scala implementors. For example, this could include progress such as using scrooge to generate java types (which still have dependencies on scala and finagle).

shakuzen commented 5 years ago

The server has long since been rewritten in Java so I believe this issue is now obsolete.