Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
adriancole opened this issue Jul 9, 2015 · 28 comments
Closed

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

adriancole opened this issue Jul 9, 2015 · 28 comments
Labels

Comments

@adriancole
Copy link
Contributor

@adriancole adriancole commented Jul 9, 2015

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.

@adriancole

This comment has been minimized.

Copy link
Contributor Author

@adriancole adriancole commented Jul 9, 2015

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)

@adriancole

This comment has been minimized.

Copy link
Contributor Author

@adriancole adriancole commented Jul 9, 2015

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

@NiteshKant

This comment has been minimized.

Copy link
Member

@NiteshKant NiteshKant commented Jul 9, 2015

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

This comment has been minimized.

Copy link

@autoletics autoletics commented Jul 9, 2015

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.

@adriancole

This comment has been minimized.

Copy link
Contributor Author

@adriancole adriancole commented Jul 10, 2015

@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

This comment has been minimized.

Copy link
Contributor

@eirslett eirslett commented Jul 10, 2015

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

This comment has been minimized.

Copy link
Contributor

@mosesn mosesn commented Jul 10, 2015

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

@jpinner

This comment has been minimized.

Copy link

@jpinner jpinner commented Jul 10, 2015

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

This comment has been minimized.

Copy link
Contributor

@eirslett eirslett commented Jul 10, 2015

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

This comment has been minimized.

Copy link
Member

@kristofa kristofa commented Jul 10, 2015

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

This comment has been minimized.

Copy link
Member

@michaelsembwever michaelsembwever commented Jul 10, 2015

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

This comment has been minimized.

Copy link
Contributor

@eirslett eirslett commented Jul 10, 2015

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.

@adriancole

This comment has been minimized.

Copy link
Contributor Author

@adriancole adriancole commented Jul 11, 2015

@b

This comment has been minimized.

Copy link

@b b commented Jul 16, 2015

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

This comment has been minimized.

Copy link
Contributor

@eirslett eirslett commented Jul 16, 2015

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?

@adriancole

This comment has been minimized.

Copy link
Contributor Author

@adriancole adriancole commented Jul 16, 2015

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.

@adriancole

This comment has been minimized.

Copy link
Contributor Author

@adriancole adriancole commented Jul 16, 2015

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?

@adriancole

This comment has been minimized.

Copy link
Contributor Author

@adriancole adriancole commented Jul 16, 2015

@rtyler rtyler added the enhancement label Jul 16, 2015
@michaelsembwever

This comment has been minimized.

Copy link
Member

@michaelsembwever michaelsembwever commented Jul 16, 2015

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
#451 (comment).

Mick Semb Wever
Scandinavia

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

@b

This comment has been minimized.

Copy link

@b b commented Jul 16, 2015

I agree, a separate issue is best.

#463

@michaelsembwever

This comment has been minimized.

Copy link
Member

@michaelsembwever michaelsembwever commented Jul 17, 2015

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

This comment has been minimized.

Copy link
Contributor

@eirslett eirslett commented Jul 17, 2015

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

This comment has been minimized.

Copy link
Contributor

@eirslett eirslett commented Jul 17, 2015

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

This comment has been minimized.

Copy link
Member

@michaelsembwever michaelsembwever commented Jul 17, 2015

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.

@adriancole

This comment has been minimized.

Copy link
Contributor Author

@adriancole adriancole commented Jul 17, 2015

@adriancole

This comment has been minimized.

Copy link
Contributor Author

@adriancole adriancole commented Jul 17, 2015

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:

  • scrooge finagle java support didn't work. I think eirik may have fixed
    this.
  • even if it did, Scala code uses the Scala types generated by scrooge.
  • finagle code tends to always return futures, and these are Twitter
    specific futures.
  • Twitter libraries tend to have a lot of deps and also one on a specific
    Scala runtime.
  • guava conversion seemed to be the only way out, though we know guava
    versions are sensitive for everyone.
  • current scala+finagle mix isn't java 8 friendly, so yeah no lambdas

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

  • when t-mux isn't used, other thrift impls can interop (Like Facebook
    swift)
  • this means you don't have to use scrooge, as you aren't trying to share
    libraries, rather types (or IDL)
  • this isn't that different when thinking about how to address Scala 2.12,
    java or another language.

On pushing thrift to an extension

  • thrift is often conflated with finagle and by extension Scala in Twitter
    codebases (even ones written in Java)
  • by using plain types and not using scrooge, we could open to codec that
    isn't thrift (ex json) and doesn't pull in vast dependency trees that are
    coupled to specific Scala versions.
  • since many prefer json or protobuf, this indirection is likely worth
    whatever conversion cost incurred
    • most build systems can run JMH, so we can at least measure that

Hope this helps.

@adriancole

This comment has been minimized.

Copy link
Contributor Author

@adriancole adriancole commented Aug 3, 2015

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).

adriancole added a commit that referenced this issue Aug 13, 2015
This makes SpanStore an abstract class, and adds an implementation in
the java source tree to prove it can be written in that language.

SpanStore was a mixture of traits, which made it impossible to write in
Java. Moreover, eventhough all implementations close synchronously, it
mixed in Twitter's Closeable, which was both a trait and also async.

See #451
Closes #584
adriancole added a commit that referenced this issue Aug 13, 2015
This makes the critical storage interfaces, SpanStore and Aggregates,
abstract classes, and adds implementations in the java test tree to
prove they can compile with Java 7.

SpanStore was a mixture of traits, which made it impossible to write in
Java. Moreover, eventhough all implementations close synchronously, it
mixed in Twitter's Closeable, which was both a trait and also async.

Aggregates was a trait and implemented a synchronous close method
already.

See #451
Closes #584
@shakuzen

This comment has been minimized.

Copy link
Member

@shakuzen shakuzen commented May 9, 2019

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

@shakuzen shakuzen closed this May 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
You can’t perform that action at this time.