-
Notifications
You must be signed in to change notification settings - Fork 828
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
#4056 remove oringin handler when removelast in netty #4123
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -11,6 +11,7 @@ | |
import static net.bytebuddy.matcher.ElementMatchers.named; | ||
import static net.bytebuddy.matcher.ElementMatchers.returns; | ||
import static net.bytebuddy.matcher.ElementMatchers.takesArgument; | ||
import static net.bytebuddy.matcher.ElementMatchers.takesArguments; | ||
|
||
import io.netty.channel.ChannelHandler; | ||
import io.netty.channel.ChannelPipeline; | ||
|
@@ -47,11 +48,18 @@ public void transform(TypeTransformer transformer) { | |
transformer.applyAdviceToMethod( | ||
isMethod().and(named("remove").or(named("replace"))).and(takesArgument(0, Class.class)), | ||
AbstractNettyChannelPipelineInstrumentation.class.getName() + "$RemoveByClassAdvice"); | ||
transformer.applyAdviceToMethod( | ||
isMethod().and(named("removeFirst")).and(returns(named("io.netty.channel.ChannelHandler"))), | ||
AbstractNettyChannelPipelineInstrumentation.class.getName() + "$RemoveFirstAdvice"); | ||
transformer.applyAdviceToMethod( | ||
isMethod().and(named("removeLast")).and(returns(named("io.netty.channel.ChannelHandler"))), | ||
AbstractNettyChannelPipelineInstrumentation.class.getName() + "$RemoveLastAdvice"); | ||
transformer.applyAdviceToMethod( | ||
isMethod() | ||
.and(named("removeFirst").or(named("removeLast"))) | ||
.and(returns(named("io.netty.channel.ChannelHandler"))), | ||
AbstractNettyChannelPipelineInstrumentation.class.getName() + "$RemoveFirstLastAdvice"); | ||
.and(named("addAfter")) | ||
.and(takesArgument(1, String.class)) | ||
.and(takesArguments(4)), | ||
AbstractNettyChannelPipelineInstrumentation.class.getName() + "$AddAfterAdvice"); | ||
} | ||
|
||
@SuppressWarnings("unused") | ||
|
@@ -114,7 +122,23 @@ public static void removeHandler( | |
} | ||
|
||
@SuppressWarnings("unused") | ||
public static class RemoveFirstLastAdvice { | ||
public static class RemoveFirstAdvice { | ||
|
||
@Advice.OnMethodExit(suppress = Throwable.class) | ||
public static void removeHandler( | ||
@Advice.This ChannelPipeline pipeline, @Advice.Return ChannelHandler handler) { | ||
ContextStore<ChannelHandler, ChannelHandler> contextStore = | ||
InstrumentationContext.get(ChannelHandler.class, ChannelHandler.class); | ||
ChannelHandler ourHandler = contextStore.get(handler); | ||
if (ourHandler != null) { | ||
pipeline.remove(ourHandler); | ||
contextStore.put(handler, null); | ||
} | ||
} | ||
} | ||
|
||
@SuppressWarnings("unused") | ||
public static class RemoveLastAdvice { | ||
|
||
@Advice.OnMethodExit(suppress = Throwable.class) | ||
public static void removeHandler( | ||
|
@@ -125,6 +149,30 @@ public static void removeHandler( | |
if (ourHandler != null) { | ||
pipeline.remove(ourHandler); | ||
contextStore.put(handler, null); | ||
} else if (handler | ||
.getClass() | ||
.getName() | ||
.startsWith("io.opentelemetry.javaagent.instrumentation.netty.")) { | ||
pipeline.removeLast(); | ||
} | ||
} | ||
} | ||
|
||
@SuppressWarnings("unused") | ||
public static class AddAfterAdvice { | ||
|
||
@Advice.OnMethodEnter(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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you add There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yes, of course |
||
ChannelHandler handler = pipeline.get(name); | ||
if (handler != null) { | ||
ContextStore<ChannelHandler, ChannelHandler> contextStore = | ||
InstrumentationContext.get(ChannelHandler.class, ChannelHandler.class); | ||
ChannelHandler ourHandler = contextStore.get(handler); | ||
if (ourHandler != null) { | ||
name = ourHandler.getClass().getName(); | ||
} | ||
} | ||
} | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 shouldoriginalHandler
andourHandler
(i.e. putourHandler -> originalHandler
into theContextStore
too) and remove theoriginalHandler
wheneverremoveLast()
removes our handleraddAfter()
so that it actually adds the new handler afterourHandler
whenever it's associated to theoriginalHandler
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, u r right.Maybe we can instrument
addAfter()
to make sureourhandler
will always be added after theoriginalHandler
. I'll have a try.