search

openzipkin-contrib / play-zipkin-tracing

Provides distributed tracing for Play Framework and Akka using Zipkin.
Apache License 2.0
48 stars 19 forks source link

Support for Play 2.7.0 #42

Closed shimamoto closed 5 years ago

shimamoto commented 5 years ago

@takezoe Please take a review when you have time.

codefromthecrypt commented 5 years ago

maybe I can help with this.

On Sat, Mar 2, 2019, 1:21 PM Naoki Takezoe notifications@github.com wrote:

@takezoe approved this pull request.

I confirmed if it works with Play 2.7 by porting an existing sample application to play-zipkin-tracing 2.2.0-SNAPSHOT. 👍

[image: zipkin-play27] https://user-images.githubusercontent.com/1094760/53677604-0b41e500-3cf6-11e9-8478-401edf53bf2a.png

By the way, Zipkin1 version of play-zipkin-tracing doesn't work with the latest Zipkin server any more. Currently it instructs to use a play24 example for testing nested remote call, but it doesn't work. So I used a play26 example with tiny modification to test as above screenshot shows. We need to update "How to run sample projects" instruction in README as well.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/bizreach/play-zipkin-tracing/pull/42#pullrequestreview-209853627, or mute the thread https://github.com/notifications/unsubscribe-auth/AAD61w0_rSJbtF9lT9x5CkOBhbIVvQnFks5vSgpEgaJpZM4bHKow .

takezoe commented 5 years ago

I got an error below with the combination of a newer version of play-zipkin-tracing and an older version of play-zipkin-tracing in the older version side:

[warn] z.r.AsyncReporter$BoundedAsyncReporter - Dropped 1 spans due to IllegalStateException(response failed: Response{protocol=http/1.1, code=400, message=Bad Request, url=http://localhost:9411/api/v1/spans})
java.lang.IllegalStateException: response failed: Response{protocol=http/1.1, code=400, message=Bad Request, url=http://localhost:9411/api/v1/spans}
    at zipkin.reporter.okhttp3.OkHttpSender$CallbackAdapter.onResponse(OkHttpSender.java:262) [zipkin-sender-okhttp3-0.10.0.jar:na]
    at okhttp3.RealCall$AsyncCall.execute(RealCall.java:141) [okhttp-3.8.0.jar:na]
    at okhttp3.internal.NamedRunnable.run(NamedRunnable.java:32) [okhttp-3.8.0.jar:na]
    at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1149) [na:1.8.0_191]
    at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:624) [na:1.8.0_191]
    at java.lang.Thread.run(Thread.java:748) [na:1.8.0_191]
[warn] z.r.AsyncReporter$BoundedAsyncReporter - Dropped 2 spans due to IllegalStateException(response failed: Response{protocol=http/1.1, code=400, message=Bad Request, url=http://localhost:9411/api/v1/spans})
java.lang.IllegalStateException: response failed: Response{protocol=http/1.1, code=400, message=Bad Request, url=http://localhost:9411/api/v1/spans}
    at zipkin.reporter.okhttp3.OkHttpSender$CallbackAdapter.onResponse(OkHttpSender.java:262) [zipkin-sender-okhttp3-0.10.0.jar:na]
    at okhttp3.RealCall$AsyncCall.execute(RealCall.java:141) [okhttp-3.8.0.jar:na]
    at okhttp3.internal.NamedRunnable.run(NamedRunnable.java:32) [okhttp-3.8.0.jar:na]
    at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1149) [na:1.8.0_191]
    at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:624) [na:1.8.0_191]
    at java.lang.Thread.run(Thread.java:748) [na:1.8.0_191]

We have two applications to test nested API call tracing. The first one is Play 2.7 application with the latest version of play-zipkin-tracing. It calls the second one that is Play 2.4 application with the older version of play-zipkin-tracing which supports only zipkin-v1. Zipkin server I tested is zipkin-server-2.12.3-exec.jar.

@adriancole Is this correct behavior?

codefromthecrypt commented 5 years ago

problem is that we have text in the http body that would explain why it dropped. I think we need to update the zipkin-reporter library to include that in the error message. Usually it is when you send v2 format to the v1 endpoint. cc @shakuzen

codefromthecrypt commented 5 years ago

@takezoe I have narrowed this down to the first byte that is supposed to be 12 (thrift list), but gzipping back as 11.

takezoe commented 5 years ago

@adriancole Thanks for quick fix! In my understanding, we will able to handle requests from the v1 okhttp reporter if we use the next version of zipkin-server. Right?

Anyway, I think we can merge this pull request. @shimamoto

codefromthecrypt commented 5 years ago

I agree. very soon 2.12.4 will workaround the v1 okhttp reporter bug, something we never noticed before. There's no change needed here.

shimamoto commented 5 years ago

@takezoe @adriancole many thanks!