Separate span for input and output streams. #425
Conversation
tylerbenson
force-pushed
the
tyler/httpurlconnection
branch
from
e1fcc2f
to
064ae4c
Compare
| } | ||
| // We get here in cases where connect() is called first. | ||
| // We need to inject headers now because after connected=true no more headers can be | ||
| // added. |
There was a problem hiding this comment.
Am I reading the code correctly and header injection is no longer happening in this class? And in fact it happens deeper so this code really doesn't control they way headers are injected anymore. In fact as far as I can see all this switch is controlling is creation of multiple spans when connect/getInputSpan/getOutputSpan are called multiple times.
If above is correct you may want to rewrite this comment to better clarify what is happening here.
There was a problem hiding this comment.
Updated. Is it better?
| @@ -187,16 +187,9 @@ public static HttpURLState get(final HttpURLConnection connection) { | |||
| return state; | |||
| } | |||
|
|
|||
| public boolean hasDoneIO = false; | |||
| public boolean calledOutputStream = false; | |||
| public boolean calledInputStream = false; | |||
There was a problem hiding this comment.
Above comment says // not thread-safe, but neither is HttpURLConnection. I think this is only partially true: sun.net.www.protocol.http.HttpURLConnection seems to be thread-safe. That's probably not a huge issue in reality, but comment may be confusing.
There was a problem hiding this comment.
I made it threadsafe. Though I'm not sure if it's a problem synchronizing on the connection.
| final Tracer tracer = GlobalTracer.get(); | ||
| final Span span = tracer.activeSpan(); | ||
| // Use the active span, not the one created in SpanStartAdvice, | ||
| // since we aren't sure that span will complete. |
There was a problem hiding this comment.
Could you please clarify what this comment means and what is SpanStartAdvice it is referring to?
There was a problem hiding this comment.
Is the purpose of this instrumentation to add header injection in cases where connect() is called before get*Stream()?
There was a problem hiding this comment.
That comment is from a previous attempt at instrumenting the sun HttpClient further. I've removed it. Sorry for the confusion.
There was a problem hiding this comment.
Nice work!
Just to confirm:
- Do we only create
connectspans on the actual connection (when the connection is not cached)? - Does getInputStream do header injection in cases where the connection is cached (keep-alive) and
connect()is called beforegetInputStream()?
tylerbenson
force-pushed
the
tyler/httpurlconnection
branch
from
3d9b520
to
e7aa7c5
Compare
yes, it will create a connect span if connect is called. This is to ensure a span exists for propagation.
It shouldn't. It only does header injection right before the headers are written out to the socket, so in your question, it would be when connect is called. |
@realark I've separated the input and output streams as you requested.
I also pushed the
Tracer.injectpoint a bit deeper toHttpClient.writeRequestswhere we know that it's actually happening. Let me know if you'd rather I leave that how you had it.