Skip to content

Compatible with 3.x and 4.x RabbitMQ Client#389

Merged
wu-sheng merged 4 commits intoapache:mainfrom
nisiyong:compatible-rabbitmq-connection
Nov 20, 2022
Merged

Compatible with 3.x and 4.x RabbitMQ Client#389
wu-sheng merged 4 commits intoapache:mainfrom
nisiyong:compatible-rabbitmq-connection

Conversation

@nisiyong
Copy link
Copy Markdown
Contributor

@nisiyong nisiyong commented Nov 19, 2022

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, apache/skywalking#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.

There have the same 1st argument com.rabbitmq.client.impl.AMQConnection, and the peer address is retrieved 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

image

After changing the instrument logic, both rabbtimq-3.x-producer and rabbtimq-3.x-consumer could get more infomation

image

image


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.

@wu-sheng
Copy link
Copy Markdown
Member

Please update changelog and supported list.
More importantly, could yiu try to update test scenarion versions? If possible.

@wu-sheng wu-sheng added enhancement New feature or request plugin labels Nov 19, 2022
@wu-sheng wu-sheng added this to the 8.14.0 milestone Nov 19, 2022
@wu-sheng
Copy link
Copy Markdown
Member

Tests are passed, please update the mentioned docs.

@nisiyong
Copy link
Copy Markdown
Contributor Author

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.

I also wanna rename rabbitmq-5.x-plugin to rabbitmq-plugin. What do you think about this?

@nisiyong nisiyong changed the title Compatible with 3.x and 4.x RabbitMQ Connection Compatible with 3.x and 4.x RabbitMQ Client Nov 20, 2022
@wu-sheng
Copy link
Copy Markdown
Member

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.

@nisiyong
Copy link
Copy Markdown
Contributor Author

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.

@nisiyong
Copy link
Copy Markdown
Contributor Author

nisiyong commented Nov 20, 2022

All things are finished, please take a look.

@wu-sheng wu-sheng merged commit d541b7b into apache:main Nov 20, 2022
@nisiyong nisiyong deleted the compatible-rabbitmq-connection branch November 20, 2022 06:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request plugin

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants