Re: [PR] Add Spring RabbitMQ plugin [skywalking-java]

2026-03-12 Thread via GitHub 


wu-sheng merged PR #796:
URL: https://github.com/apache/skywalking-java/pull/796


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]

Re: [PR] Add Spring RabbitMQ plugin [skywalking-java]

2026-03-12 Thread via GitHub 


liuhaolong10 commented on PR #796:
URL: https://github.com/apache/skywalking-java/pull/796#issuecomment-4047428879

   Changes in this commit
   
   1. Fixed the batch message consumption issue in Spring AMQP
   Following the implementation of the Kafka plugin for batch message 
processing, I use the first message as the EntrySpan and associate trace 
information from the remaining messages with the EntrySpan.
   
   2. Added exclusion for DirectMessageListenerContainer
   Added the class check for DirectMessageListenerContainer$SimpleConsumer to 
the exclusion list in the RabbitMQ plugin to avoid duplicate traces.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]

Re: [PR] Add Spring RabbitMQ plugin [skywalking-java]

2026-03-11 Thread via GitHub 


wu-sheng commented on PR #796:
URL: https://github.com/apache/skywalking-java/pull/796#issuecomment-4043460824

   ### 🤖 Gemini (AI) Review Results for PR #796
   
   I have performed a deep-dive review of the implementation and identified two 
critical areas that could lead to runtime exceptions or duplicate traces in 
production environments.
   
    1. Batch Mode Safety in `SpringRabbitMQConsumerInterceptor`
   
   In `SpringRabbitMQConsumerInterceptor#beforeMethod`, `allArguments[1]` is 
directly cast to `org.springframework.amqp.core.Message`. However, in Spring 
AMQP's `AbstractMessageListenerContainer`, this argument (`data`) is an 
`Object` that can be a **`List`** when **Consumer-side Batching** is 
enabled.
   
   Refer to Spring AMQP's [`AbstractMessageListenerContainer.java` 
(L1734-L1744)](https://github.com/spring-projects/spring-amqp/blob/60759f20108398d57d770c069b2742965650170a/spring-rabbit/src/main/java/org/springframework/amqp/rabbit/listener/AbstractMessageListenerContainer.java#L1734-L1744):
   
   ```java
   // Logic in Spring AMQP's AbstractMessageListenerContainer.java
   protected void executeListener(Channel channel, Object data) {
   if (data instanceof Message) {
   doExecuteListener(channel, (Message) data);
   }
   else {
   try {
   // This is where List occurs!
   doExecuteListener(channel, (List) data);
   }
   catch (ClassCastException e) {
   // ...
   }
   }
   }
   ```
   
   **Recommendation:** Update the interceptor to handle `allArguments[1]` as an 
`Object` and check if it is a `List` before casting to avoid a 
`ClassCastException` in batch-mode applications.
   
   ---
   
    2. Duplicate Trace Risk for `DirectMessageListenerContainer` (DMLC)
   
   The current fix in `RabbitMQConsumerInterceptor.java` effectively excludes 
`SimpleMessageListenerContainer` (SMLC) by checking for 
`BlockingQueueConsumer$InternalConsumer`. However, Spring AMQP also provides 
the **`DirectMessageListenerContainer` (DMLC)**, which uses a different 
internal consumer class.
   
   In DMLC, the internal consumer is 
[`org.springframework.amqp.rabbit.listener.DirectMessageListenerContainer$SimpleConsumer`](https://github.com/spring-projects/spring-amqp/blob/main/spring-rabbit/src/main/java/org/springframework/amqp/rabbit/listener/DirectMessageListenerContainer.java#L1127).
   
   **The Risk:** Since `SimpleConsumer` is not in the exclusion list, the 
original `rabbitmq-plugin` will still create a "shallow" trace for DMLC users. 
Because DMLC executes the listener on the same RabbitMQ client thread, the new 
`spring-rabbitmq-plugin` will *also* create a trace, leading to **duplicate 
spans** or **broken trace segments**.
   
   **Recommendation:** Add the DMLC internal consumer to the exclusion list in 
`RabbitMQConsumerInterceptor.java`:
   
   ```java
   public static final String SMLC_INTERNAL_CONSUMER = 
"org.springframework.amqp.rabbit.listener.BlockingQueueConsumer$InternalConsumer";
   public static final String DMLC_INTERNAL_CONSUMER = 
"org.springframework.amqp.rabbit.listener.DirectMessageListenerContainer$SimpleConsumer";
   
   if (consumer != null) {
   String className = consumer.getClass().getName();
   if (SMLC_INTERNAL_CONSUMER.equals(className) || 
DMLC_INTERNAL_CONSUMER.equals(className)) {
   return;
   }
   }
   ```


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]

Re: [PR] Add Spring RabbitMQ plugin [skywalking-java]

2026-03-11 Thread via GitHub 


wu-sheng commented on code in PR #796:
URL: https://github.com/apache/skywalking-java/pull/796#discussion_r2921819673


##
apm-sniffer/apm-sdk-plugin/spring-plugins/spring-rabbitmq-plugin/src/main/java/org/apache/skywalking/apm/plugin/spring/rabbitmq/SpringRabbitMQConsumerInterceptor.java:
##
@@ -0,0 +1,85 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ *
+ */
+
+package org.apache.skywalking.apm.plugin.spring.rabbitmq;
+
+import java.lang.reflect.Method;
+import java.util.Map;
+
+import org.apache.skywalking.apm.agent.core.context.CarrierItem;
+import org.apache.skywalking.apm.agent.core.context.ContextCarrier;
+import org.apache.skywalking.apm.agent.core.context.ContextManager;
+import org.apache.skywalking.apm.agent.core.context.tag.Tags;
+import org.apache.skywalking.apm.agent.core.context.trace.AbstractSpan;
+import org.apache.skywalking.apm.agent.core.context.trace.SpanLayer;
+import 
org.apache.skywalking.apm.agent.core.plugin.interceptor.enhance.EnhancedInstance;
+import 
org.apache.skywalking.apm.agent.core.plugin.interceptor.enhance.v2.InstanceMethodsAroundInterceptorV2;
+import 
org.apache.skywalking.apm.agent.core.plugin.interceptor.enhance.v2.MethodInvocationContext;
+import org.apache.skywalking.apm.network.trace.component.ComponentsDefine;
+import org.springframework.amqp.core.Message;
+import org.springframework.amqp.core.MessageProperties;
+
+import com.rabbitmq.client.Channel;
+import com.rabbitmq.client.Connection;
+
+public class SpringRabbitMQConsumerInterceptor implements 
InstanceMethodsAroundInterceptorV2 {
+public static final String OPERATE_NAME_PREFIX = "RabbitMQ/";
+public static final String CONSUMER_OPERATE_NAME_SUFFIX = "/Consumer";
+
+@Override
+public void beforeMethod(EnhancedInstance objInst, Method method, Object[] 
allArguments, Class[] argumentsTypes,
+MethodInvocationContext context) throws Throwable {
+Channel channel = (Channel) allArguments[0];
+Message message = (Message) allArguments[1];

Review Comment:
   In SpringRabbitMQConsumerInterceptor#beforeMethod, allArguments[1] is 
directly cast to
 org.springframework.amqp.core.Message. However, in Spring AMQP, this 
argument (the data
 parameter in executeListener) is an Object that can be a List 
when batch mode is enabled.
   
   Refer to Spring AMQP's AbstractMessageListenerContainer.java 
   
   
https://github.com/spring-projects/spring-amqp/blob/d892bfce5d002f051370dd6316538298a75a4eb6/spring-rabbit/src/main/java/org/springframework/amqp/rabbit/listener/AbstractMessageListenerContainer.java#L1734-L1744
   
   Performing a direct cast (Message) allArguments[1] will throw a 
ClassCastException in any Spring RabbitMQ application where batching is enabled.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]

Re: [PR] Add Spring RabbitMQ plugin [skywalking-java]

2026-03-11 Thread via GitHub 


wu-sheng commented on PR #796:
URL: https://github.com/apache/skywalking-java/pull/796#issuecomment-4042994296

   Please resolve conflicts as another PR merged.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]

Re: [PR] Add Spring RabbitMQ plugin [skywalking-java]

2026-03-10 Thread via GitHub 


wu-sheng commented on PR #796:
URL: https://github.com/apache/skywalking-java/pull/796#issuecomment-4036619442

   If after <2> to <4> changed, <1> is still valid, and you want to keep that, 
you could. But please mentioned in supported docs, this plugin is for 
deprecated versions.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]

Re: [PR] Add Spring RabbitMQ plugin [skywalking-java]

2026-03-10 Thread via GitHub 


liuhaolong10 commented on PR #796:
URL: https://github.com/apache/skywalking-java/pull/796#issuecomment-4036106091

   1. When adding the new plugin, my intention was to verify and cover as many 
versions as possible, so I validated the `spring-boot-starter-amqp` versions 
from 2.0.0.RELEASE to 2.4.0. Although these versions are no longer maintained 
by Spring, they are still widely used in many companies' projects. I verified 
these versions to clearly indicate which versions the new plugin supports. If 
the test scenarios involving EOL (End of Life) versions introduce security 
risks, I can remove the corresponding test scenario code.
   
   2. You are absolutely right that the thread name check mechanism is 
problematic — this was an oversight on my part. After sorting out the code 
logic, I plan to modify it to judge by object type instead:
  ```java
  Consumer consumer = (Consumer) allArguments[6];
  if (consumer != null && 
"org.springframework.amqp.rabbit.listener.BlockingQueueConsumer$InternalConsumer".equals(consumer.getClass().getName()))
 {
  return;
  }
   
   3. I will refactor the plugin code to use the plugin v2 APIs in my next 
commit.
   4. I will update the Supported-list.d file in my next commit as well.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]

Re: [PR] Add Spring RabbitMQ plugin [skywalking-java]

2026-03-10 Thread via GitHub 


wu-sheng commented on PR #796:
URL: https://github.com/apache/skywalking-java/pull/796#issuecomment-4030797297

   1. The 2.x test scenario uses spring-boot-starter-amqp versions 
2.0.0.RELEASE through 2.4.0 — these are all EOL Spring Boot 2 versions. Given 
that 2.x is no longer maintained. We may be better to remove that?
   2. Thread name check in RabbitMQConsumerInterceptor is fragile and unreliable
 (rabbitmq-plugin/.../RabbitMQConsumerInterceptor.java)
   ```
 if 
(Thread.currentThread().getName().toLowerCase().contains("springframework")) {
 return;
 }
   ```
   
 This is the biggest problem in the PR. Relying on thread names to 
determine behavior is extremely brittle:
 - Thread names are not a contract and can be customized by users or 
framework versions.
 - If a non-Spring application happens to name threads with 
"springframework", the original RabbitMQ plugin silently breaks.
 - If Spring changes thread naming conventions, both plugins break 
(duplicate spans or no spans).
 - This couples the existing rabbitmq-plugin to Spring-specific knowledge, 
violating separation of concerns.
3. Consider adopting plugin v2 APIs.
4. Supported-list.d should be updated.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]

Re: [PR] Add Spring RabbitMQ plugin [skywalking-java]

2026-03-08 Thread via GitHub 


wu-sheng commented on PR #796:
URL: https://github.com/apache/skywalking-java/pull/796#issuecomment-4019068288

   >In my local runs, most of the health checks don't pass at all.
   
   Health check should be passed, as it verifies whether the service is ready 
to send traffic.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]

Re: [PR] Add Spring RabbitMQ plugin [skywalking-java]

2026-03-08 Thread via GitHub 


liuhaolong10 commented on PR #796:
URL: https://github.com/apache/skywalking-java/pull/796#issuecomment-4019059440

   The health check for `spring-rabbitmq-2. x-scenario` failed when I ran it 
around 2 PM.
   The health check for `spring-rabbitmq-3.x-4. x-scenario` passed, but the 
expected data was incorrect.
   
   In my local runs, most of the health checks don't pass at all.
   I am using the `rabbitmq:3.8.18` image, which I copied directly from the 
scenario code in the rabbitmq-scenario test module.
   I believe the image itself should not be the problem.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]

Re: [PR] Add Spring RabbitMQ plugin [skywalking-java]

2026-03-08 Thread via GitHub 


wu-sheng commented on PR #796:
URL: https://github.com/apache/skywalking-java/pull/796#issuecomment-4019003017

   From my check of the logs, the server indeed is never booted. Because 
`HEAD:/spring-rabbitmq-3.x-4.x-scenario/case/healthcheck` is never responding. 
Is you app waiting for RabbitMQ server booted properly?


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]

Re: [PR] Add Spring RabbitMQ plugin [skywalking-java]

2026-03-08 Thread via GitHub 


wu-sheng commented on PR #796:
URL: https://github.com/apache/skywalking-java/pull/796#issuecomment-4018873911

   GitHub is on Azure US, it should not have image pulling issue. Have you 
rechecked whether this image exists on DockerHub?


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]

Re: [PR] Add Spring RabbitMQ plugin [skywalking-java]

2026-03-08 Thread via GitHub 


liuhaolong10 commented on PR #796:
URL: https://github.com/apache/skywalking-java/pull/796#issuecomment-4018857174

   @wu-sheng 
   When I run the test script locally with the command `bash 
./test/plugin/run.sh -f spring-rabbitmq-3.x-4.x-scenario`, it consistently 
fails to execute successfully.  There are two main reasons identified:
   1.  Timeout when connecting to GitHub.
   2.  Failure to pull the `rabbitmq-server` image via Docker.
   
   I have a proxy enabled locally, and accessing GitHub via the browser works 
without any issues.  Despite spending the entire afternoon troubleshooting this 
test, I still haven't been able to make it run successfully.
   
   Could you please share some tips or best practices that you use when running 
this test locally?  Any guidance would be greatly appreciated.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]

Re: [PR] Add Spring RabbitMQ plugin [skywalking-java]

2026-03-08 Thread via GitHub 


liuhaolong10 commented on PR #796:
URL: https://github.com/apache/skywalking-java/pull/796#issuecomment-4018856815

   @wu-sheng When I run the test script locally with the command `bash 
./test/plugin/run.sh -f spring-rabbitmq-3.x-4.x-scenario`, it consistently 
fails to execute successfully.  There are two main reasons identified:
   1.  Timeout when connecting to GitHub.
   2.  Failure to pull the `rabbitmq-server` image via Docker.
   
   I have a proxy enabled locally, and accessing GitHub via the browser works 
without any issues.  Despite spending the entire afternoon troubleshooting this 
test, I still haven't been able to make it run successfully.
   
   Could you please share some tips or best practices that you use when running 
this test locally?  Any guidance would be greatly appreciated.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]