Add tracing support for Apache HttpAsyncClient #84

HaloFour · 2018-03-01T15:19:05Z

Adds trace-friendly helper wrapper around Apache HttpAsyncClient. This is an NIO-based HTTP client which executes the requests asynchronously and uses callback interfaces in order to notify of completion. The helper class wraps said callback interfaces in order to intercept the HttpRequest and HttpResponse objects in order to instrument them as well as to restore the parent tracing span on the callback thread.

I've not yet looked into AspectJ instrumentation of HttpAsyncClient.

This PR also includes a general-purpose mechanism for capturing the current tracing state that can then be restored on a separate thread. This is following the pattern established in TraceFriendlyThreadPoolExecutor.

…tation

CLAassistant · 2018-03-01T15:19:12Z

All committers have signed the CLA.

pauljamescleary · 2018-03-01T16:11:16Z

Thanks for this! Will review this week. Looks like travis is unhappy, so should fix that first.

codecov-io · 2018-03-01T17:03:26Z

Codecov Report

Merging #84 into master will increase coverage by 0.21%.
The diff coverage is 93.13%.

@@            Coverage Diff            @@
##           master     #84      +/-   ##
=========================================
+ Coverage   90.98%   91.2%   +0.21%     
=========================================
  Files          30      33       +3     
  Lines         599     682      +83     
  Branches       57      56       -1     
=========================================
+ Hits          545     622      +77     
- Misses         54      60       +6

Impacted Files	Coverage Δ
...ain/scala/com/comcast/money/core/state/State.scala	`100% <100%> (ø)`
...mcast/money/http/client/HttpAsyncTraceConfig.scala	`50% <50%> (ø)`
...e/concurrent/TraceFriendlyThreadPoolExecutor.scala	`78.57% <66.66%> (-11.91%)`	⬇️
...st/money/http/client/TraceFriendlyHttpClient.scala	`88.23% <80%> (-3.2%)`	⬇️
...ney/http/client/TraceFriendlyHttpAsyncClient.scala	`97.29% <97.29%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6397144...5381eb7. Read the comment docs.

HaloFour · 2018-03-03T00:25:50Z

When working on the unit tests for the aspect intercepting HttpAsyncClient calls I noticed something about how spans work in asynchronous contexts. I had followed the design established in TraceFriendlyThreadPoolExecutor where the current span and MDC are captured and then restored onto the worker thread on which the execution will continue. However, this design assumes that the span submitting the request will remain open for the duration. When instrumented via @Traced annotations the span is closed when the method returns. That might work fine in a strictly fork-join model, but I envision using this in a reactive model, where the method coordinating the HTTP request returns almost immediately with a CompletionStage<T>/Completable<T>/Mono<T>/Observable<T> representing the result.

On the surface it would seem that the appropriate route here would be to have the instrumentation automatically create a child span of the current span. Per @kristomasette the child span can continue to trace after the parent span is closed and that you can correlate the children back to their parent. This seems like it will work, although I'll have to think of a good way to derive a name for the child span.

I'd like to mull over the nature of spans and tracing in such an asynchronous context, though, to see if a different approach might fit better. For the sake of comparison, with the NewRelic API in the context of a transaction (what they call a span) you can obtain a reference of a "token" to that transaction. You can then link any method on any thread back to that transaction. The transaction would maintain a count of these tokens and would only consider the transaction complete when both it was closed and all of the tokens have been expired.

The reason I'd like to explorer this further is because I'd like to look into adding more modules for adding instrumentation around common libraries like RxJava and Spring Reactor. Kris has also mentioned wanting to explore better tracing across Akka-actor, perhaps correlating activity in a similar manner to the X-MoneyTrace HTTP header.

pauljamescleary · 2018-03-09T16:49:18Z

money-http-client/src/main/scala/com/comcast/money/http/client/TraceFriendlyHttpClient.scala

-      wrapee.asInstanceOf[Closeable].close()
-    else if (wrapee.isInstanceOf[AutoCloseable])
-      wrapee.asInstanceOf[AutoCloseable].close()
+    wrapee match {


nice cleanup. I think money in general needs a refresh / update because there is likely lots of opportunity for code cleanup

pauljamescleary · 2018-03-09T16:51:39Z

...async-client/src/main/scala/com/comcast/money/http/client/TraceFriendlyHttpAsyncClient.scala

+  override def getException: Exception = wrappee.getException
+  override def cancel(): Boolean = wrappee.cancel()
+  override def close(): Unit = wrappee.close()
+}


add a new line to close this file

pauljamescleary · 2018-03-09T16:53:22Z

...async-client/src/main/scala/com/comcast/money/http/client/TraceFriendlyHttpAsyncClient.scala

+
+  import com.comcast.money.http.client.TraceFriendlyHttpAsyncSupport._
+
+  val tracer = Money.Environment.tracer


would it make sense to pass in the tracer in the constructor?

I was following the convention already set by TraceFriendlyHttpClient. I don't mind breaking from that convention if it makes more sense. Perhaps overloaded constructors for both? Might make mocking easier rather than having to extend from the class which is what the unit tests currently do.

Sorry about what is likely some shaky precedent. I appreciate it that you tried to maintain consistency. But yea, would make things easier just passing it in.

Let me know if you can make that adjustment or if you need me to pair with you on this one.

Nah, I can get it. I was putting off working on this PR waiting on #86 as I think that could simplify some of what I'm doing here.

pauljamescleary

Thanks, sorry it took so long to review. A few questions but this is great!

pauljamescleary · 2018-10-21T14:07:30Z

@HaloFour circling back to this. I am readying a 0.9.0 release. Is this something that is still needed?

HaloFour · 2018-10-21T15:01:04Z

Sorry, this slipped off of my radar. Our team is currently in the process of moving to Java 11 on OpenJDK and my interest has turned to supporting the now built-in java.net.http.HttpClient class, which supports both sync and async operations. Do you think that this would PR would be valuable to others?

Speaking of JDK11, do you know if it's possible to support a project under money that supports the newer runtimes? Or would that involve updating all of the projects?

pauljamescleary · 2018-10-22T11:19:55Z

@HaloFour no worries on this PR, I was just asking if it was something that you guys specifically used. If not, we can add it after the 0.9.0 release in a dot release afterwards.

As far as JDK 11, I have updated all of our dependencies. This is rather significant:

removed support for Java 7
Java 8 is the default target environment now for 0.9.x+
updated all dependencies to be current
will likely remove support for Spring 3 as well, since that has been EOL for almost 2 years now.
in theory, we should be fine running in JDK 11 going forward. Should really update travis to do a dual build on jdk 11

ptravers · 2018-10-22T13:15:41Z

To support JDK 11 we might have to drop Scala 11 support?

HaloFour added 7 commits February 13, 2018 13:51

Initial support for Apache HttpAsyncClient

72d9242

Fix State and restore

42b8d9c

Allow restore to return a value

449b0a6

Fix Scala compilation errors (wrong version) and consolidate implemen…

30ad77d

…tation

Add unit tests for TraceFriendlyHttpAsyncClient

06791f6

Propagate span onto request producer and response consumer threads

b1df0ae

Minor clean-up and add scaladocs

1cf4264

HaloFour added 2 commits March 1, 2018 11:57

Add unit tests for callback interface wrappers

00793e3

Kick TravisCI

5381eb7

HaloFour mentioned this pull request Mar 8, 2018

Discussion: Asynchronous Tracing #85

Open

pauljamescleary reviewed Mar 9, 2018

View reviewed changes

pauljamescleary requested changes Mar 9, 2018

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add tracing support for Apache HttpAsyncClient #84

Add tracing support for Apache HttpAsyncClient #84

HaloFour commented Mar 1, 2018

CLAassistant commented Mar 1, 2018 •

edited

Loading

pauljamescleary commented Mar 1, 2018

codecov-io commented Mar 1, 2018 •

edited

Loading

HaloFour commented Mar 3, 2018

pauljamescleary Mar 9, 2018

pauljamescleary Mar 9, 2018

pauljamescleary Mar 9, 2018

HaloFour Mar 9, 2018

pauljamescleary Mar 10, 2018

pauljamescleary Mar 31, 2018

HaloFour Mar 31, 2018

pauljamescleary Apr 3, 2018

pauljamescleary left a comment

pauljamescleary commented Oct 21, 2018

HaloFour commented Oct 21, 2018

pauljamescleary commented Oct 22, 2018

ptravers commented Oct 22, 2018


		import com.comcast.money.http.client.TraceFriendlyHttpAsyncSupport._

		val tracer = Money.Environment.tracer

Add tracing support for Apache HttpAsyncClient #84

Are you sure you want to change the base?

Add tracing support for Apache HttpAsyncClient #84

Conversation

HaloFour commented Mar 1, 2018

CLAassistant commented Mar 1, 2018 • edited Loading

pauljamescleary commented Mar 1, 2018

codecov-io commented Mar 1, 2018 • edited Loading

Codecov Report

HaloFour commented Mar 3, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pauljamescleary left a comment

Choose a reason for hiding this comment

pauljamescleary commented Oct 21, 2018

HaloFour commented Oct 21, 2018

pauljamescleary commented Oct 22, 2018

ptravers commented Oct 22, 2018

CLAassistant commented Mar 1, 2018 •

edited

Loading

codecov-io commented Mar 1, 2018 •

edited

Loading