open-telemetry / open-telemetry/opentelemetry-java-instrumentation
Netty instrumentation considered harmful
Nobody has claimed this yet.
- Dominant language
- Java
- Stars
- 2.6k
- Forks
- 1.2k
- Avg merge
- 2d 18h
- Merged PRs (30d)
- 228
Description
First, take a look at a pair of HttpClientRequestTracingHandler/HttpClientResponseTracingHandler from out Netty client instrumentation.
Second, think what will happen when the following code executes:
Channel ch = ...
try(Scope scope = startNewSpanAndScope()) {
ch.writeAndFlush(...)
}
try(Scope scope = startNewSpanAndScope()) {
ch.writeAndFlush(...)
}
We have two outgoing requests on the same channel (request pipelining). Each one of them will install its own context as channel attribute (see HttpClientRequestTracingHandler around line 38). The second one will overwrite the first one. Thus HttpClientResponseTracingHandler will try to close second span twice and the first span will remain opened.
Netty does not do any request-response correlation. This is left for a higher level API's or protocols implementations. Which forces me to think that we should drop Netty instrumentation altogether and replace it with library-specific higher level API instrumentation.
Yes, this seems like a huge amount of work. But think about this. The purpose of Netty is to be an async low-level library. This means that, most probably, higher level http clients which use Netty are also async. Which means that we anyway have to write an integration code for them to transfer span context from user code, which initiates the http request, all the way down to Netty channel. So the amount of work needed for properly supporting Netty-based http clients does not actually change.
Contributor guide
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
Research direction
Read HttpClientRequestTracingHandler around line 38 and its paired HttpClientResponseTracingHandler. Reproduce the two pipelined writes described in the issue and trace how the channel attribute is overwritten. The issue proposes replacing low-level Netty instrumentation with library-specific higher-level integrations, but names no concrete entry points or tests; done would require preserving separate request-response span lifecycles.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- java
- Domain
- networking
- Issue type
- Refactor
- Difficulty
- 5/5
- Estimated time
- Over a week
- Activity status
- Stale
- Clarity
- Mostly clear
- Newbie friendliness
- 25/100