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!

[codefromthecrypt]

fyi this isn't usually tagged by default. remoteEndpoint is, though

[simao]

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

[simao]

Also, the test I added is failing here because ti depends on #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 ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In play-zipkin-tracing/play/src/main/scala/brave/play/TraceWSClient.scala <#54 (comment)> : > @@ -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 <#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.