You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@skywalking.apache.org by GitBox <gi...@apache.org> on 2022/11/19 16:21:05 UTC
[GitHub] [skywalking-java] nisiyong opened a new pull request, #389: Compatible with 3.x and 4.x RabbitMQ Connection
nisiyong opened a new pull request, #389:
URL: https://github.com/apache/skywalking-java/pull/389
<!--
⚠️ Please make sure to read this template first, pull requests that don't accord with this template
maybe closed without notice.
Texts surrounded by `<` and `>` are meant to be replaced by you, e.g. <framework name>, <issue number>.
Put an `x` in the `[ ]` to mark the item as CHECKED. `[x]`
-->
<!-- ==== 🐛 Remove this line WHEN AND ONLY WHEN you're fixing a bug, follow the checklist 👇 ====
### Fix <bug description or the bug issue number or bug issue link>
- [ ] Add a unit test to verify that the fix works.
- [ ] Explain briefly why the bug exists and how to fix it.
==== 🐛 Remove this line WHEN AND ONLY WHEN you're fixing a bug, follow the checklist 👆 ==== -->
Recently I analyze all skywalking java agents logs in our production environment, and found there are some legacy applications using `rabbitmq-java-client-3.x` print a lot of intercept failure log, as follow:
```
class[class com.rabbitmq.client.impl.ChannelN] before method[basicPublish] intercept failure
java.lang.IllegalStateException: Exit span doesn't include meaningful peer information.
at org.apache.skywalking.apm.agent.core.context.TracingContext.inject(TracingContext.java:169)
at org.apache.skywalking.apm.agent.core.context.TracingContext.inject(TracingContext.java:149)
at org.apache.skywalking.apm.agent.core.context.ContextManager.createExitSpan(ContextManager.java:126)
at org.apache.skywalking.apm.plugin.rabbitmq.RabbitMQProducerInterceptor.beforeMethod(RabbitMQProducerInterceptor.java:76)
at org.apache.skywalking.apm.agent.core.plugin.interceptor.enhance.InstMethodsInterWithOverrideArgs.intercept(InstMethodsInterWithOverrideArgs.java:75)
at com.rabbitmq.client.impl.ChannelN.basicPublish(ChannelN.java)
at com.rabbitmq.client.impl.ChannelN.basicPublish(ChannelN.java:639)
at org.springframework.amqp.rabbit.support.PublisherCallbackChannelImpl.basicPublish(PublisherCallbackChannelImpl.java:259)
at sun.reflect.GeneratedMethodAccessor419.invoke(Unknown Source)
at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
at java.lang.reflect.Method.invoke(Method.java:498)
at org.springframework.amqp.rabbit.connection.CachingConnectionFactory$CachedChannelInvocationHandler.invoke(CachingConnectionFactory.java:917)
```
## Why
There is an old issue has mentioned this situation, https://github.com/apache/skywalking/issues/2638. This because the the rabbitmq-client could not intercept the 3.x and 4.x contrunctor function, then set the peer info failed.
## Solution
I compare the `com.rabbitmq.client.impl.ChannelN` contructors at 3.x, 4.x and 5.x.
* 5.x: [ChannelN.java](https://github.com/rabbitmq/rabbitmq-java-client/blob/v5.16.0/src/main/java/com/rabbitmq/client/impl/ChannelN.java)
* 4.x: [ChannelN.java](https://github.com/rabbitmq/rabbitmq-java-client/blob/v4.12.0/src/main/java/com/rabbitmq/client/impl/ChannelN.java)
* 3.x: [ChannelN.java](https://github.com/rabbitmq/rabbitmq-java-client/blob/rabbitmq_v3_6_10/src/main/java/com/rabbitmq/client/impl/ChannelN.java)
There have the same 1st argument `com.rabbitmq.client.impl.AMQConnection`, and the peer address retrieve from this object. I think we just need to polish the contrunctor instrumentation logic.
## Sceenshots
### Before changing the instrument logic, rabbtimq-3.x-producer could not get peer
<img width="881" alt="image" src="https://user-images.githubusercontent.com/8198862/202860380-3ee69f87-dc33-49ca-85d5-699cda81ec0d.png">
### After changing the instrument logic, both rabbtimq-3.x-producer and rabbtimq-3.x-consumer could get more infomation
<img width="938" alt="image" src="https://user-images.githubusercontent.com/8198862/202860438-769e92b1-76dc-4d37-8c02-ccc4507d7d93.png">
<img width="922" alt="image" src="https://user-images.githubusercontent.com/8198862/202860473-e7bc31cd-6758-44ac-920d-cb6724073eed.png">
---
Finally, I think it's unnecessary to add `rabbitmq-3.x-plugin` or `rabbit-4.x-plugin`, the previous constructor instrumentation is not inherently well implemented, I just make it more compatible.
- [ ] Update the [`CHANGES` log](https://github.com/apache/skywalking-java/blob/main/CHANGES.md).
--
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: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] wu-sheng merged pull request #389: Compatible with 3.x and 4.x RabbitMQ Client
Posted by GitBox <gi...@apache.org>.
wu-sheng merged PR #389:
URL: https://github.com/apache/skywalking-java/pull/389
--
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: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] wu-sheng commented on pull request #389: Compatible with 3.x and 4.x RabbitMQ Client
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on PR #389:
URL: https://github.com/apache/skywalking-java/pull/389#issuecomment-1321040206
> I also wanna rename rabbitmq-5.x-plugin to rabbitmq-plugin. What do you think about this?
It should be good. But you have to change several files, and name in `skywalking-plugin.def`. You should mention this as breaking changes in the change log. (`* [Breaking Change]: xxxxx`)
> The previous test case used com.rabbitmq.client.DeliverCallback introduced in 5.0.0, and if we tested with 3.x or 4.x, it will fail to compile.
OK, we may should consider two test scenarios, one is your way, the other is the previous way only for 5.x.
--
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: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] nisiyong commented on pull request #389: Compatible with 3.x and 4.x RabbitMQ Client
Posted by GitBox <gi...@apache.org>.
nisiyong commented on PR #389:
URL: https://github.com/apache/skywalking-java/pull/389#issuecomment-1321041494
> OK, we may should consider two test scenarios, one is your way, the other is the previous way only for 5.x.
Using the new test scenario is fine, we don't need the previous test case. The `com.rabbitmq.client.DeliverCallback` is just for users to easier to use API, the core API consume Class still is `com.rabbitmq.client.Consumer`. The `DeliverCallback` is implemented with `Consumer` inside.
--
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: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] wu-sheng commented on pull request #389: Compatible with 3.x and 4.x RabbitMQ Connection
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on PR #389:
URL: https://github.com/apache/skywalking-java/pull/389#issuecomment-1320991961
Please update changelog and supported list.
More importantly, could yiu try to update test scenarion versions? If possible.
--
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: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] wu-sheng commented on pull request #389: Compatible with 3.x and 4.x RabbitMQ Connection
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on PR #389:
URL: https://github.com/apache/skywalking-java/pull/389#issuecomment-1321035862
Tests are passed, please update the mentioned docs.
--
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: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] nisiyong commented on pull request #389: Compatible with 3.x and 4.x RabbitMQ Connection
Posted by GitBox <gi...@apache.org>.
nisiyong commented on PR #389:
URL: https://github.com/apache/skywalking-java/pull/389#issuecomment-1321037527
The previous test case used `com.rabbitmq.client.DeliverCallback` introduced in 5.0.0, and if we tested with 3.x or 4.x, it will fail to compile.
Now the case uses the old way of consuming messages, which is compatible with versions from 3.0.0 to 5.16.0.
* https://mvnrepository.com/artifact/com.rabbitmq/amqp-client
I also wanna rename `rabbitmq-5.x-plugin` to `rabbitmq-plugin`. What do you think about this?
--
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: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] nisiyong commented on pull request #389: Compatible with 3.x and 4.x RabbitMQ Client
Posted by GitBox <gi...@apache.org>.
nisiyong commented on PR #389:
URL: https://github.com/apache/skywalking-java/pull/389#issuecomment-1321044587
All things are finished, lease take a look.
--
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: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org