Instrumentation of Google Http Client

[randomanderson]

Instruments Google Http Client.

Support for both synchronous and asynchronous requests

[tylerbenson]

Great job on your first integration!
I think the async instrumentation still needs some work. I can help out if you need.

[tylerbenson]

Since this is sync advice, I suggest returning the opened Scope the end of this method, then in the OnMethodExit, you can declare a @Advice.Enter method parameter reference that scope directly (and close it there). This way you don't need to use the contextStore.

[randomanderson]

Google does things different than other http clients in that there's no callback.
Paraphasing the code:

execute() { // do stuff }

Future executeAsync(Executor executor) {
   return executor.submit( this::execute);
}

The method advice around execute() happens in both modes, synchronous and asynchronous. And, in both modes, the span ends when execute returns.

The difference is whether the span starts on execute()-enter (synchronous case) or whether the span was started before execute()-enter (async case).

I'm using the contextStore to track the span that was created in the async method so that the same span can be populated when execute() is called

[tylerbenson]

Thanks for the summary. I was expecting a bit more from the framework in terms of async support. The status quo is certainly underwhelming.

Looking at the code, since the primary distinction is that it schedules the same work on a different thread instead of running directly, I can see a good argument that we should just start the span when the request is made, not when it gets scheduled. What do you think?

[tylerbenson]

I don't understand how this advice is "async". It seems to wrap the method called, but not handle the callback or such.

[randomanderson]

See above comment. In the normal case, methodExit of executeAsync() does nothing (execute() finishes the span). The only time methodExit needs to do something is if an exception was thrown when submitting to the executor: it finishes the span because execute() will never be called.

[tylerbenson]

This callback should be executed inside the async callback, not here...

[randomanderson]

There isn't a callback. Google's HttpClient returns a basic Future that you can call get() on. From the user's perspective, they only know the request is finished when get() returns.

[tylerbenson]

I think this is good. In the future we should reevaluate how to allocate the "queuing time" vs "network time".

[tylerbenson]

This is more of a stylistic thing, but I would have probably put these two options as nested classes of the parent class. Fine to leave as is though.

[tylerbenson]

Thanks for the summary. I was expecting a bit more from the framework in terms of async support. The status quo is certainly underwhelming.

Looking at the code, since the primary distinction is that it schedules the same work on a different thread instead of running directly, I can see a good argument that we should just start the span when the request is made, not when it gets scheduled. What do you think?