From 429ee40f8147e5647c20368db014c71227c4d352 Mon Sep 17 00:00:00 2001 From: Gary Huang Date: Wed, 6 Jun 2018 22:52:43 -0400 Subject: [PATCH] add workaround to avoid certain commands from crashing the trace agent. --- .../lettuce/LettuceAsyncCommandsAdvice.java | 7 ++- .../lettuce/LettuceInstrumentationUtil.java | 50 ++++++++++++++++++- .../rx/LettuceFluxTerminationRunnable.java | 5 +- .../lettuce/rx/LettuceMonoDualConsumer.java | 4 +- .../test/groovy/LettuceAsyncClientTest.groovy | 3 +- .../groovy/LettuceReactiveClientTest.groovy | 7 +-- .../test/groovy/LettuceSyncClientTest.groovy | 3 +- 7 files changed, 69 insertions(+), 10 deletions(-) diff --git a/dd-java-agent/instrumentation/lettuce-5/src/main/java8/datadog/trace/instrumentation/lettuce/LettuceAsyncCommandsAdvice.java b/dd-java-agent/instrumentation/lettuce-5/src/main/java8/datadog/trace/instrumentation/lettuce/LettuceAsyncCommandsAdvice.java index 95157a45fa..2e0343b8b7 100644 --- a/dd-java-agent/instrumentation/lettuce-5/src/main/java8/datadog/trace/instrumentation/lettuce/LettuceAsyncCommandsAdvice.java +++ b/dd-java-agent/instrumentation/lettuce-5/src/main/java8/datadog/trace/instrumentation/lettuce/LettuceAsyncCommandsAdvice.java @@ -26,7 +26,8 @@ public class LettuceAsyncCommandsAdvice { Tags.SPAN_KIND.set(span, Tags.SPAN_KIND_CLIENT); Tags.COMPONENT.set(span, LettuceInstrumentationUtil.COMPONENT_NAME); - span.setTag(DDTags.RESOURCE_NAME, commandName); + span.setTag( + DDTags.RESOURCE_NAME, LettuceInstrumentationUtil.getCommandResourceName(commandName)); span.setTag(DDTags.SERVICE_NAME, LettuceInstrumentationUtil.SERVICE_NAME); span.setTag(DDTags.SPAN_TYPE, LettuceInstrumentationUtil.SERVICE_NAME); @@ -35,6 +36,7 @@ public class LettuceAsyncCommandsAdvice { @Advice.OnMethodExit(onThrowable = Throwable.class, suppress = Throwable.class) public static void stopSpan( + @Advice.Argument(0) final RedisCommand command, @Advice.Enter final Scope scope, @Advice.Thrown final Throwable throwable, @Advice.Return final AsyncCommand asyncCommand) { @@ -48,8 +50,9 @@ public class LettuceAsyncCommandsAdvice { return; } + String commandName = LettuceInstrumentationUtil.getCommandName(command); // close spans on error or normal completion - if (!LettuceInstrumentationUtil.doFinishSpanEarly(span.getBaggageItem(DDTags.RESOURCE_NAME))) { + if (!LettuceInstrumentationUtil.doFinishSpanEarly(commandName)) { asyncCommand.handleAsync(new LettuceAsyncBiFunction<>(scope.span())); } scope.close(); diff --git a/dd-java-agent/instrumentation/lettuce-5/src/main/java8/datadog/trace/instrumentation/lettuce/LettuceInstrumentationUtil.java b/dd-java-agent/instrumentation/lettuce-5/src/main/java8/datadog/trace/instrumentation/lettuce/LettuceInstrumentationUtil.java index 4406c9ea82..d1da8d072f 100644 --- a/dd-java-agent/instrumentation/lettuce-5/src/main/java8/datadog/trace/instrumentation/lettuce/LettuceInstrumentationUtil.java +++ b/dd-java-agent/instrumentation/lettuce-5/src/main/java8/datadog/trace/instrumentation/lettuce/LettuceInstrumentationUtil.java @@ -10,13 +10,60 @@ public class LettuceInstrumentationUtil { public static final String[] NON_INSTRUMENTING_COMMAND_WORDS = new String[] {"SHUTDOWN", "DEBUG", "OOM", "SEGFAULT"}; + + public static final String[] AGENT_CRASHING_COMMANDS_WORDS = + new String[] {"CLIENT", "CLUSTER", "COMMAND", "CONFIG", "DEBUG", "SCRIPT"}; + + public static final String AGENT_CRASHING_COMMAND_PREFIX = "COMMAMND-NAME:"; + public static final Set nonInstrumentingCommands = new HashSet<>(Arrays.asList(NON_INSTRUMENTING_COMMAND_WORDS)); + public static final Set agentCrashingCommands = + new HashSet<>(Arrays.asList(AGENT_CRASHING_COMMANDS_WORDS)); + + /** + * Determines whether a redis command should finish its relevant span early (as soon as tags are + * added and the command is executed) because these commands have no return values/call backs, so + * we must close the span early in order to provide info for the users + * + * @param commandName a redis command, without any prefixes + * @return true if finish the span early (the command will not have a return value) + */ public static boolean doFinishSpanEarly(String commandName) { return nonInstrumentingCommands.contains(commandName); } + // Workaround to keep trace agent from crashing + // Currently the commands in AGENT_CRASHING_COMMANDS_WORDS will crash the trace agent and + // traces with these commands as the resource name will not be processed by the trace agent + // https://github.com/DataDog/datadog-trace-agent/blob/master/quantizer/redis.go#L18 has + // list of commands that will currently fail at the trace agent level. + + /** + * Workaround to keep trace agent from crashing Currently the commands in + * AGENT_CRASHING_COMMANDS_WORDS will crash the trace agent and traces with these commands as the + * resource name will not be processed by the trace agent + * https://github.com/DataDog/datadog-trace-agent/blob/master/quantizer/redis.go#L18 has list of + * commands that will currently fail at the trace agent level. + * + * @param actualCommandName the actual redis command + * @return the redis command with a prefix if it is a command that will crash the trace agent, + * otherwise, the original command is returned. + */ + public static String getCommandResourceName(String actualCommandName) { + if (agentCrashingCommands.contains(actualCommandName)) { + return AGENT_CRASHING_COMMAND_PREFIX + actualCommandName; + } + return actualCommandName; + } + + /** + * Retrieves the actual redis command name from a RedisCommand object + * + * @param command the lettuce RedisCommand object + * @return the redis command as a string + */ public static String getCommandName(RedisCommand command) { String commandName = "Redis Command"; if (command != null) { @@ -31,9 +78,10 @@ public class LettuceInstrumentationUtil { } } */ + // get the redis command name (i.e. GET, SET, HMSET, etc) if (command.getType() != null) { - commandName = command.getType().name(); + commandName = command.getType().name().trim(); /* // if it is an AUTH command, then remove the extracted command arguments since it is the password if ("AUTH".equals(commandName)) { diff --git a/dd-java-agent/instrumentation/lettuce-5/src/main/java8/datadog/trace/instrumentation/lettuce/rx/LettuceFluxTerminationRunnable.java b/dd-java-agent/instrumentation/lettuce-5/src/main/java8/datadog/trace/instrumentation/lettuce/rx/LettuceFluxTerminationRunnable.java index f7c8884f84..3f87174443 100644 --- a/dd-java-agent/instrumentation/lettuce-5/src/main/java8/datadog/trace/instrumentation/lettuce/rx/LettuceFluxTerminationRunnable.java +++ b/dd-java-agent/instrumentation/lettuce-5/src/main/java8/datadog/trace/instrumentation/lettuce/rx/LettuceFluxTerminationRunnable.java @@ -95,7 +95,10 @@ public class LettuceFluxTerminationRunnable implements Consumer, Runnabl Tags.SPAN_KIND.set(span, Tags.SPAN_KIND_CLIENT); Tags.COMPONENT.set(span, LettuceInstrumentationUtil.COMPONENT_NAME); - span.setTag(DDTags.RESOURCE_NAME, commandName); + // should be command name only, but use workaround to prepend string to agent crashing commands + span.setTag( + DDTags.RESOURCE_NAME, + LettuceInstrumentationUtil.getCommandResourceName(this.commandName)); span.setTag(DDTags.SERVICE_NAME, LettuceInstrumentationUtil.SERVICE_NAME); span.setTag(DDTags.SPAN_TYPE, LettuceInstrumentationUtil.SERVICE_NAME); scope.close(); diff --git a/dd-java-agent/instrumentation/lettuce-5/src/main/java8/datadog/trace/instrumentation/lettuce/rx/LettuceMonoDualConsumer.java b/dd-java-agent/instrumentation/lettuce-5/src/main/java8/datadog/trace/instrumentation/lettuce/rx/LettuceMonoDualConsumer.java index 0f25eb4533..aa9dcd4ae4 100644 --- a/dd-java-agent/instrumentation/lettuce-5/src/main/java8/datadog/trace/instrumentation/lettuce/rx/LettuceMonoDualConsumer.java +++ b/dd-java-agent/instrumentation/lettuce-5/src/main/java8/datadog/trace/instrumentation/lettuce/rx/LettuceMonoDualConsumer.java @@ -52,7 +52,9 @@ public class LettuceMonoDualConsumer Tags.SPAN_KIND.set(this.span, Tags.SPAN_KIND_CLIENT); Tags.COMPONENT.set(this.span, LettuceInstrumentationUtil.COMPONENT_NAME); - this.span.setTag(DDTags.RESOURCE_NAME, this.commandName); + // should be command name only, but use workaround to prepend string to agent crashing commands + this.span.setTag( + DDTags.RESOURCE_NAME, LettuceInstrumentationUtil.getCommandResourceName(this.commandName)); this.span.setTag(DDTags.SERVICE_NAME, LettuceInstrumentationUtil.SERVICE_NAME); this.span.setTag(DDTags.SPAN_TYPE, LettuceInstrumentationUtil.SERVICE_NAME); scope.close(); diff --git a/dd-java-agent/instrumentation/lettuce-5/src/test/groovy/LettuceAsyncClientTest.groovy b/dd-java-agent/instrumentation/lettuce-5/src/test/groovy/LettuceAsyncClientTest.groovy index b946943b30..456120a955 100644 --- a/dd-java-agent/instrumentation/lettuce-5/src/test/groovy/LettuceAsyncClientTest.groovy +++ b/dd-java-agent/instrumentation/lettuce-5/src/test/groovy/LettuceAsyncClientTest.groovy @@ -22,6 +22,7 @@ import java.util.function.Consumer import java.util.function.Function import static datadog.trace.agent.test.ListWriterAssert.assertTraces +import static datadog.trace.instrumentation.lettuce.LettuceInstrumentationUtil.AGENT_CRASHING_COMMAND_PREFIX class LettuceAsyncClientTest extends AgentTestRunner { @@ -487,7 +488,7 @@ class LettuceAsyncClientTest extends AgentTestRunner { serviceName "redis" operationName "redis.query" spanType "redis" - resourceName "DEBUG" + resourceName AGENT_CRASHING_COMMAND_PREFIX + "DEBUG" errored false tags { diff --git a/dd-java-agent/instrumentation/lettuce-5/src/test/groovy/LettuceReactiveClientTest.groovy b/dd-java-agent/instrumentation/lettuce-5/src/test/groovy/LettuceReactiveClientTest.groovy index 77ee5c52f3..80f77d436c 100644 --- a/dd-java-agent/instrumentation/lettuce-5/src/test/groovy/LettuceReactiveClientTest.groovy +++ b/dd-java-agent/instrumentation/lettuce-5/src/test/groovy/LettuceReactiveClientTest.groovy @@ -10,6 +10,7 @@ import spock.util.concurrent.AsyncConditions import java.util.function.Consumer import static datadog.trace.agent.test.ListWriterAssert.assertTraces +import static datadog.trace.instrumentation.lettuce.LettuceInstrumentationUtil.AGENT_CRASHING_COMMAND_PREFIX class LettuceReactiveClientTest extends AgentTestRunner { @@ -201,7 +202,7 @@ class LettuceReactiveClientTest extends AgentTestRunner { serviceName "redis" operationName "redis.query" spanType "redis" - resourceName "COMMAND" + resourceName AGENT_CRASHING_COMMAND_PREFIX + "COMMAND" errored false tags { @@ -228,7 +229,7 @@ class LettuceReactiveClientTest extends AgentTestRunner { serviceName "redis" operationName "redis.query" spanType "redis" - resourceName "COMMAND" + resourceName AGENT_CRASHING_COMMAND_PREFIX + "COMMAND" errored false tags { @@ -268,7 +269,7 @@ class LettuceReactiveClientTest extends AgentTestRunner { serviceName "redis" operationName "redis.query" spanType "redis" - resourceName "DEBUG" + resourceName AGENT_CRASHING_COMMAND_PREFIX + "DEBUG" errored false tags { diff --git a/dd-java-agent/instrumentation/lettuce-5/src/test/groovy/LettuceSyncClientTest.groovy b/dd-java-agent/instrumentation/lettuce-5/src/test/groovy/LettuceSyncClientTest.groovy index 32e56129e5..27b30a8681 100644 --- a/dd-java-agent/instrumentation/lettuce-5/src/test/groovy/LettuceSyncClientTest.groovy +++ b/dd-java-agent/instrumentation/lettuce-5/src/test/groovy/LettuceSyncClientTest.groovy @@ -10,6 +10,7 @@ import spock.lang.Shared import java.util.concurrent.CompletionException import static datadog.trace.agent.test.ListWriterAssert.assertTraces +import static datadog.trace.instrumentation.lettuce.LettuceInstrumentationUtil.AGENT_CRASHING_COMMAND_PREFIX class LettuceSyncClientTest extends AgentTestRunner { @@ -327,7 +328,7 @@ class LettuceSyncClientTest extends AgentTestRunner { serviceName "redis" operationName "redis.query" spanType "redis" - resourceName "DEBUG" + resourceName AGENT_CRASHING_COMMAND_PREFIX + "DEBUG" errored false tags {