Implement HTTP resend spec for OkHttp 3 #7652
Conversation
Extracted from #7652 - I figured this'll be useful on its own in some of the POCs/prototypes that we'll make
mateuszrzeszutek
force-pushed
the
http-retries
branch
from
5ef5d7e
to
32cd926
Compare
| if (emitEncompassingSpan) { | ||
| builder.interceptors().add(1, new EncompassingSpanInterceptor()); | ||
| } else if (createSpanForConnectionErrors) { | ||
| builder.interceptors().add(1, new ConnectionErrorSpanInterceptor(instrumenter)); | ||
| } |
There was a problem hiding this comment.
@trask @lmolkova I added two different ways of handling the connection stuff to this PR:
- the first one is the wrapper span
- the second one is a minor hack that only creates an HTTP span when there was an error thrown and no attempt to send an HTTP request was made - this way of handling things might not need any new spec (?)
There was a problem hiding this comment.
There are some example traces for both of these in the OkHttp3Test class
mateuszrzeszutek
force-pushed
the
http-retries
branch
from
32cd926
to
20e318a
Compare
|
This PR is now ready for review. Outstanding TODO items left for future PRs:
|
.../testing/src/main/java/io/opentelemetry/instrumentation/okhttp/v3_0/AbstractOkHttp3Test.java
Show resolved
Hide resolved
.../testing/src/main/java/io/opentelemetry/instrumentation/okhttp/v3_0/AbstractOkHttp3Test.java
Show resolved
Hide resolved
.../testing/src/main/java/io/opentelemetry/instrumentation/okhttp/v3_0/AbstractOkHttp3Test.java
Show resolved
Hide resolved
...-3.0/library/src/main/java/io/opentelemetry/instrumentation/okhttp/v3_0/OkHttpTelemetry.java
Show resolved
Hide resolved
| @@ -56,7 +47,7 @@ public OkHttpTelemetryBuilder addAttributesExtractor( | |||
| */ | |||
| @CanIgnoreReturnValue | |||
| public OkHttpTelemetryBuilder setCapturedRequestHeaders(List<String> requestHeaders) { | |||
| httpAttributesExtractorBuilder.setCapturedRequestHeaders(requestHeaders); | |||
| this.capturedRequestHeaders = requestHeaders; | |||
There was a problem hiding this comment.
nit: reassigning the reference allows the caller to later manipulate the list. It's silly/pedantic for a builder, I know, but prefer making the field final and just adding the to list. Could even rename to addCapturedRequestHeaders() if that's nicer (and avoids a clear/add race).
Same applies to response headers.
There was a problem hiding this comment.
reassigning the reference allows the caller to later manipulate the list.
Great point! Will fix that, thanks
but prefer making the field final and just adding the to list. Could even rename to
addCapturedRequestHeaders()if that's nicer (and avoids a clear/add race).
I think at some point we've decided to have a setter method (not an "adder") in all HTTP instrumentations, as it resulted in simpler API. We can revisit that discussion, but we'd have to do it for all HTTP instrumentations.
...brary/src/main/java/io/opentelemetry/instrumentation/okhttp/v3_0/OkHttpTelemetryBuilder.java
Outdated
Show resolved
Hide resolved
mateuszrzeszutek
force-pushed
the
http-retries
branch
from
fbc0042
to
3019622
Compare
|
FYI @open-telemetry/java-instrumentation-approvers I'm planning to merge this PR this week |
|
(cc @lmolkova) |
Part of #5722
It's a draft because it unfortunately breaks the "connection error" scenarios. Establishing the connection in OkHttp happens before sending the HTTP request (duh), and the instrumentation reflects that - there's no HTTP client span around opening a connection.
Perhaps we should spec the "informal"
CONNECTspans that we have in some of our instrumentations? This issue is bound to surface in other HTTP client instrumentations as well, and the telemetry around opening a connection is super useful in failure scenarios.And, we have to rewrite all the resend/retry tests, but I've left that for another PR.