#4056 remove oringin handler when removelast in netty #4123
Conversation
tydhot
requested review from
anuraaga,
iNikem,
jkwatson,
laurit,
mateuszrzeszutek,
pavolloffay,
trask and
tylerbenson
as code owners
| } else if (handler | ||
| .getClass() | ||
| .getName() | ||
| .startsWith("io.opentelemetry.javaagent.instrumentation.netty.")) { | ||
| pipeline.removeLast(); |
There was a problem hiding this comment.
Hmm, what if somebody calls pipeline.addAfter(originalHandler, ...)? Then we'll remove some random custom handler that has nothing to do with our instrumentation. If we want this method to work correctly we should
- Maintain a two-way association between the
originalHandlerandourHandler(i.e. putourHandler -> originalHandlerinto theContextStoretoo) and remove theoriginalHandlerwheneverremoveLast()removes our handler - Instrument
addAfter()so that it actually adds the new handler afterourHandlerwhenever it's associated to theoriginalHandler
There was a problem hiding this comment.
yes, u r right.Maybe we can instrument addAfter() to make sure ourhandler will always be added after the originalHandler. I'll have a try.
| isMethod().and(named("removeLast")).and(returns(named("io.netty.channel.ChannelHandler"))), | ||
| AbstractNettyChannelPipelineInstrumentation.class.getName() + "$RemoveLastAdvice"); | ||
| transformer.applyAdviceToMethod( | ||
| isMethod().and(named("addAfter")).and(takesArguments(3)), |
There was a problem hiding this comment.
The 3-args variant calls the 4-args one, it should be enough to instrument the 4-args one.
| @Advice.OnMethodExit(suppress = Throwable.class) | ||
| public static void addAfterHandler( | ||
| @Advice.This ChannelPipeline pipeline, | ||
| @Advice.Argument(value = 1, readOnly = false) String name) { |
There was a problem hiding this comment.
Can you add takesArgument(1, String.class) to the method matcher? In general we aim to enclose all types that we use in advice classes (arguments, return types) in the method matcher.
|
@mateuszrzeszutek thanks! It looked better now. But it seemed the check meet some problem now. |
| channelPipeline.first() == httpHandler | ||
| // our handler was also added | ||
| channelPipeline.last().getClass() == HttpClientTracingHandler |
There was a problem hiding this comment.
These 2 assertions are already present in the then: section.
There was a problem hiding this comment.
Same applies to the other test.
There was a problem hiding this comment.
thanks, should I remove then in this pr?
|
@mateuszrzeszutek you're welcome :) |
No description provided.