open-telemetry / open-telemetry/opentelemetry-java-instrumentation

Netty instrumentation considered harmful

Open
#2,496 1 comment 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

bug
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

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. 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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.