Make netty ChannelPipeline removeLast return user handler (#9584)

instrumentation/netty/netty-4-common/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/netty/v4/common/AbstractNettyChannelPipelineInstrumentation.java

@@ -141,23 +141,29 @@ public abstract class AbstractNettyChannelPipelineInstrumentation implements Typ
 
     @Advice.OnMethodExit(suppress = Throwable.class)
     public static void removeHandler(
-        @Advice.This ChannelPipeline pipeline, @Advice.Return ChannelHandler handler) {
+        @Advice.This ChannelPipeline pipeline,
+        @Advice.Return(readOnly = false) ChannelHandler handler) {
       VirtualField<ChannelHandler, ChannelHandler> virtualField =
           VirtualField.find(ChannelHandler.class, ChannelHandler.class);
       ChannelHandler ourHandler = virtualField.get(handler);
       if (ourHandler != null) {
-        pipeline.remove(ourHandler);
+        // Context is null when our handler has already been removed. This happens when calling
+        // removeLast first removed our handler and we called removeLast again to remove the http
+        // handler.
+        if (pipeline.context(ourHandler) != null) {
+          pipeline.remove(ourHandler);
+        }
         virtualField.set(handler, null);
       } else if (handler
           .getClass()
           .getName()
           .startsWith("io.opentelemetry.javaagent.instrumentation.netty.")) {
-        pipeline.removeLast();
+        handler = pipeline.removeLast();
       } else if (handler
           .getClass()
           .getName()
           .startsWith("io.opentelemetry.instrumentation.netty.")) {
-        pipeline.removeLast();
+        handler = pipeline.removeLast();
       }
     }
   }

instrumentation/netty/netty-4.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/netty/v4_0/NettyChannelPipelineInstrumentation.java

@@ -48,8 +48,7 @@ public class NettyChannelPipelineInstrumentation
 
   /**
    * When certain handlers are added to the pipeline, we want to add our corresponding tracing
-   * handlers. If those handlers are later removed, we may want to remove our handlers. That is not
-   * currently implemented.
+   * handlers. If those handlers are later removed, we may want to remove our handlers.
    */
   @SuppressWarnings("unused")
   public static class ChannelPipelineAddAdvice {

instrumentation/netty/netty-4.0/javaagent/src/test/groovy/ChannelPipelineTest.groovy

@@ -125,10 +125,12 @@ class ChannelPipelineTest extends AgentInstrumentationSpecification {
     channelPipeline.last().getClass().getSimpleName() == "HttpClientTracingHandler"
 
     when:
-    channelPipeline.removeLast()
+    def removed = channelPipeline.removeLast()
 
     then: "there is no handler in pipeline"
     channelPipeline.size() == 0
+    // removing tracing handler also removes the http handler and returns it
+    removed == httpHandler
   }
 
   private static class NoopChannelHandler extends ChannelHandlerAdapter {

instrumentation/netty/netty-4.1/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/netty/v4_1/NettyChannelPipelineInstrumentation.java

@@ -50,8 +50,7 @@ public class NettyChannelPipelineInstrumentation
 
   /**
    * When certain handlers are added to the pipeline, we want to add our corresponding tracing
-   * handlers. If those handlers are later removed, we also remove our handlers. Support for
-   * replacing handlers and removeFirst/removeLast is currently not implemented.
+   * handlers. If those handlers are later removed, we also remove our handlers.
    */
   @SuppressWarnings("unused")
   public static class ChannelPipelineAddAdvice {

instrumentation/netty/netty-4.1/javaagent/src/test/groovy/ChannelPipelineTest.groovy

@@ -123,10 +123,12 @@ class ChannelPipelineTest extends AgentInstrumentationSpecification {
     channelPipeline.last().getClass().simpleName == "HttpClientTracingHandler"
 
     when:
-    channelPipeline.removeLast()
+    def removed = channelPipeline.removeLast()
 
     then: "there is no handler in pipeline"
     channelPipeline.size() == 0
+    // removing tracing handler also removes the http handler and returns it
+    removed == httpHandler
   }
 
   private static class NoopChannelHandler extends ChannelHandlerAdapter {