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

Adds http integration tests #32

Closed codefromthecrypt closed 11 months ago

codefromthecrypt commented 6 years ago

Many thanks to Kamon https://github.com/kamon-io/kamon-play/blob/master/kamon-play-2.6.x/src/test/scala/kamon/play/RequestInstrumentationSpec.scala as I used some approaches there to weld brave tests into this. It isn't a great job of it, but starting to work.

this test fails as the current span is not in scope, making the child a part of a different trace. I'm not sure where the scoping of the span is missing

codefromthecrypt commented 6 years ago

@takezoe will need a hand to get this one complete

codefromthecrypt commented 6 years ago

ps I have no idea how to get the executor working like I want... basically I want the test routes to use an executor like below.. Right now, all the routes cannot see the current span


class CurrentTraceContextExecutionContext(currentTraceContext: CurrentTraceContext, delegate: ExecutionContext) extends ExecutionContext {
  override def execute(runnable: Runnable) = delegate.execute(runnable)

  override def prepare() = new ExecutionContext {
    val invocationContext = currentTraceContext.get()

    def execute(r: Runnable) = delegate.execute(() => {
      val scope = currentTraceContext.newScope(invocationContext)
      try {
        r.run()
      } finally {
        scope.close()
      }
    })

    override def reportFailure(cause: Throwable) = delegate.reportFailure(cause)
  }

  override def reportFailure(cause: Throwable) = delegate.reportFailure(cause)
}
codefromthecrypt commented 6 years ago

I saw this, which might be helpful, just would be nice to inject the Tracing component into it vs looking up the current one each time https://gist.github.com/yanns/f7da61c582ab1da3535d

codefromthecrypt commented 6 years ago

@takezoe FYI I think this code is ready for review. What we do about implicit trace context propagation can be a separate change

takezoe commented 6 years ago

@adriancole Sorry for my slow response. I always thank for your great work! I'll look at this pull request this weekend.

codefromthecrypt commented 6 years ago

thanks @takezoe I added a commit which incorporate's brave's http.route based naming. This doesn't work because I currently can't read the route.

codefromthecrypt commented 6 years ago

Oh that is very interesting. Maybe i can change this to reading a literal for the routes file? Unless there is some way to simulate that otherwise.

Very good find

codefromthecrypt commented 5 years ago

@simao sorry I never finished this but it would have solved the two pull requests you have

simao commented 5 years ago

@adriancole Ok, so then what is the status of this and how could I help move this forward?