Tag WSClient requests with information about requests

[simao]

This adds more information to the span used by TracedWSClient.

I added a test, but instantiating another application seems to break ZipkinModuleSpec, which uses Tracing.current. Each test is green when running them separately. I could try changing that to something else but didn't want to change that spec, not sure how to proceed here?

Thanks!

[simao]

Also, the test I added is failing here because ti depends on https://github.com/openzipkin-contrib/play-zipkin-tracing/pull/53

[codefromthecrypt]

sorry I meant like this.

https://github.com/openzipkin/brave/blob/master/brave/src/main/java/brave/Span.java#L184

I expect play should have access to precise endpoint details but I also might be wrong.

another way is to simply not add the host tag by default and instead make it optional. There was a prior effort to use http abstraction here, but it fizzled out.

On Wed, Jul 10, 2019, 9:32 PM Simão Mata notifications@github.com wrote:

@simao commented on this pull request.

In play-zipkin-tracing/play/src/main/scala/brave/play/TraceWSClient.scala https://github.com/openzipkin-contrib/play-zipkin-tracing/pull/54#discussion_r302039304 :

@@ -65,12 +65,22 @@ private class TraceWSRequest(spanName: String, request: WSRequest, tracer: Zipki override def withUrl(url: String): TraceWSRequest = new TraceWSRequest(spanName, request.withUrl(url), tracer, traceData) override def withMethod(method: String): TraceWSRequest = new TraceWSRequest(spanName, request.withMethod(method), tracer, traceData)

• override def execute(): Future[Response] = tracer.traceFuture(spanName){ data =>
• request.addHttpHeaders(tracer.toMap(data.span).toSeq: _*).execute()
• }(traceData)
• override def stream(): Future[Response] = tracer.traceFuture(spanName){ data =>
• request.addHttpHeaders(tracer.toMap(data.span).toSeq: _*).stream()
• }(traceData)
• private def executeTraced(executeRequestFn: WSRequest => Future[Response]): Future[Response] = {
• val tracingTags = List(
• "http.method" -> this.method,
• "http.path" -> this.uri.getPath,
• "http.host" -> this.uri.getHost

you mean we should call remoteServiceName in ZipkinTraceServiceLike ? Because right now it just calls name on the span and I see remoteEndpoint is deprecated.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/openzipkin-contrib/play-zipkin-tracing/pull/54?email_source=notifications&email_token=AAAPVV4AG2FYANPZN5EFOJ3P6XJFHA5CNFSM4H7OX4K2YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOB6ADHLA#discussion_r302039304, or mute the thread https://github.com/notifications/unsubscribe-auth/AAAPVVZJW2WVKA3QF2TCZM3P6XJFHANCNFSM4H7OX4KQ .

[simao]

@adriancole Thanks for checking my PRs, I won't have time in the next few days but I will work a bit more on this asap.