Migrate Netty 4.x client instrumentations (except CONNECT) to Instrum… #4381
Conversation
mateuszrzeszutek
requested review from
anuraaga,
iNikem,
jkwatson,
laurit,
pavolloffay,
trask and
tylerbenson
as code owners
| @@ -139,9 +139,4 @@ class Netty40ClientTest extends HttpClientTest<DefaultFullHttpRequest> implement | |||
| boolean testHttps() { | |||
| false | |||
| } | |||
|
|
|||
| @Override | |||
| boolean testReadTimeout() { | |||
There was a problem hiding this comment.
Now that we've switched over to using NetClientAttributesExtractor in netty instrumentations the net.peer.* attributes are no longer captured (channel.remoteAddress() returns null in case of timeout) and this test case fails.
There are no more tests in any instrumentations that enable it - we should verify how other clients behave and, if the lack of net.* attributes is a common thing, change the test scenario to reflect that. I'll create a separate issue for this once this PR gets merged.
There was a problem hiding this comment.
This test was added for #3558 I'm sure we have other instrumentations that behave the same way, when there is an error they can't provide all attributes because response isn't available. To me this really doesn't seem ideal. I'd assume that having attributes on errored request might be more useful than having them on successful requests as these are the ones that someone might actually try to inspect. #4287 removed setting net attributes in onStart, would restoring the old behaviour make this test work?
There was a problem hiding this comment.
#4287 removed setting net attributes in onStart, would restoring the old behaviour make this test work?
Yep, that should work.
Also, for a broader picture, note that exception & http request attributes are being set, it's only net attributes that are missing.
There was a problem hiding this comment.
I'll create a separate issue for this once this PR gets merged.
👍
btw, is the problem that Channel.remoteAddress() returns null in onEnd (while it returns non-null earlier in onStart)?
There was a problem hiding this comment.
btw, is the problem that
Channel.remoteAddress()returns null inonEnd(while it returns non-null earlier inonStart)?
Yep, it's exactly that. I believe that we can do 2 things to fix that:
- change the assertions in the timeout test so that they don't expect
net.*attributes - this has a downside of losing some attributes that, as Lauri said, might be incredibly useful on a failing request span; - always capture
net.*attributes on start - which would probably force us to re-think the net attributes extractors naming; it is also worth noticing that there were some HTTP clients that reportednet*attributes on start, so it could be worth implementing anyway.
I think I'm leaning towards option 2, and just simply reusing the server net attributes extractor in this PR.
There was a problem hiding this comment.
One more way would be to capture remote address when it is still available and just use it in onEnd. I suspect that always capturing in onStart isn't good either as clients only know an URI there and resolve an InetSocketAddress later on.
...entelemetry/javaagent/instrumentation/netty/v4_1/client/HttpClientRequestTracingHandler.java
Show resolved
Hide resolved
open-telemetry#4381) * Migrate Netty 4.x client instrumentations (except CONNECT) to Instrumenter API * Revert testReadTimeout changes
…enter API
Connection spans are not included in this PR, I'll migrate them separately to make those PRs a bit easier to review.
Part of #2713