Skip to content
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.

Sign up for GitHub

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

Fix bugs in Redisson plugin #693

Merged
merged 3 commits into from
May 22, 2024
Merged

Fix bugs in Redisson plugin #693

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
cb9766c
enhance redisson plugin.
CzyerChen May 22, 2024
ad7e320
update startup.sh.
CzyerChen May 22, 2024
06368cb
change ping_ignore to show_ping_command & update showBatchCommands()
CzyerChen May 22, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions CHANGES.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,9 @@ Release Notes.
* Add support for `Derby`/`Sybase`/`SQLite`/`DB2`/`OceanBase` jdbc url format in `URLParser`.
* Optimize spring-plugins:scheduled-annotation-plugin compatibility about Spring 6.1.x support.
* Add a forceIgnoring mechanism in a CROSS_THREAD scenario.
* Fix NPE in Redisson plugin since Redisson 3.20.0.
* Support for showing batch command details and ignoring PING commands in Redisson plugin.
* Fix peer value of Master-Slave mode in Redisson plugin.

All issues and pull requests are [here](https://github.com/apache/skywalking/milestone/213?closed=1)

Expand Down
2 changes: 1 addition & 1 deletion apm-sniffer/apm-sdk-plugin/redisson-3.x-plugin/pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@
<name>redisson-3.x-plugin</name>
<url>http://maven.apache.org</url>
<properties>
<redisson.version>3.6.0</redisson.version>
<redisson.version>3.20.0</redisson.version>
</properties>

<dependencies>
Expand Down
43 changes: 33 additions & 10 deletions .../main/java/org/apache/skywalking/apm/plugin/redisson/v3/ConnectionManagerInterceptor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@

package org.apache.skywalking.apm.plugin.redisson.v3;

import java.util.Objects;
import org.apache.skywalking.apm.agent.core.context.util.PeerFormat;
import org.apache.skywalking.apm.agent.core.logging.api.ILog;
import org.apache.skywalking.apm.agent.core.logging.api.LogManager;
Expand All @@ -26,11 +27,12 @@
import org.apache.skywalking.apm.agent.core.plugin.interceptor.enhance.MethodInterceptResult;
import org.apache.skywalking.apm.plugin.redisson.v3.util.ClassUtil;
import org.redisson.config.Config;
import org.redisson.connection.ConnectionManager;

import java.lang.reflect.Method;
import java.net.URI;
import java.util.Collection;
import org.redisson.connection.MasterSlaveConnectionManager;
import org.redisson.connection.ServiceManager;

public class ConnectionManagerInterceptor implements InstanceMethodsAroundInterceptor {

Expand All @@ -45,14 +47,19 @@ public void beforeMethod(EnhancedInstance objInst, Method method, Object[] allAr
public Object afterMethod(EnhancedInstance objInst, Method method, Object[] allArguments, Class<?>[] argumentsTypes,
Object ret) throws Throwable {
try {
ConnectionManager connectionManager = (ConnectionManager) objInst;
Config config = connectionManager.getCfg();

Object singleServerConfig = ClassUtil.getObjectField(config, "singleServerConfig");
Object sentinelServersConfig = ClassUtil.getObjectField(config, "sentinelServersConfig");
Object masterSlaveServersConfig = ClassUtil.getObjectField(config, "masterSlaveServersConfig");
Object clusterServersConfig = ClassUtil.getObjectField(config, "clusterServersConfig");
Object replicatedServersConfig = ClassUtil.getObjectField(config, "replicatedServersConfig");
Config config = getConfig(objInst);
Object singleServerConfig = null;
Object sentinelServersConfig = null;
Object masterSlaveServersConfig = null;
Object clusterServersConfig = null;
Object replicatedServersConfig = null;
if (Objects.nonNull(config)) {
singleServerConfig = ClassUtil.getObjectField(config, "singleServerConfig");
sentinelServersConfig = ClassUtil.getObjectField(config, "sentinelServersConfig");
masterSlaveServersConfig = ClassUtil.getObjectField(config, "masterSlaveServersConfig");
clusterServersConfig = ClassUtil.getObjectField(config, "clusterServersConfig");
replicatedServersConfig = ClassUtil.getObjectField(config, "replicatedServersConfig");
}

StringBuilder peer = new StringBuilder();
EnhancedInstance retInst = (EnhancedInstance) ret;
Expand All @@ -70,7 +77,7 @@ public Object afterMethod(EnhancedInstance objInst, Method method, Object[] allA
}
if (masterSlaveServersConfig != null) {
Object masterAddress = ClassUtil.getObjectField(masterSlaveServersConfig, "masterAddress");
peer.append(getPeer(masterAddress));
peer.append(getPeer(masterAddress)).append(";");
appendAddresses(peer, (Collection) ClassUtil.getObjectField(masterSlaveServersConfig, "slaveAddresses"));
retInst.setSkyWalkingDynamicField(PeerFormat.shorten(peer.toString()));
return ret;
Expand Down Expand Up @@ -118,6 +125,22 @@ static String getPeer(Object obj) {
}
}

private Config getConfig(EnhancedInstance objInst) {
Config config = null;
MasterSlaveConnectionManager connectionManager = (MasterSlaveConnectionManager) objInst;
try {
config = (Config) ClassUtil.getObjectField(connectionManager, "cfg");
} catch (NoSuchFieldException | IllegalAccessException ignore) {
try {
ServiceManager serviceManager = (ServiceManager) ClassUtil.getObjectField(
connectionManager, "serviceManager");
config = serviceManager.getCfg();
} catch (NoSuchFieldException | IllegalAccessException ignore2) {
}
}
return config;
}

@Override
public void handleMethodException(EnhancedInstance objInst, Method method, Object[] allArguments,
Class<?>[] argumentsTypes, Throwable t) {
Expand Down
19 changes: 17 additions & 2 deletions ...n/java/org/apache/skywalking/apm/plugin/redisson/v3/RedisConnectionMethodInterceptor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
package org.apache.skywalking.apm.plugin.redisson.v3;

import io.netty.channel.Channel;
import java.util.stream.Collectors;
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;
Expand Down Expand Up @@ -66,11 +67,18 @@ public void beforeMethod(EnhancedInstance objInst, Method method, Object[] allAr
if (allArguments[0] instanceof CommandsData) {
operationName = operationName + "BATCH_EXECUTE";
command = "BATCH_EXECUTE";
if (RedissonPluginConfig.Plugin.Redisson.SHOW_BATCH_COMMANDS) {
command += ":" + showBatchCommands((CommandsData) allArguments[0]);
}
} else if (allArguments[0] instanceof CommandData) {
CommandData commandData = (CommandData) allArguments[0];
command = commandData.getCommand().getName();
operationName = operationName + command;
arguments = commandData.getParams();
if ("PING".equals(command) && !RedissonPluginConfig.Plugin.Redisson.SHOW_PING_COMMAND) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@wu-sheng This change will lead to a problem that the plugin cannot find an active span to close after "Ping" execution. Skywalking would throw a NullPointerException. Maybe it needs a flag to duel this situation.
NullPointerException

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you want to write that PR? I think this is easy. You just need to add if ("PING".equals(command) && !RedissonPluginConfig.Plugin.Redisson.SHOW_PING_COMMAND) { check in the #afterMethod and avoid ContextManager.stopSpan(); in this case.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@CzyerChen I think this is definitely going to happen.

Copy link
Contributor

@lujiajing1126 lujiajing1126 Jun 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This issue has been resolved by @Ricehomesky from our team in our private code several days ago. I suppose he can raise a PR very soon.

return;
} else {
operationName = operationName + command;
arguments = commandData.getParams();
}
}

AbstractSpan span = ContextManager.createExitSpan(operationName, peer);
Expand Down Expand Up @@ -143,4 +151,11 @@ private Optional<String> parseOperation(String cmd) {
}
return Optional.empty();
}

private String showBatchCommands(CommandsData commandsData) {
return commandsData.getCommands()
.stream()
.map(data -> data.getCommand().getName())
.collect(Collectors.joining(";"));
}
}
8 changes: 8 additions & 0 deletions ...ugin/src/main/java/org/apache/skywalking/apm/plugin/redisson/v3/RedissonPluginConfig.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,14 @@ public static class Redisson {
* Set a negative number to save specified length of parameter string to the tag.
*/
public static int REDIS_PARAMETER_MAX_LENGTH = 128;
/**
* If set to true, the PING command would be collected.
*/
public static boolean SHOW_PING_COMMAND = false;
/**
* If set to true, the detail of the Redis batch commands would be collected.
*/
public static boolean SHOW_BATCH_COMMANDS = false;

/**
* Operation represent a cache span is "write" or "read" action , and "op"(operation) is tagged with key "cache.op" usually
Expand Down
2 changes: 1 addition & 1 deletion docs/en/setup/service-agent/java-agent/Supported-list.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,7 @@ metrics based on the tracing data.
* [aerospike](https://github.com/aerospike/aerospike-client-java) 3.x -> 6.x
* Redis
* [Jedis](https://github.com/xetorthio/jedis) 2.x-4.x
* [Redisson](https://github.com/redisson/redisson) Easy Java Redis client 3.5.2+
* [Redisson](https://github.com/redisson/redisson) Easy Java Redis client 3.5.0 -> 3.30.0
* [Lettuce](https://github.com/lettuce-io/lettuce-core) 5.x
* [MongoDB Java Driver](https://github.com/mongodb/mongo-java-driver) 2.13-2.14, 3.4.0-3.12.7, 4.0.0-4.1.0
* Memcached Client
Expand Down
7 changes: 7 additions & 0 deletions test/plugin/scenarios/redisson-scenario/support-version.list
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,13 @@
# See the License for the specific language governing permissions and
# limitations under the License.

# 3.5.0-3.12.4, 3.26.1-3.30.0 have been tested, and 3.12.5-3.26.0 are also supported but not included in the test
3.30.0
3.29.0
3.28.0
3.27.2
3.26.1
3.12.4
3.11.5
3.10.7
3.9.1
Expand Down
Loading