search

twitter / finagle

A fault tolerant, protocol-agnostic RPC system
https://twitter.github.io/finagle
Apache License 2.0
8.77k stars 1.45k forks source link

Please help Zipkin remove scribe #675

Open codefromthecrypt opened 6 years ago

codefromthecrypt commented 6 years ago

One of the last frameworks to require the scribe transport is Finagle. On the server-side scribe not only is an archived architecture, but pins to abandoned libraries. It would help the greater OpenZipkin ecosystem a great deal if Finagle could stop using scribe as a default. This doesn't just impact finagle frameworks, but others like linkerd. We simply can't afford to do scribe forever.

A suggestion would be to change the approach to http here (not relying on zipkin-finagle project). Because not everyone uses zipkin-finagle repo, this would allow a sustainable option here. Then, via deprecation or otherwise, we could move people off scribe and stop the serious weight this has on zipkin projects.

(did I plead emotionally enough? :) )

mosesn commented 6 years ago

@adriancole we're going to talk to folks internally about whether we can move off the scribe transport immediately, or if we'd have to figure out some kind of support strategy for scribe.

isabelmartin commented 6 years ago

@adriancole Hi, would you mind clarifying what you're hoping for a bit? As far as I understand it, Finagle doesn't use scribe by default, but you can using the finagle-zipkin module which will use scribe if you choose to depend on it. And if you wouldn't mind explaining, which part of maintaining scribe is burdensome to OpenZipkin?

codefromthecrypt commented 6 years ago

Hi. There is no working tracer library in finagle except scribe. If there was http then folks who don't depend on non finagle libs (such as zipkin-finagle) can operate without using scribe.

Practically speaking, very few know what scribe is or have experience with troubleshooting it. You will find people making remote connections to zipkin over scribe as if this is a normal rpc (not a log arch). These connections are not able to take advantage of intermediaries such as https proxies etc.

Zipkin server is not written in finagle, and depending on finagle is quite a large dependency and easy to conflict with other code. Apache libthrift isnt a capable impl. The next best is FB swift service, but scribe was archived ages ago. This still uses netty 3. The encoding uses an old format which is itself wrapped in a base64 variant. This has caused compat glitches with others.

Keep in mind I am the only fulltime maintainer of zipkin and volunteers usually have no interest in maintaining abandoned tech or dancing around conflicts due to it.

When at twitter I understand scribe was still "good enough" or too much a mountain to move, but if we are honest it is dead arch that yes some use but in no way helps with most folks sites. For the wider ecosystem outside twitter, I ask to get rid of it or at least add a finagle native http option by default. A dep literally called zipkin anchors people to keep using scribe and something we can change.

If 2018 is too hard even starting towards 2019 would be helpful, looking for some hope here.

On 22 Feb 2018 6:25 am, "Isabel Martin" notifications@github.com wrote:

@adriancole https://github.com/adriancole Hi, would you mind clarifying what you're hoping for a bit? As far as I understand it, Finagle doesn't use scribe by default, but you can using the finagle-zipkin module which will use scribe if you choose to depend on it. And if you wouldn't mind explaining, which part of maintaining scribe is burdensome to OpenZipkin?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/twitter/finagle/issues/675#issuecomment-367497330, or mute the thread https://github.com/notifications/unsubscribe-auth/AAD61x3fTb6LN3J_Aj30xuhvujSvRv66ks5tXJfjgaJpZM4SJJHB .

mosesn commented 6 years ago

@adriancole would changing the name to "finagle-zipkin-scribe" and adding some documentation fit what you're looking for? It seems like unnecessary work for us to add our own http tracer implementation when zipkin-finagle already has one.

codefromthecrypt commented 6 years ago

@adriancole https://github.com/adriancole would changing the name to "finagle-zipkin-scribe" and adding some documentation fit what you're looking for? It seems like unnecessary work for us to add our own http tracer implementation when zipkin-finagle already has one.

I still think people will use this because it comes from the authoritative repo, don't you? How much maintenance are you expecting from the http variant? Is there a way we can help? We've been trying not to rely too much on your help for things (I've helped with other things in finagle as you probably recall)

codefromthecrypt commented 6 years ago

PS what you mentioned would be an improvement as breaking the artifact ID can alert folks they should be looking elsewhere. I do think people will still use scribe anyway, because of the mild inconvenience of version skew and different group ID but this could certainly help (and is trackable via maven central stats)

mosesn commented 6 years ago

OK, so to be clear on what your concern is:

  1. finagle users today default to finagle-zipkin because it sounds like the "right thing".
  2. finagle-zipkin uses the scribe transport, which means that you feel like you should continue supporting scribe, even though it's a burden to maintain it.
  3. you want to stop supporting scribe, but you first want finagle-zipkin to no longer seem like "the right thing".

Is that right?

codefromthecrypt commented 6 years ago

one clarification: if finagle-zipkin could become the right thing (ie package http transport to let people switch over) this would be ideal.

If there's only one working transport upstream, even if people know it isn't the right thing, they likely will still use it. However, knowing it isn't the right thing is an improvement. So yes, if finagle-zipkin must be scribe, and only scribe, let's somehoe make it no longer seem like "the right thing".

mosesn commented 6 years ago

@adriancole it looks like in the short term, we can rename finagle-zipkin to finagle-zipkin-scribe, and we'll update our docs to refer to zipkin-finagle. In the long term we'll migrate off of it and delete finagle-zipkin-scribe. When that happens, we'll either supply a new transport that's whatever twitter is using internally, or we'll stop exporting our own Tracing facade and will migrate to a common facade, like opentracing or zipkin-reporter. Does that seem reasonable?

codefromthecrypt commented 6 years ago

@adriancole https://github.com/adriancole it looks like in the short term, we can rename finagle-zipkin to finagle-zipkin-scribe, and we'll update our docs to refer to zipkin-finagle.

ok and to be clear you are actively disinterested in an out of the box, even if community contributed finagle-zipkin-http?

In the long term we'll migrate off of it and delete finagle-zipkin-scribe. When that happens, we'll either supply a new transport that's whatever twitter is using internally, or we'll stop exporting our own Tracing facade and will migrate to a common facade, like opentracing or zipkin-reporter. Does that seem reasonable?

I don't think ot has anything to do with this unless you are suggesting yanking the entire tracing api in finagle, which is overkill for the transport decision IMHO. Reason is that OT does not have a data structure; it is a front-end api not an exporter interface like zipkin-reporter is. To export via OT will be a dramatic redo, but also up to you. If you are doing a complete rewrite you should also consider census which is more RPC in nature.

On Tue, Feb 27, 2018 at 3:50 AM, Moses Nakamura notifications@github.com wrote:

@adriancole https://github.com/adriancole it looks like in the short term, we can rename finagle-zipkin to finagle-zipkin-scribe, and we'll update our docs to refer to zipkin-finagle. In the long term we'll migrate off of it and delete finagle-zipkin-scribe. When that happens, we'll either supply a new transport that's whatever twitter is using internally, or we'll stop exporting our own Tracing facade and will migrate to a common facade, like opentracing or zipkin-reporter. Does that seem reasonable?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/twitter/finagle/issues/675#issuecomment-368627255, or mute the thread https://github.com/notifications/unsubscribe-auth/AAD6190utMSR-jfeGfQSlwM3k6nOid-Bks5tYwsDgaJpZM4SJJHB .

mosesn commented 6 years ago

I think we'd be open to a community contributed finagle-zipkin-http if we had a commitment from the community to help support it, although it seems a bit redundant, given that zipkin-finagle http already exists. Would you be willing to commit to reviewing PRs that come in for finagle-zipkin-http? You've been helpful with reviewing other zipkin-related PRs, so I don't think this would be a big shift.

OK, that's good feedback. We haven't looked too seriously at any of this stuff. I'll mention census in our internal ticket.

We've renamed finagle-zipkin to finagle-zipkin-scribe, so the next step is to change the docs to mention zipkin-finagle.

codefromthecrypt commented 6 years ago

I think we'd be open to a community contributed finagle-zipkin-http if we had a commitment from the community to help support it, although it seems a bit redundant, given that zipkin-finagle http already exists. Would you be willing to commit to reviewing PRs that come in for finagle-zipkin-http? You've been helpful with reviewing other zipkin-related PRs, so I don't think this would be a big shift.

In ideal world I think finagle-zipkin-http doesn't need to exist in, but practically speaking as people do download things and used what's pack-in, if a pack-in exists and that's only scribe, I hypothesize people will start downloading it (we can look at stats later to see how wrong I am :) ).

If there was no pack-in, it would be more clear.. it is just where we are. Having a pack-in that is http and works with tech that can consume zipkin data (like jaeger, datadog, google cloud, aws).. that's worth doing eventhough I have little time. If at some point scribe is removed, then we can also consider removing the http transport (for the zipkin-finagle repo). It is really about this transition period and making sure that we aren't discouraging a transition. I'll also try to find someone else to help reviewing this stuff.

OK, that's good feedback. We haven't looked too seriously at any of this stuff. I'll mention census in our internal ticket.

:thumbsup: thanks for listening

We've renamed finagle-zipkin to finagle-zipkin-scribe, so the next step is to change the docs to mention zipkin-finagle.

thank you for the help!

codefromthecrypt commented 6 years ago

PS sorry I didn't think about this earlier, but... let's indeed hold off on added http here.

Here's why: now that the artifact ID is changed, you should be getting maven central stats about it. If the stats don't change from before the artifact ID change, we could sense the rename isn't helping folks from resisting the pack-in. We can also cross this with zipkin-finagle stats. Presuming you have access, how about we review stats in a few months to see if there have been any impacts and take it from there?

mosesn commented 6 years ago

Works for me, thanks for following up!