OkHttp tracing breaks when backlogged

[codefromthecrypt]

We attach state read by interceptors by controlling the OkHttp's Dispatcher's ExecutorService.

It works like this..

• attach a span to a thread local
• wrap an executor service which re-attaches the calling thread's span to a runnable
• make the OkHttp dispatcher use that executor service

This works for synchronous requests (since they don't use the dispatcher thread anyway) and normal asynchronous requests.

It doesn't work when there are more in-flight connections than permitted, because asynchronous requests are pushed into a ready queue before they are executed. The ready queue is not processed by the calling thread of the request, so the re-attach won't work. When an interceptor runs on a delayed request, it cannot see its calling span which breaks tracing.

I was thinking that maybe the application interceptor might not have this problem. If the host/connection limit was enforced (via the mentioned queue) at the network level, I would be able to coordinate the state by dual-registering an interceptor. This interceptor could re-attach the state without relying on thread locals by mapping application/network level requests.

Sadly, this doesn't work because the host/connection limit is enforced (via the mentioned queue) at the application level, so I'm stumped.

I would like to be able to trace requests especially when they are backlogged. Any ideas?

Hats off to @brianm for finding this problem, btw

here's a copy of this in stackoverflow as I'm not sure if this needs a change to okhttp or not https://stackoverflow.com/questions/41040727/propagating-state-even-when-backlogged

[brianm]

Our solution to this was to create our own Call.Factory which actually creates a new OkHttpClient for each call creation. The new client adds an interceptor that does the context propagation:

https://gist.github.com/brianm/5358a1aa3e39b0ffe3467e19e4ec740c

This might not be super palatable for something that tries to be transparent, but even the OkHttpClient api surface area isn't that big, so you could expose exactly that. We expose a Call.Factory with a couple things (like a newCall() with explicit context propagation instead of threadlocal/implicit)

[codefromthecrypt]

I think the short term way out of this is to make brave's current tracer apis accept an explicit parent (as opposed to requiring usage of thread locals). Future apis can sort the context propagation thing more elegantly.

[codefromthecrypt]

Brave 4.3's okhttp approach is a call factory which isn't subject to this problem