From cb2a4ad084d260a6158b59ed05d91f0a1ac8d2a3 Mon Sep 17 00:00:00 2001 From: jean-philippe bempel Date: Wed, 8 Nov 2023 15:52:08 +0100 Subject: [PATCH 1/6] Update keyword hardcoded list --- .../bootstrap/debugger/util/Redaction.java | 85 ++++++++++++++++--- 1 file changed, 71 insertions(+), 14 deletions(-) diff --git a/dd-java-agent/agent-debugger/debugger-bootstrap/src/main/java/datadog/trace/bootstrap/debugger/util/Redaction.java b/dd-java-agent/agent-debugger/debugger-bootstrap/src/main/java/datadog/trace/bootstrap/debugger/util/Redaction.java index 41383d3c50a..69b720c60a9 100644 --- a/dd-java-agent/agent-debugger/debugger-bootstrap/src/main/java/datadog/trace/bootstrap/debugger/util/Redaction.java +++ b/dd-java-agent/agent-debugger/debugger-bootstrap/src/main/java/datadog/trace/bootstrap/debugger/util/Redaction.java @@ -18,30 +18,87 @@ public class Redaction { private static final Pattern COMMA_PATTERN = Pattern.compile(","); private static final List PREDEFINED_KEYWORDS = Arrays.asList( - "password", - "passwd", - "secret", + "2fa", + "accesstoken", + "aiohttpsession", + "apisecret", + "apisignature", "apikey", "auth", + "authtoken", + "authorization", + "ccnumber", + "certificatepin", + "cipher", + "clientid", + "clientsecret", + "config", + "connect.sid", + "cookie", "credentials", + "creditcard", + "csrf", + "csrftoken", + "cvv", + "databaseurl", + "dburl", + "encryptionkey", + "encryptionkeyid", + "env", + "gpgkey", + "jti", + "jwt", + "licensekey", + "masterkey", "mysqlpwd", + "nonce", + "oauth", + "oauth_token", + "otp", + "passhash", + "passwd", + "password", + "passwordb", + "pemfile", + "pgpkey", + "phpsessid", + "pin", + "pincode", + "pkcs8", "privatekey", - "token", - "ipaddress", + "publickey", + "recaptchakey", + "refreshtoken", + "routingnumber", + "salt", + "secret", + "secrettoken", + "secretKey", + "securityanswer", + "securitycode", + "securityquestion", + "serviceaccountcredentials", "session", - // django - "csrftoken", + "sessionkey", "sessionid", - // wsgi - "remoteaddr", - "xcsrftoken", - "xforwardedfor", "setcookie", - "cookie", - "authorization", + "signature", + "signaturekey", + "sshkey", + "ssn", + "symfony", + "token", + "transactionid", + "twiliotoken", + "usersession", + "voterid", "xapikey", + "xcsrftoken", "xforwardedfor", - "xrealip"); + "xrealip", + "xauthtoken", + "xsrftoken", + "pwd"); private static final Set KEYWORDS = ConcurrentHashMap.newKeySet(); private static ClassNameTrie typeTrie = ClassNameTrie.Builder.EMPTY_TRIE; private static List redactedClasses; From 6f812d5d3df5eb8b411c4352c6325dcdd3ea2c4f Mon Sep 17 00:00:00 2001 From: jean-philippe bempel Date: Wed, 8 Nov 2023 15:37:45 +0100 Subject: [PATCH 2/6] Add support for synthetic vars for metric probes Add support of `@return` and `@duration` for method metric probes if the return value of the method is compatible with long or double (like integer, byte, short or float) we can use `@return` as input for the metric. `@duration` is now supported as input for metric also the value is a double representing the ms elapsed for the execution of the method --- .../bootstrap/debugger/CapturedContext.java | 2 +- .../instrumentation/MetricInstrumentor.java | 96 +++++++++++++++++-- .../MetricProbesInstrumentationTest.java | 57 +++++++++++ 3 files changed, 145 insertions(+), 10 deletions(-) diff --git a/dd-java-agent/agent-debugger/debugger-bootstrap/src/main/java/datadog/trace/bootstrap/debugger/CapturedContext.java b/dd-java-agent/agent-debugger/debugger-bootstrap/src/main/java/datadog/trace/bootstrap/debugger/CapturedContext.java index 0bb90dc0305..7c01cd8ee56 100644 --- a/dd-java-agent/agent-debugger/debugger-bootstrap/src/main/java/datadog/trace/bootstrap/debugger/CapturedContext.java +++ b/dd-java-agent/agent-debugger/debugger-bootstrap/src/main/java/datadog/trace/bootstrap/debugger/CapturedContext.java @@ -206,7 +206,7 @@ public void addReturn(CapturedValue retValue) { if (locals == null) { locals = new HashMap<>(); } - locals.put("@return", retValue); // special local name for the return value + locals.put(ValueReferences.RETURN_REF, retValue); // special local name for the return value extensions.put(ValueReferences.RETURN_EXTENSION_NAME, retValue); } diff --git a/dd-java-agent/agent-debugger/src/main/java/com/datadog/debugger/instrumentation/MetricInstrumentor.java b/dd-java-agent/agent-debugger/src/main/java/com/datadog/debugger/instrumentation/MetricInstrumentor.java index 38642365cc4..6d361e5c1a9 100644 --- a/dd-java-agent/agent-debugger/src/main/java/com/datadog/debugger/instrumentation/MetricInstrumentor.java +++ b/dd-java-agent/agent-debugger/src/main/java/com/datadog/debugger/instrumentation/MetricInstrumentor.java @@ -11,6 +11,8 @@ import static com.datadog.debugger.instrumentation.ASMHelper.ldc; import static com.datadog.debugger.instrumentation.Types.*; import static datadog.trace.util.Strings.getClassName; +import static org.objectweb.asm.Type.DOUBLE_TYPE; +import static org.objectweb.asm.Type.LONG_TYPE; import com.datadog.debugger.el.InvalidValueException; import com.datadog.debugger.el.Visitor; @@ -46,6 +48,7 @@ import com.datadog.debugger.el.values.StringValue; import com.datadog.debugger.probe.MetricProbe; import com.datadog.debugger.probe.Where; +import datadog.trace.bootstrap.debugger.MethodLocation; import datadog.trace.bootstrap.debugger.el.ValueReferences; import java.lang.reflect.Field; import java.util.ArrayList; @@ -141,6 +144,7 @@ protected InsnList getBeforeReturnInsnList(AbstractInsnNode node) { int size = 1; int storeOpCode = 0; int loadOpCode = 0; + Type returnType = null; switch (node.getOpcode()) { case Opcodes.RET: case Opcodes.RETURN: @@ -149,29 +153,35 @@ protected InsnList getBeforeReturnInsnList(AbstractInsnNode node) { storeOpCode = Opcodes.LSTORE; loadOpCode = Opcodes.LLOAD; size = 2; + returnType = Type.LONG_TYPE; break; case Opcodes.DRETURN: storeOpCode = Opcodes.DSTORE; loadOpCode = Opcodes.DLOAD; size = 2; + returnType = Type.DOUBLE_TYPE; break; case Opcodes.IRETURN: storeOpCode = Opcodes.ISTORE; loadOpCode = Opcodes.ILOAD; + returnType = Type.INT_TYPE; break; case Opcodes.FRETURN: storeOpCode = Opcodes.FSTORE; loadOpCode = Opcodes.FLOAD; + returnType = Type.FLOAT_TYPE; break; case Opcodes.ARETURN: storeOpCode = Opcodes.ASTORE; loadOpCode = Opcodes.ALOAD; + returnType = OBJECT_TYPE; break; default: throw new UnsupportedOperationException("Unsupported opcode: " + node.getOpcode()); } - InsnList insnList = wrapTryCatch(callMetric(metricProbe)); int tmpIdx = newVar(size); + InsnList insnList = + wrapTryCatch(callMetric(metricProbe, new ReturnContext(tmpIdx, loadOpCode, returnType))); // store return value from the stack to local before wrapped call insnList.insert(new VarInsnNode(storeOpCode, tmpIdx)); // restore return value to the stack after wrapped call @@ -179,7 +189,7 @@ protected InsnList getBeforeReturnInsnList(AbstractInsnNode node) { return insnList; } - private InsnList callCount(MetricProbe metricProbe) { + private InsnList callCount(MetricProbe metricProbe, ReturnContext returnContext) { if (metricProbe.getValue() == null) { InsnList insnList = new InsnList(); // consider the metric as an increment counter one @@ -203,16 +213,20 @@ private InsnList callCount(MetricProbe metricProbe) { // stack [] return insnList; } - return internalCallMetric(metricProbe); + return internalCallMetric(metricProbe, returnContext); } - private InsnList internalCallMetric(MetricProbe metricProbe) { + private InsnList internalCallMetric(MetricProbe metricProbe, ReturnContext returnContext) { InsnList insnList = new InsnList(); InsnList nullBranch = new InsnList(); VisitorResult result; Type resultType; try { - result = metricProbe.getValue().getExpr().accept(new MetricValueVisitor(this, nullBranch)); + result = + metricProbe + .getValue() + .getExpr() + .accept(new MetricValueVisitor(this, nullBranch, returnContext)); } catch (InvalidValueException | UnsupportedOperationException ex) { reportError(ex.getMessage()); return EMPTY_INSN_LIST; @@ -271,16 +285,20 @@ private Type convertIfRequired(Type currentType, InsnList insnList) { } private InsnList callMetric(MetricProbe metricProbe) { + return callMetric(metricProbe, null); + } + + private InsnList callMetric(MetricProbe metricProbe, ReturnContext returnContext) { switch (metricProbe.getKind()) { case COUNT: - return callCount(metricProbe); + return callCount(metricProbe, returnContext); case GAUGE: case HISTOGRAM: case DISTRIBUTION: if (metricProbe.getValue() == null) { return EMPTY_INSN_LIST; } - return internalCallMetric(metricProbe); + return internalCallMetric(metricProbe, returnContext); default: reportError(String.format("Unknown metric kind: %s", metricProbe.getKind())); } @@ -332,10 +350,13 @@ public VisitorResult(ASMHelper.Type type, InsnList insnList) { private static class MetricValueVisitor implements Visitor { private final MetricInstrumentor instrumentor; private final InsnList nullBranch; + private final ReturnContext returnContext; - public MetricValueVisitor(MetricInstrumentor instrumentor, InsnList nullBranch) { + public MetricValueVisitor( + MetricInstrumentor instrumentor, InsnList nullBranch, ReturnContext returnContext) { this.instrumentor = instrumentor; this.nullBranch = nullBranch; + this.returnContext = returnContext; } @Override @@ -463,7 +484,11 @@ public VisitorResult visit(ValueRefExpression valueRefExpression) { insnList.add(new VarInsnNode(Opcodes.ALOAD, 0)); // stack [this] } else { - currentType = tryRetrieve(name, insnList); + if (name.startsWith(ValueReferences.SYNTHETIC_PREFIX)) { + currentType = tryRetrieveSynthetic(name, insnList); + } else { + currentType = tryRetrieve(name, insnList); + } if (currentType == null) { throw new InvalidValueException("Cannot resolve symbol " + name); } @@ -727,5 +752,58 @@ private ASMHelper.Type tryRetrieveField( } return returnType; } + + private ASMHelper.Type tryRetrieveSynthetic(String name, InsnList insnList) { + if (name.equals(ValueReferences.RETURN_REF)) { + if (instrumentor.metricProbe.getEvaluateAt() != MethodLocation.EXIT) { + return null; + } + if (returnContext == null) { + return null; + } + VarInsnNode varInsnNode = + new VarInsnNode(returnContext.opLoad, returnContext.returnLocalVarIdx); + insnList.add(varInsnNode); + // stack [return_value] + return new ASMHelper.Type(returnContext.type); + } + if (name.equals(ValueReferences.DURATION_REF)) { + if (instrumentor.metricProbe.getEvaluateAt() != MethodLocation.EXIT) { + return null; + } + // call System.nanoTime at the beginning of the method + int var = instrumentor.newVar(LONG_TYPE); + InsnList nanoTimeList = new InsnList(); + invokeStatic(nanoTimeList, Type.getType(System.class), "nanoTime", LONG_TYPE); + nanoTimeList.add(new VarInsnNode(Opcodes.LSTORE, var)); + instrumentor.methodNode.instructions.insert(instrumentor.methodEnterLabel, nanoTimeList); + // diff nanoTime before calling metric + invokeStatic(insnList, Type.getType(System.class), "nanoTime", LONG_TYPE); + // stack [long] + insnList.add(new VarInsnNode(Opcodes.LLOAD, var)); + // stack [long, long] + insnList.add(new InsnNode(Opcodes.LSUB)); + // stack [long] + insnList.add(new InsnNode(Opcodes.L2D)); + insnList.add(new LdcInsnNode(1_000_000D)); + // stack [long, double] + insnList.add(new InsnNode(Opcodes.DDIV)); + // stack [double] + return new ASMHelper.Type(DOUBLE_TYPE); + } + return null; + } + } + + private static class ReturnContext { + private final int returnLocalVarIdx; + private final int opLoad; + private final Type type; + + public ReturnContext(int returnLocalVarIdx, int opLoad, Type type) { + this.returnLocalVarIdx = returnLocalVarIdx; + this.opLoad = opLoad; + this.type = type; + } } } diff --git a/dd-java-agent/agent-debugger/src/test/java/com/datadog/debugger/agent/MetricProbesInstrumentationTest.java b/dd-java-agent/agent-debugger/src/test/java/com/datadog/debugger/agent/MetricProbesInstrumentationTest.java index d898d7d904a..0fbb89f2a2e 100644 --- a/dd-java-agent/agent-debugger/src/test/java/com/datadog/debugger/agent/MetricProbesInstrumentationTest.java +++ b/dd-java-agent/agent-debugger/src/test/java/com/datadog/debugger/agent/MetricProbesInstrumentationTest.java @@ -366,6 +366,63 @@ public void methodFieldRefValueDistributionDoubleMetric() throws IOException, UR Assertions.assertArrayEquals(new String[] {METRIC_PROBEID_TAG}, listener.lastTags); } + @Test + public void methodSyntheticReturnGaugeMetric() throws IOException, URISyntaxException { + final String CLASS_NAME = "CapturedSnapshot06"; + String METRIC_NAME = "syn_gauge"; + MetricProbe metricProbe = + createMetricBuilder(METRIC_ID, METRIC_NAME, GAUGE) + .where(CLASS_NAME, "f", "()") + .valueScript(new ValueScript(DSL.ref("@return"), "@return")) + .evaluateAt(MethodLocation.EXIT) + .build(); + MetricForwarderListener listener = installMetricProbes(metricProbe); + Class testClass = compileAndLoadClass(CLASS_NAME); + int result = Reflect.on(testClass).call("main", "f").get(); + Assertions.assertEquals(42, result); + Assertions.assertTrue(listener.gauges.containsKey(METRIC_NAME)); + Assertions.assertEquals(42, listener.gauges.get(METRIC_NAME).longValue()); + Assertions.assertArrayEquals(new String[] {METRIC_PROBEID_TAG}, listener.lastTags); + } + + @Test + public void methodSyntheticReturnInvalidType() throws IOException, URISyntaxException { + final String CLASS_NAME = "CapturedSnapshot06"; + final String INHERITED_CLASS_NAME = CLASS_NAME + "$Inherited"; + String METRIC_NAME = "syn_gauge"; + MetricProbe metricProbe = + createMetricBuilder(METRIC_ID, METRIC_NAME, GAUGE) + .where(INHERITED_CLASS_NAME, "", "()") + .valueScript(new ValueScript(DSL.ref("@return"), "@return")) + .evaluateAt(MethodLocation.EXIT) + .build(); + MetricForwarderListener listener = installMetricProbes(metricProbe); + Class testClass = compileAndLoadClass(CLASS_NAME); + int result = Reflect.on(testClass).call("main", "").get(); + Assertions.assertEquals(42, result); + Assertions.assertFalse(listener.gauges.containsKey(METRIC_NAME)); + verify(probeStatusSink).addError(eq(METRIC_ID), eq("Cannot resolve symbol @return")); + } + + @Test + public void methodSyntheticDurationGaugeMetric() throws IOException, URISyntaxException { + final String CLASS_NAME = "CapturedSnapshot06"; + String METRIC_NAME = "syn_gauge"; + MetricProbe metricProbe = + createMetricBuilder(METRIC_ID, METRIC_NAME, GAUGE) + .where(CLASS_NAME, "f", "()") + .valueScript(new ValueScript(DSL.ref("@duration"), "@duration")) + .evaluateAt(MethodLocation.EXIT) + .build(); + MetricForwarderListener listener = installMetricProbes(metricProbe); + Class testClass = compileAndLoadClass(CLASS_NAME); + int result = Reflect.on(testClass).call("main", "f").get(); + Assertions.assertEquals(42, result); + Assertions.assertTrue(listener.doubleGauges.containsKey(METRIC_NAME)); + Assertions.assertTrue(listener.doubleGauges.get(METRIC_NAME).doubleValue() > 0); + Assertions.assertArrayEquals(new String[] {METRIC_PROBEID_TAG}, listener.lastTags); + } + @Test public void lineArgumentRefValueCountMetric() throws IOException, URISyntaxException { final String CLASS_NAME = "CapturedSnapshot03"; From 709f24cfe192ddf39914588b345fb4fc5aa042fe Mon Sep 17 00:00:00 2001 From: jean-philippe bempel Date: Thu, 9 Nov 2023 15:40:47 +0100 Subject: [PATCH 3/6] disable symdb tests for IBM JVM flaky tests that needs to be resolved later --- .../SymbolExtractionTransformerTest.java | 57 +++++++++++++++++++ 1 file changed, 57 insertions(+) diff --git a/dd-java-agent/agent-debugger/src/test/java/com/datadog/debugger/symbol/SymbolExtractionTransformerTest.java b/dd-java-agent/agent-debugger/src/test/java/com/datadog/debugger/symbol/SymbolExtractionTransformerTest.java index b3fda99615f..1d57f4f22b6 100644 --- a/dd-java-agent/agent-debugger/src/test/java/com/datadog/debugger/symbol/SymbolExtractionTransformerTest.java +++ b/dd-java-agent/agent-debugger/src/test/java/com/datadog/debugger/symbol/SymbolExtractionTransformerTest.java @@ -9,6 +9,7 @@ import com.datadog.debugger.sink.SymbolSink; import datadog.trace.api.Config; +import datadog.trace.api.Platform; import java.io.IOException; import java.lang.instrument.Instrumentation; import java.net.URISyntaxException; @@ -55,6 +56,10 @@ public void noIncludesFilterOutDatadogClass() throws IOException, URISyntaxExcep @Test public void symbolExtraction01() throws IOException, URISyntaxException { + if (Platform.isJ9()) { + // Skip for IBM JVM + return; + } final String CLASS_NAME = SYMBOL_PACKAGE + "SymbolExtraction01"; final String SOURCE_FILE = SYMBOL_PACKAGE_DIR + "SymbolExtraction01.java"; SymbolSinkMock symbolSinkMock = new SymbolSinkMock(config); @@ -120,6 +125,10 @@ public void symbolExtraction01() throws IOException, URISyntaxException { @Test public void symbolExtraction02() throws IOException, URISyntaxException { + if (Platform.isJ9()) { + // Skip for IBM JVM + return; + } final String CLASS_NAME = SYMBOL_PACKAGE + "SymbolExtraction02"; final String SOURCE_FILE = SYMBOL_PACKAGE_DIR + "SymbolExtraction02.java"; SymbolSinkMock symbolSinkMock = new SymbolSinkMock(config); @@ -147,6 +156,10 @@ public void symbolExtraction02() throws IOException, URISyntaxException { @Test public void symbolExtraction03() throws IOException, URISyntaxException { + if (Platform.isJ9()) { + // Skip for IBM JVM + return; + } final String CLASS_NAME = SYMBOL_PACKAGE + "SymbolExtraction03"; final String SOURCE_FILE = SYMBOL_PACKAGE_DIR + "SymbolExtraction03.java"; SymbolSinkMock symbolSinkMock = new SymbolSinkMock(config); @@ -214,6 +227,10 @@ public void symbolExtraction03() throws IOException, URISyntaxException { @Test public void symbolExtraction04() throws IOException, URISyntaxException { + if (Platform.isJ9()) { + // Skip for IBM JVM + return; + } final String CLASS_NAME = SYMBOL_PACKAGE + "SymbolExtraction04"; final String SOURCE_FILE = SYMBOL_PACKAGE_DIR + "SymbolExtraction04.java"; SymbolSinkMock symbolSinkMock = new SymbolSinkMock(config); @@ -285,6 +302,10 @@ public void symbolExtraction04() throws IOException, URISyntaxException { @Test public void symbolExtraction05() throws IOException, URISyntaxException { + if (Platform.isJ9()) { + // Skip for IBM JVM + return; + } final String CLASS_NAME = SYMBOL_PACKAGE + "SymbolExtraction05"; final String SOURCE_FILE = SYMBOL_PACKAGE_DIR + "SymbolExtraction05.java"; SymbolSinkMock symbolSinkMock = new SymbolSinkMock(config); @@ -330,6 +351,10 @@ public void symbolExtraction05() throws IOException, URISyntaxException { @Test public void symbolExtraction06() throws IOException, URISyntaxException { + if (Platform.isJ9()) { + // Skip for IBM JVM + return; + } final String CLASS_NAME = SYMBOL_PACKAGE + "SymbolExtraction06"; final String SOURCE_FILE = SYMBOL_PACKAGE_DIR + "SymbolExtraction06.java"; SymbolSinkMock symbolSinkMock = new SymbolSinkMock(config); @@ -375,6 +400,10 @@ public void symbolExtraction06() throws IOException, URISyntaxException { @Test public void symbolExtraction07() throws IOException, URISyntaxException { + if (Platform.isJ9()) { + // Skip for IBM JVM + return; + } final String CLASS_NAME = SYMBOL_PACKAGE + "SymbolExtraction07"; final String SOURCE_FILE = SYMBOL_PACKAGE_DIR + "SymbolExtraction07.java"; SymbolSinkMock symbolSinkMock = new SymbolSinkMock(config); @@ -406,6 +435,10 @@ public void symbolExtraction07() throws IOException, URISyntaxException { @Test public void symbolExtraction08() throws IOException, URISyntaxException { + if (Platform.isJ9()) { + // Skip for IBM JVM + return; + } final String CLASS_NAME = SYMBOL_PACKAGE + "SymbolExtraction08"; final String SOURCE_FILE = SYMBOL_PACKAGE_DIR + "SymbolExtraction08.java"; SymbolSinkMock symbolSinkMock = new SymbolSinkMock(config); @@ -439,6 +472,10 @@ public void symbolExtraction08() throws IOException, URISyntaxException { @Test public void symbolExtraction09() throws IOException, URISyntaxException { + if (Platform.isJ9()) { + // Skip for IBM JVM + return; + } final String CLASS_NAME = SYMBOL_PACKAGE + "SymbolExtraction09"; final String SOURCE_FILE = SYMBOL_PACKAGE_DIR + "SymbolExtraction09.java"; SymbolSinkMock symbolSinkMock = new SymbolSinkMock(config); @@ -530,6 +567,10 @@ public void symbolExtraction09() throws IOException, URISyntaxException { @Test public void symbolExtraction10() throws IOException, URISyntaxException { + if (Platform.isJ9()) { + // Skip for IBM JVM + return; + } final String CLASS_NAME = SYMBOL_PACKAGE + "SymbolExtraction10"; final String SOURCE_FILE = SYMBOL_PACKAGE_DIR + "SymbolExtraction10.java"; when(config.getDebuggerSymbolFlushThreshold()).thenReturn(2); @@ -581,6 +622,10 @@ public void symbolExtraction10() throws IOException, URISyntaxException { @Test public void symbolExtraction11() throws IOException, URISyntaxException { + if (Platform.isJ9()) { + // Skip for IBM JVM + return; + } final String CLASS_NAME = SYMBOL_PACKAGE + "SymbolExtraction11"; final String SOURCE_FILE = SYMBOL_PACKAGE_DIR + "SymbolExtraction11.java"; SymbolSinkMock symbolSinkMock = new SymbolSinkMock(config); @@ -614,6 +659,10 @@ public void symbolExtraction11() throws IOException, URISyntaxException { @Test public void symbolExtraction12() throws IOException, URISyntaxException { + if (Platform.isJ9()) { + // Skip for IBM JVM + return; + } final String CLASS_NAME = SYMBOL_PACKAGE + "SymbolExtraction12"; final String SOURCE_FILE = SYMBOL_PACKAGE_DIR + "SymbolExtraction12.java"; SymbolSinkMock symbolSinkMock = new SymbolSinkMock(config); @@ -687,6 +736,10 @@ public void symbolExtraction12() throws IOException, URISyntaxException { @Test public void symbolExtraction13() throws IOException, URISyntaxException { + if (Platform.isJ9()) { + // Skip for IBM JVM + return; + } final String CLASS_NAME = SYMBOL_PACKAGE + "SymbolExtraction13"; SymbolSinkMock symbolSinkMock = new SymbolSinkMock(config); SymbolExtractionTransformer transformer = @@ -748,6 +801,10 @@ public void symbolExtraction13() throws IOException, URISyntaxException { @Test public void symbolExtraction14() throws IOException, URISyntaxException { + if (Platform.isJ9()) { + // Skip for IBM JVM + return; + } final String CLASS_NAME = SYMBOL_PACKAGE + "SymbolExtraction14"; SymbolSinkMock symbolSinkMock = new SymbolSinkMock(config); SymbolExtractionTransformer transformer = From 376c39b5802a43bc4d56bafa37560fb49d5217f5 Mon Sep 17 00:00:00 2001 From: Bruce Bujon Date: Fri, 10 Nov 2023 08:33:13 +0100 Subject: [PATCH 4/6] Add span operation naming convention (#6104) * feat: Add span operation naming convention * feat: Update OTel smoke tests --- .../OpenTelemetryInstrumentation.java | 2 + .../OpenTelemetryContextInstrumentation.java | 2 + ...elemetryContextStorageInstrumentation.java | 2 + .../trace/OtelConventions.java | 230 ++++++++++++++++++ .../opentelemetry14/trace/OtelSpan.java | 5 +- .../trace/OtelSpanBuilder.java | 19 +- .../opentelemetry14/trace/OtelTracer.java | 6 +- .../OpenTelemetry14ConventionsTest.groovy | 169 +++++++++++++ .../test/groovy/OpenTelemetry14Test.groovy | 46 ++-- .../context/ContextTest.groovy | 13 +- .../propagation/AbstractPropagatorTest.groovy | 5 +- .../app/actions/AbstractAction.java | 8 +- .../app/filters/AbstractFilter.java | 15 +- .../smoketest/Play28OTelSmokeTest.groovy | 20 +- 14 files changed, 477 insertions(+), 65 deletions(-) create mode 100644 dd-java-agent/instrumentation/opentelemetry/opentelemetry-1.4/src/main/java/datadog/trace/instrumentation/opentelemetry14/trace/OtelConventions.java create mode 100644 dd-java-agent/instrumentation/opentelemetry/opentelemetry-1.4/src/test/groovy/OpenTelemetry14ConventionsTest.groovy diff --git a/dd-java-agent/instrumentation/opentelemetry/opentelemetry-1.4/src/main/java/datadog/trace/instrumentation/opentelemetry14/OpenTelemetryInstrumentation.java b/dd-java-agent/instrumentation/opentelemetry/opentelemetry-1.4/src/main/java/datadog/trace/instrumentation/opentelemetry14/OpenTelemetryInstrumentation.java index 35f0425cb90..02b3bd9b12d 100644 --- a/dd-java-agent/instrumentation/opentelemetry/opentelemetry-1.4/src/main/java/datadog/trace/instrumentation/opentelemetry14/OpenTelemetryInstrumentation.java +++ b/dd-java-agent/instrumentation/opentelemetry/opentelemetry-1.4/src/main/java/datadog/trace/instrumentation/opentelemetry14/OpenTelemetryInstrumentation.java @@ -64,6 +64,8 @@ public String[] helperClassNames() { packageName + ".context.propagation.AgentTextMapPropagator", packageName + ".context.propagation.OtelContextPropagators", packageName + ".trace.OtelExtractedContext", + packageName + ".trace.OtelConventions", + packageName + ".trace.OtelConventions$1", packageName + ".trace.OtelSpan", packageName + ".trace.OtelSpan$1", packageName + ".trace.OtelSpan$NoopSpan", diff --git a/dd-java-agent/instrumentation/opentelemetry/opentelemetry-1.4/src/main/java/datadog/trace/instrumentation/opentelemetry14/context/OpenTelemetryContextInstrumentation.java b/dd-java-agent/instrumentation/opentelemetry/opentelemetry-1.4/src/main/java/datadog/trace/instrumentation/opentelemetry14/context/OpenTelemetryContextInstrumentation.java index df0676a5d6d..c8e5a6c193b 100644 --- a/dd-java-agent/instrumentation/opentelemetry/opentelemetry-1.4/src/main/java/datadog/trace/instrumentation/opentelemetry14/context/OpenTelemetryContextInstrumentation.java +++ b/dd-java-agent/instrumentation/opentelemetry/opentelemetry-1.4/src/main/java/datadog/trace/instrumentation/opentelemetry14/context/OpenTelemetryContextInstrumentation.java @@ -56,6 +56,8 @@ public String[] helperClassNames() { packageName + ".OtelContext", packageName + ".OtelScope", ROOT_PACKAGE_NAME + ".trace.OtelExtractedContext", + ROOT_PACKAGE_NAME + ".trace.OtelConventions", + ROOT_PACKAGE_NAME + ".trace.OtelConventions$1", ROOT_PACKAGE_NAME + ".trace.OtelSpan", ROOT_PACKAGE_NAME + ".trace.OtelSpan$1", ROOT_PACKAGE_NAME + ".trace.OtelSpan$NoopSpan", diff --git a/dd-java-agent/instrumentation/opentelemetry/opentelemetry-1.4/src/main/java/datadog/trace/instrumentation/opentelemetry14/context/OpenTelemetryContextStorageInstrumentation.java b/dd-java-agent/instrumentation/opentelemetry/opentelemetry-1.4/src/main/java/datadog/trace/instrumentation/opentelemetry14/context/OpenTelemetryContextStorageInstrumentation.java index b7130cd55a3..a5e0537e074 100644 --- a/dd-java-agent/instrumentation/opentelemetry/opentelemetry-1.4/src/main/java/datadog/trace/instrumentation/opentelemetry14/context/OpenTelemetryContextStorageInstrumentation.java +++ b/dd-java-agent/instrumentation/opentelemetry/opentelemetry-1.4/src/main/java/datadog/trace/instrumentation/opentelemetry14/context/OpenTelemetryContextStorageInstrumentation.java @@ -62,6 +62,8 @@ public String[] helperClassNames() { packageName + ".OtelContext", packageName + ".OtelScope", ROOT_PACKAGE_NAME + ".trace.OtelExtractedContext", + ROOT_PACKAGE_NAME + ".trace.OtelConventions", + ROOT_PACKAGE_NAME + ".trace.OtelConventions$1", ROOT_PACKAGE_NAME + ".trace.OtelSpan", ROOT_PACKAGE_NAME + ".trace.OtelSpan$1", ROOT_PACKAGE_NAME + ".trace.OtelSpan$NoopSpan", diff --git a/dd-java-agent/instrumentation/opentelemetry/opentelemetry-1.4/src/main/java/datadog/trace/instrumentation/opentelemetry14/trace/OtelConventions.java b/dd-java-agent/instrumentation/opentelemetry/opentelemetry-1.4/src/main/java/datadog/trace/instrumentation/opentelemetry14/trace/OtelConventions.java new file mode 100644 index 00000000000..03dba691ee9 --- /dev/null +++ b/dd-java-agent/instrumentation/opentelemetry/opentelemetry-1.4/src/main/java/datadog/trace/instrumentation/opentelemetry14/trace/OtelConventions.java @@ -0,0 +1,230 @@ +package datadog.trace.instrumentation.opentelemetry14.trace; + +import static datadog.trace.api.DDTags.ANALYTICS_SAMPLE_RATE; +import static datadog.trace.bootstrap.instrumentation.api.Tags.SPAN_KIND_CLIENT; +import static datadog.trace.bootstrap.instrumentation.api.Tags.SPAN_KIND_CONSUMER; +import static datadog.trace.bootstrap.instrumentation.api.Tags.SPAN_KIND_PRODUCER; +import static datadog.trace.bootstrap.instrumentation.api.Tags.SPAN_KIND_SERVER; +import static io.opentelemetry.api.trace.SpanKind.CLIENT; +import static io.opentelemetry.api.trace.SpanKind.CONSUMER; +import static io.opentelemetry.api.trace.SpanKind.INTERNAL; +import static io.opentelemetry.api.trace.SpanKind.PRODUCER; +import static io.opentelemetry.api.trace.SpanKind.SERVER; +import static java.util.Locale.ROOT; + +import datadog.trace.bootstrap.instrumentation.api.AgentSpan; +import io.opentelemetry.api.trace.SpanKind; +import javax.annotation.Nullable; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +public final class OtelConventions { + /** The Datadog span default operation name. */ + public static final String DEFAULT_OPERATION_NAME = "otel_unknown"; + + private static final Logger LOGGER = LoggerFactory.getLogger(OtelConventions.class); + private static final String OPERATION_NAME_SPECIFIC_ATTRIBUTE = "operation.name"; + private static final String SERVICE_NAME_SPECIFIC_ATTRIBUTE = "service.name"; + private static final String RESOURCE_NAME_SPECIFIC_ATTRIBUTE = "resource.name"; + private static final String SPAN_TYPE_SPECIFIC_ATTRIBUTES = "span.type"; + private static final String ANALYTICS_EVENT_SPECIFIC_ATTRIBUTES = "analytics.event"; + private static final String SPAN_KIND_INTERNAL = "internal"; + + private OtelConventions() {} + + /** + * Convert OpenTelemetry {@link SpanKind} to Datadog span type. + * + * @param spanKind The OpenTelemetry span kind to convert. + * @return The related Datadog span type. + */ + public static String toSpanType(SpanKind spanKind) { + switch (spanKind) { + case CLIENT: + return SPAN_KIND_CLIENT; + case SERVER: + return SPAN_KIND_SERVER; + case PRODUCER: + return SPAN_KIND_PRODUCER; + case CONSUMER: + return SPAN_KIND_CONSUMER; + case INTERNAL: + return SPAN_KIND_INTERNAL; + default: + return spanKind.toString().toLowerCase(ROOT); + } + } + + /** + * Convert Datadog span type to OpenTelemetry {@link SpanKind}. + * + * @param spanType The span type to convert. + * @return The related OpenTelemetry {@link SpanKind}. + */ + public static SpanKind toSpanKind(String spanType) { + if (spanType == null) { + return INTERNAL; + } + switch (spanType) { + case SPAN_KIND_CLIENT: + return CLIENT; + case SPAN_KIND_SERVER: + return SERVER; + case SPAN_KIND_PRODUCER: + return PRODUCER; + case SPAN_KIND_CONSUMER: + return CONSUMER; + default: + return INTERNAL; + } + } + + public static void applyConventions(AgentSpan span) { + String serviceName = getStringAttribute(span, SERVICE_NAME_SPECIFIC_ATTRIBUTE); + if (serviceName != null) { + span.setServiceName(serviceName); + } + String resourceName = getStringAttribute(span, RESOURCE_NAME_SPECIFIC_ATTRIBUTE); + if (resourceName != null) { + span.setResourceName(resourceName); + } + String spanType = getStringAttribute(span, SPAN_TYPE_SPECIFIC_ATTRIBUTES); + if (spanType != null) { + span.setSpanType(spanType); + } + Boolean analyticsEvent = getBooleanAttribute(span, ANALYTICS_EVENT_SPECIFIC_ATTRIBUTES); + if (analyticsEvent != null) { + span.setMetric(ANALYTICS_SAMPLE_RATE, analyticsEvent ? 1 : 0); + } + applyOperationName(span); + } + + private static void applyOperationName(AgentSpan span) { + String operationName = getStringAttribute(span, OPERATION_NAME_SPECIFIC_ATTRIBUTE); + if (operationName == null) { + operationName = computeOperationName(span); + } + span.setOperationName(operationName.toLowerCase(ROOT)); + } + + private static String computeOperationName(AgentSpan span) { + SpanKind spanKind = toSpanKind(span.getSpanType()); + /* + * HTTP convention: https://opentelemetry.io/docs/specs/otel/trace/semantic_conventions/http/ + */ + String httpRequestMethod = getStringAttribute(span, "http.request.method"); + if (spanKind == SERVER && httpRequestMethod != null) { + return "http.server.request"; + } + if (spanKind == CLIENT && httpRequestMethod != null) { + return "http.client.request"; + } + /* + * Database convention: https://opentelemetry.io/docs/specs/semconv/database/database-spans/ + */ + String dbSystem = getStringAttribute(span, "db.system"); + if (spanKind == CLIENT && dbSystem != null) { + return dbSystem + ".query"; + } + /* + * Messaging: https://opentelemetry.io/docs/specs/semconv/messaging/messaging-spans/ + */ + String messagingSystem = getStringAttribute(span, "messaging.system"); + String messagingOperation = getStringAttribute(span, "messaging.operation"); + if ((spanKind == CONSUMER || spanKind == PRODUCER || spanKind == CLIENT || spanKind == SERVER) + && messagingSystem != null + && messagingOperation != null) { + return messagingSystem + "." + messagingOperation; + } + /* + * AWS: https://opentelemetry.io/docs/specs/semconv/cloud-providers/aws-sdk/ + */ + String rpcSystem = getStringAttribute(span, "rpc.system"); + if (spanKind == CLIENT && "aws-api".equals(rpcSystem)) { + String service = getStringAttribute(span, "rpc.service"); + if (service == null) { + return "aws.client.request"; + } else { + return "aws." + service + ".request"; + } + } + /* + * RPC: https://opentelemetry.io/docs/specs/semconv/rpc/rpc-spans/ + */ + if (spanKind == CLIENT && rpcSystem != null) { + return rpcSystem + ".client.request"; + } + if (spanKind == SERVER && rpcSystem != null) { + return rpcSystem + ".server.request"; + } + /* + * FaaS: + * https://opentelemetry.io/docs/specs/semconv/faas/faas-spans/#incoming-faas-span-attributes + * https://opentelemetry.io/docs/specs/semconv/faas/faas-spans/#outgoing-invocations + */ + String faasInvokedProvider = getStringAttribute(span, "faas.invoked_provider"); + String faasInvokedName = getStringAttribute(span, "faas.invoked_name"); + if (spanKind == CLIENT && faasInvokedProvider != null && faasInvokedName != null) { + return faasInvokedProvider + "." + faasInvokedName + ".invoke"; + } + String faasTrigger = getStringAttribute(span, "faas.trigger"); + if (spanKind == SERVER && faasTrigger != null) { + return faasTrigger + ".invoke"; + } + /* + * GraphQL: https://opentelemetry.io/docs/specs/otel/trace/semantic_conventions/instrumentation/graphql/ + */ + String graphqlOperationType = getStringAttribute(span, "graphql.operation.type"); + if (graphqlOperationType != null) { + return "graphql.server.request"; + } + /* + * Generic server / client: https://opentelemetry.io/docs/specs/semconv/http/http-spans/ + */ + String networkProtocolName = getStringAttribute(span, "network.protocol.name"); + if (spanKind == SERVER) { + return networkProtocolName == null + ? "server.request" + : networkProtocolName + ".server.request"; + } + if (spanKind == CLIENT) { + return networkProtocolName == null + ? "client.request" + : networkProtocolName + ".client.request"; + } + // Fallback if no convention match + if (span.getSpanType() != null) { + return spanKind.name(); + } else { + return DEFAULT_OPERATION_NAME; + } + } + + @Nullable + private static String getStringAttribute(AgentSpan span, String key) { + Object tag = span.getTag(key); + if (tag == null) { + return null; + } else if (!(tag instanceof String)) { + LOGGER.debug("Span attributes {} is not a string", key); + return key; + } + return (String) tag; + } + + @Nullable + private static Boolean getBooleanAttribute(AgentSpan span, String key) { + Object tag = span.getTag(key); + if (tag == null) { + return null; + } + if (tag instanceof Boolean) { + return (Boolean) tag; + } else if (tag instanceof String) { + return Boolean.parseBoolean((String) tag); + } else { + LOGGER.debug("Span attributes {} is not a boolean", key); + return null; + } + } +} diff --git a/dd-java-agent/instrumentation/opentelemetry/opentelemetry-1.4/src/main/java/datadog/trace/instrumentation/opentelemetry14/trace/OtelSpan.java b/dd-java-agent/instrumentation/opentelemetry/opentelemetry-1.4/src/main/java/datadog/trace/instrumentation/opentelemetry14/trace/OtelSpan.java index 1d4cb72aa16..9845e531558 100644 --- a/dd-java-agent/instrumentation/opentelemetry/opentelemetry-1.4/src/main/java/datadog/trace/instrumentation/opentelemetry14/trace/OtelSpan.java +++ b/dd-java-agent/instrumentation/opentelemetry/opentelemetry-1.4/src/main/java/datadog/trace/instrumentation/opentelemetry14/trace/OtelSpan.java @@ -1,6 +1,7 @@ package datadog.trace.instrumentation.opentelemetry14.trace; import static datadog.trace.bootstrap.instrumentation.api.AgentTracer.activateSpan; +import static datadog.trace.instrumentation.opentelemetry14.trace.OtelConventions.applyConventions; import static io.opentelemetry.api.trace.StatusCode.ERROR; import static io.opentelemetry.api.trace.StatusCode.OK; import static io.opentelemetry.api.trace.StatusCode.UNSET; @@ -106,7 +107,7 @@ public Span recordException(Throwable exception, Attributes additionalAttributes @Override public Span updateName(String name) { if (this.recording) { - this.delegate.setOperationName(name); + this.delegate.setResourceName(name); } return this; } @@ -114,12 +115,14 @@ public Span updateName(String name) { @Override public void end() { this.recording = false; + applyConventions(this.delegate); this.delegate.finish(); } @Override public void end(long timestamp, TimeUnit unit) { this.recording = false; + applyConventions(this.delegate); this.delegate.finish(unit.toMicros(timestamp)); } diff --git a/dd-java-agent/instrumentation/opentelemetry/opentelemetry-1.4/src/main/java/datadog/trace/instrumentation/opentelemetry14/trace/OtelSpanBuilder.java b/dd-java-agent/instrumentation/opentelemetry/opentelemetry-1.4/src/main/java/datadog/trace/instrumentation/opentelemetry14/trace/OtelSpanBuilder.java index b39118edcc9..aca54bda20b 100644 --- a/dd-java-agent/instrumentation/opentelemetry/opentelemetry-1.4/src/main/java/datadog/trace/instrumentation/opentelemetry14/trace/OtelSpanBuilder.java +++ b/dd-java-agent/instrumentation/opentelemetry/opentelemetry-1.4/src/main/java/datadog/trace/instrumentation/opentelemetry14/trace/OtelSpanBuilder.java @@ -1,10 +1,10 @@ package datadog.trace.instrumentation.opentelemetry14.trace; +import static datadog.trace.instrumentation.opentelemetry14.trace.OtelConventions.toSpanType; import static datadog.trace.instrumentation.opentelemetry14.trace.OtelExtractedContext.extract; import datadog.trace.bootstrap.instrumentation.api.AgentSpan; import datadog.trace.bootstrap.instrumentation.api.AgentTracer; -import datadog.trace.bootstrap.instrumentation.api.Tags; import io.opentelemetry.api.common.AttributeKey; import io.opentelemetry.api.common.Attributes; import io.opentelemetry.api.trace.Span; @@ -13,7 +13,6 @@ import io.opentelemetry.api.trace.SpanKind; import io.opentelemetry.context.Context; import java.util.List; -import java.util.Locale; import java.util.concurrent.TimeUnit; import javax.annotation.ParametersAreNonnullByDefault; @@ -110,22 +109,6 @@ public SpanBuilder setSpanKind(SpanKind spanKind) { return this; } - private static String toSpanType(SpanKind spanKind) { - switch (spanKind) { - case CLIENT: - return Tags.SPAN_KIND_CLIENT; - case SERVER: - return Tags.SPAN_KIND_SERVER; - case PRODUCER: - return Tags.SPAN_KIND_PRODUCER; - case CONSUMER: - return Tags.SPAN_KIND_CONSUMER; - default: - case INTERNAL: - return spanKind.toString().toLowerCase(Locale.ROOT); - } - } - @Override public SpanBuilder setStartTimestamp(long startTimestamp, TimeUnit unit) { this.delegate.withStartTimestamp(unit.toMicros(startTimestamp)); diff --git a/dd-java-agent/instrumentation/opentelemetry/opentelemetry-1.4/src/main/java/datadog/trace/instrumentation/opentelemetry14/trace/OtelTracer.java b/dd-java-agent/instrumentation/opentelemetry/opentelemetry-1.4/src/main/java/datadog/trace/instrumentation/opentelemetry14/trace/OtelTracer.java index 0ce1f69bbd9..9d551550063 100644 --- a/dd-java-agent/instrumentation/opentelemetry/opentelemetry-1.4/src/main/java/datadog/trace/instrumentation/opentelemetry14/trace/OtelTracer.java +++ b/dd-java-agent/instrumentation/opentelemetry/opentelemetry-1.4/src/main/java/datadog/trace/instrumentation/opentelemetry14/trace/OtelTracer.java @@ -1,5 +1,7 @@ package datadog.trace.instrumentation.opentelemetry14.trace; +import static datadog.trace.instrumentation.opentelemetry14.trace.OtelConventions.DEFAULT_OPERATION_NAME; + import datadog.trace.bootstrap.instrumentation.api.AgentTracer; import io.opentelemetry.api.trace.SpanBuilder; import io.opentelemetry.api.trace.Tracer; @@ -19,7 +21,9 @@ public OtelTracer(String instrumentationScopeName) { @Override public SpanBuilder spanBuilder(String spanName) { AgentTracer.SpanBuilder delegate = - this.tracer.buildSpan(INSTRUMENTATION_NAME, spanName).withResourceName(spanName); + this.tracer + .buildSpan(INSTRUMENTATION_NAME, DEFAULT_OPERATION_NAME) + .withResourceName(spanName); return new OtelSpanBuilder(delegate); } } diff --git a/dd-java-agent/instrumentation/opentelemetry/opentelemetry-1.4/src/test/groovy/OpenTelemetry14ConventionsTest.groovy b/dd-java-agent/instrumentation/opentelemetry/opentelemetry-1.4/src/test/groovy/OpenTelemetry14ConventionsTest.groovy new file mode 100644 index 00000000000..558e09162b4 --- /dev/null +++ b/dd-java-agent/instrumentation/opentelemetry/opentelemetry-1.4/src/test/groovy/OpenTelemetry14ConventionsTest.groovy @@ -0,0 +1,169 @@ +import datadog.trace.agent.test.AgentTestRunner +import datadog.trace.api.DDTags +import io.opentelemetry.api.GlobalOpenTelemetry +import io.opentelemetry.context.Context +import io.opentelemetry.context.ThreadLocalContextStorage +import spock.lang.Subject + +import static datadog.trace.bootstrap.instrumentation.api.Tags.SPAN_KIND_CLIENT +import static datadog.trace.bootstrap.instrumentation.api.Tags.SPAN_KIND_SERVER +import static datadog.trace.instrumentation.opentelemetry14.trace.OtelConventions.DEFAULT_OPERATION_NAME +import static datadog.trace.instrumentation.opentelemetry14.trace.OtelConventions.toSpanType +import static io.opentelemetry.api.trace.SpanKind.CLIENT +import static io.opentelemetry.api.trace.SpanKind.CONSUMER +import static io.opentelemetry.api.trace.SpanKind.INTERNAL +import static io.opentelemetry.api.trace.SpanKind.PRODUCER +import static io.opentelemetry.api.trace.SpanKind.SERVER + +class OpenTelemetry14ConventionsTest extends AgentTestRunner { + @Subject + def tracer = GlobalOpenTelemetry.get().tracerProvider.get("conventions") + + @Override + void configurePreAgent() { + super.configurePreAgent() + + injectSysConfig("dd.integration.opentelemetry.experimental.enabled", "true") + } + + def "test span name conventions"() { + when: + def builder = tracer.spanBuilder("some-name") + if (kind != null) { + builder.setSpanKind(kind) + } + attributes.forEach { key, value -> builder.setAttribute(key, value) } + builder.startSpan() + .end() + + then: + assertTraces(1) { + trace(1) { + span { + parent() + if (kind != null) { + spanType toSpanType(kind) + } + operationName "$expectedOperationName" + resourceName "some-name" + } + } + } + + where: + kind | attributes | expectedOperationName + // Fallback behavior + null | [:] | DEFAULT_OPERATION_NAME + // Internal spans + INTERNAL | [:] | "internal" + // Server spans + SERVER | [:] | "server.request" + SERVER | ["http.request.method": "GET"] | "http.server.request" + SERVER | ["http.request.method": "GET"] | "http.server.request" + SERVER | ["network.protocol.name": "amqp"] | "amqp.server.request" + // Client spans + CLIENT | [:] | "client.request" + CLIENT | ["http.request.method": "GET"] | "http.client.request" + CLIENT | ["db.system": "mysql"] | "mysql.query" + CLIENT | ["network.protocol.name": "amqp"] | "amqp.client.request" + CLIENT | ["network.protocol.name": "AMQP"] | "amqp.client.request" + // Messaging spans + PRODUCER | [:] | "producer" + CONSUMER | [:] | "consumer" + CONSUMER | ["messaging.system": "rabbitmq", "messaging.operation": "publish"] | "rabbitmq.publish" + PRODUCER | ["messaging.system": "rabbitmq", "messaging.operation": "publish"] | "rabbitmq.publish" + CLIENT | ["messaging.system": "rabbitmq", "messaging.operation": "publish"] | "rabbitmq.publish" + SERVER | ["messaging.system": "rabbitmq", "messaging.operation": "publish"] | "rabbitmq.publish" + // RPC spans + CLIENT | ["rpc.system": "grpc"] | "grpc.client.request" + SERVER | ["rpc.system": "grpc"] | "grpc.server.request" + CLIENT | ["rpc.system": "aws-api"] | "aws.client.request" + CLIENT | ["rpc.system": "aws-api", "rpc.service": "helloworld"] | "aws.helloworld.request" + SERVER | ["rpc.system": "aws-api"] | "aws-api.server.request" + // FAAS spans + CLIENT | ["faas.invoked_provider": "alibaba_cloud", "faas.invoked_name": "my-function"] | "alibaba_cloud.my-function.invoke" + SERVER | ["faas.trigger": "datasource"] | "datasource.invoke" + // GraphQL spans + INTERNAL | ["graphql.operation.type": "query"] | "graphql.server.request" + null | ["graphql.operation.type": "query"] | "graphql.server.request" + // User override + CLIENT | ["db.system": "mysql", "operation.name": "db.query"] | "db.query" + CLIENT | ["db.system": "mysql", "operation.name": "DB.query"] | "db.query" + } + + def "test span specific tags"() { + when: + tracer.spanBuilder("some-name") + .setAttribute("service.name", "my-service") + .setAttribute("resource.name", "/my-resource") + .setAttribute("span.type", "$type") + .startSpan() + .end() + + + then: + assertTraces(1) { + trace(1) { + span { + parent() + spanType "$type" + operationName "$expectedOperationName" + resourceName "/my-resource" + serviceName "my-service" + } + } + } + + where: + type | expectedOperationName + SPAN_KIND_SERVER | "server.request" + SPAN_KIND_CLIENT | "client.request" + } + + def "test span analytics.event specific tag"() { + when: + tracer.spanBuilder("some-name") + .setAttribute("analytics.event", value) + .startSpan() + .end() + + + then: + assertTraces(1) { + trace(1) { + span { + parent() + operationName "$DEFAULT_OPERATION_NAME" + tags { + defaultTags() + if (value != null) { + "analytics.event" value + "$DDTags.ANALYTICS_SAMPLE_RATE" expectedMetric + } + } + } + } + } + + where: + value | expectedMetric + true | 1 + Boolean.TRUE | 1 + false | 0 + Boolean.FALSE | 0 + null | 0 // Not used + "true" | 1 + "false" | 0 + "TRUE" | 1 + "something-else" | 0 + "" | 0 + } + + @Override + void cleanup() { + // Test for context leak + assert Context.current() == Context.root() + // Safely reset OTel context storage + ThreadLocalContextStorage.THREAD_LOCAL_STORAGE.remove() + } +} diff --git a/dd-java-agent/instrumentation/opentelemetry/opentelemetry-1.4/src/test/groovy/OpenTelemetry14Test.groovy b/dd-java-agent/instrumentation/opentelemetry/opentelemetry-1.4/src/test/groovy/OpenTelemetry14Test.groovy index ca0952048e5..fb529889e59 100644 --- a/dd-java-agent/instrumentation/opentelemetry/opentelemetry-1.4/src/test/groovy/OpenTelemetry14Test.groovy +++ b/dd-java-agent/instrumentation/opentelemetry/opentelemetry-1.4/src/test/groovy/OpenTelemetry14Test.groovy @@ -10,6 +10,9 @@ import spock.lang.Subject import java.security.InvalidParameterException +import static datadog.trace.bootstrap.instrumentation.api.Tags.SPAN_KIND_SERVER +import static datadog.trace.instrumentation.opentelemetry14.trace.OtelConventions.DEFAULT_OPERATION_NAME +import static io.opentelemetry.api.trace.SpanKind.SERVER import static io.opentelemetry.api.trace.StatusCode.ERROR import static io.opentelemetry.api.trace.StatusCode.OK import static io.opentelemetry.api.trace.StatusCode.UNSET @@ -41,12 +44,12 @@ class OpenTelemetry14Test extends AgentTestRunner { trace(2) { span { parent() - operationName "some-name" + operationName DEFAULT_OPERATION_NAME resourceName "some-name" } span { childOfPrevious() - operationName "other-name" + operationName DEFAULT_OPERATION_NAME resourceName "other-name" } } @@ -69,11 +72,13 @@ class OpenTelemetry14Test extends AgentTestRunner { trace(2) { span { parent() - operationName "some-name" + operationName DEFAULT_OPERATION_NAME + resourceName "some-name" } span { childOfPrevious() - operationName "other-name" + operationName DEFAULT_OPERATION_NAME + resourceName "other-name" } } } @@ -90,7 +95,6 @@ class OpenTelemetry14Test extends AgentTestRunner { TEST_WRITER.waitForTraces(1) def trace = TEST_WRITER.firstTrace() - then: trace.size() == 1 trace[0].spanId != 0 @@ -114,13 +118,15 @@ class OpenTelemetry14Test extends AgentTestRunner { trace(1) { span { parent() - operationName "some-name" + operationName DEFAULT_OPERATION_NAME + resourceName"some-name" } } trace(1) { span { parent() - operationName "other-name" + operationName DEFAULT_OPERATION_NAME + resourceName"other-name" } } } @@ -195,7 +201,7 @@ class OpenTelemetry14Test extends AgentTestRunner { trace(1) { span { parent() - operationName "some-name" + operationName DEFAULT_OPERATION_NAME if (tagSpan) { resourceName "other-resource" } else if (tagBuilder) { @@ -280,7 +286,7 @@ class OpenTelemetry14Test extends AgentTestRunner { SpanKind.CONSUMER | Tags.SPAN_KIND_CONSUMER SpanKind.INTERNAL | "internal" SpanKind.PRODUCER | Tags.SPAN_KIND_PRODUCER - SpanKind.SERVER | Tags.SPAN_KIND_SERVER + SERVER | Tags.SPAN_KIND_SERVER } def "test span error status"() { @@ -297,7 +303,7 @@ class OpenTelemetry14Test extends AgentTestRunner { trace(1) { span { parent() - operationName "some-name" + operationName DEFAULT_OPERATION_NAME resourceName "some-name" errored true @@ -349,7 +355,7 @@ class OpenTelemetry14Test extends AgentTestRunner { trace(1) { span { parent() - operationName "some-name" + operationName DEFAULT_OPERATION_NAME resourceName "some-name" errored false tags { @@ -390,7 +396,7 @@ class OpenTelemetry14Test extends AgentTestRunner { trace(1) { span { parent() - operationName "some-name" + operationName DEFAULT_OPERATION_NAME resourceName "some-name" errored false tags { @@ -405,16 +411,18 @@ class OpenTelemetry14Test extends AgentTestRunner { def "test span name update"() { setup: def builder = tracer.spanBuilder("some-name") - def result = builder.startSpan() + def result = builder.setSpanKind(SERVER).startSpan() expect: - result.delegate.operationName == "some-name" + result.delegate.operationName == DEFAULT_OPERATION_NAME + result.delegate.resourceName == "some-name" when: result.updateName("other-name") then: - result.delegate.operationName == "other-name" + result.delegate.operationName == DEFAULT_OPERATION_NAME + result.delegate.resourceName == "other-name" when: result.end() @@ -424,8 +432,9 @@ class OpenTelemetry14Test extends AgentTestRunner { trace(1) { span { parent() - operationName "other-name" - resourceName "some-name" + spanType SPAN_KIND_SERVER + operationName "server.request" + resourceName "other-name" } } } @@ -449,7 +458,8 @@ class OpenTelemetry14Test extends AgentTestRunner { trace(1) { span { parent() - operationName "some-name" + operationName DEFAULT_OPERATION_NAME + resourceName"some-name" errored true tags { defaultTags() diff --git a/dd-java-agent/instrumentation/opentelemetry/opentelemetry-1.4/src/test/groovy/opentelemetry14/context/ContextTest.groovy b/dd-java-agent/instrumentation/opentelemetry/opentelemetry-1.4/src/test/groovy/opentelemetry14/context/ContextTest.groovy index 3c1805133a8..bd834f9aefb 100644 --- a/dd-java-agent/instrumentation/opentelemetry/opentelemetry-1.4/src/test/groovy/opentelemetry14/context/ContextTest.groovy +++ b/dd-java-agent/instrumentation/opentelemetry/opentelemetry-1.4/src/test/groovy/opentelemetry14/context/ContextTest.groovy @@ -12,6 +12,7 @@ import spock.lang.Subject import static datadog.trace.bootstrap.instrumentation.api.ScopeSource.MANUAL import static datadog.trace.instrumentation.opentelemetry14.context.OtelContext.DATADOG_CONTEXT_ROOT_SPAN_KEY import static datadog.trace.instrumentation.opentelemetry14.context.OtelContext.OTEL_CONTEXT_SPAN_KEY +import static datadog.trace.instrumentation.opentelemetry14.trace.OtelConventions.DEFAULT_OPERATION_NAME class ContextTest extends AgentTestRunner { @Subject @@ -188,7 +189,8 @@ class ContextTest extends AgentTestRunner { def activeSpan = TEST_TRACER.activeSpan() then: - activeSpan.operationName == "some-name" + activeSpan.operationName == DEFAULT_OPERATION_NAME + activeSpan.resourceName == "some-name" DDSpanId.toHexStringPadded(activeSpan.spanId) == otelParentSpan.getSpanContext().spanId when: @@ -205,7 +207,8 @@ class ContextTest extends AgentTestRunner { activeSpan = TEST_TRACER.activeSpan() then: - activeSpan.operationName == "another-name" + activeSpan.operationName == DEFAULT_OPERATION_NAME + activeSpan.resourceName == "another-name" DDSpanId.toHexStringPadded(activeSpan.spanId) == otelGrandChildSpan.getSpanContext().spanId when: @@ -221,7 +224,8 @@ class ContextTest extends AgentTestRunner { trace(3) { span { parent() - operationName "some-name" + operationName DEFAULT_OPERATION_NAME + resourceName "some-name" } span { childOfPrevious() @@ -229,7 +233,8 @@ class ContextTest extends AgentTestRunner { } span { childOfPrevious() - operationName "another-name" + operationName DEFAULT_OPERATION_NAME + resourceName "another-name" } } } diff --git a/dd-java-agent/instrumentation/opentelemetry/opentelemetry-1.4/src/test/groovy/opentelemetry14/context/propagation/AbstractPropagatorTest.groovy b/dd-java-agent/instrumentation/opentelemetry/opentelemetry-1.4/src/test/groovy/opentelemetry14/context/propagation/AbstractPropagatorTest.groovy index 351ea547b24..977f5f9c40a 100644 --- a/dd-java-agent/instrumentation/opentelemetry/opentelemetry-1.4/src/test/groovy/opentelemetry14/context/propagation/AbstractPropagatorTest.groovy +++ b/dd-java-agent/instrumentation/opentelemetry/opentelemetry-1.4/src/test/groovy/opentelemetry14/context/propagation/AbstractPropagatorTest.groovy @@ -13,6 +13,8 @@ import spock.lang.Subject import javax.annotation.Nullable +import static datadog.trace.instrumentation.opentelemetry14.trace.OtelConventions.DEFAULT_OPERATION_NAME + abstract class AbstractPropagatorTest extends AgentTestRunner { @Subject def tracer = GlobalOpenTelemetry.get().tracerProvider.get("propagator" + Math.random()) // TODO FIX LATER @@ -86,7 +88,8 @@ abstract class AbstractPropagatorTest extends AgentTestRunner { assertTraces(1) { trace(1) { span { - operationName "some-name" + operationName DEFAULT_OPERATION_NAME + resourceName "some-name" traceDDId(DD128bTraceId.fromHex(traceId)) parentSpanId(DDSpanId.fromHex(spanId).toLong() as BigInteger) } diff --git a/dd-smoke-tests/play-2.8-otel/app/actions/AbstractAction.java b/dd-smoke-tests/play-2.8-otel/app/actions/AbstractAction.java index 3d56e52cadd..9e27a2421f4 100644 --- a/dd-smoke-tests/play-2.8-otel/app/actions/AbstractAction.java +++ b/dd-smoke-tests/play-2.8-otel/app/actions/AbstractAction.java @@ -9,16 +9,16 @@ public abstract class AbstractAction extends Action.Simple { - private final String operationName; + private final String spanName; - protected AbstractAction(String operationName) { - this.operationName = operationName; + protected AbstractAction(String spanName) { + this.spanName = spanName; } @Override public CompletionStage call(Http.Request req) { Tracer tracer = GlobalOpenTelemetry.getTracer("play-test"); - Span span = tracer.spanBuilder(operationName).startSpan(); + Span span = tracer.spanBuilder(spanName).startSpan(); Scope scope = span.makeCurrent(); try { return delegate.call(req); diff --git a/dd-smoke-tests/play-2.8-otel/app/filters/AbstractFilter.java b/dd-smoke-tests/play-2.8-otel/app/filters/AbstractFilter.java index f9e28b8f6b8..b52223036aa 100644 --- a/dd-smoke-tests/play-2.8-otel/app/filters/AbstractFilter.java +++ b/dd-smoke-tests/play-2.8-otel/app/filters/AbstractFilter.java @@ -12,17 +12,16 @@ public abstract class AbstractFilter extends Filter { private final HttpExecutionContext ec; - private final String operationName; + private final String spanName; private final boolean wrap; - public AbstractFilter(String operationName, Materializer mat, HttpExecutionContext ec) { - this(operationName, false, mat, ec); + public AbstractFilter(String spanName, Materializer mat, HttpExecutionContext ec) { + this(spanName, false, mat, ec); } - public AbstractFilter( - String operationName, boolean wrap, Materializer mat, HttpExecutionContext ec) { + public AbstractFilter(String spanName, boolean wrap, Materializer mat, HttpExecutionContext ec) { super(mat); - this.operationName = operationName; + this.spanName = spanName; this.wrap = wrap; this.ec = ec; } @@ -32,14 +31,14 @@ public CompletionStage apply( Function> nextFilter, Http.RequestHeader requestHeader) { final Tracer tracer = GlobalOpenTelemetry.getTracer("play-test"); - final Span startedSpan = wrap ? tracer.spanBuilder(operationName).startSpan() : null; + final Span startedSpan = wrap ? tracer.spanBuilder(spanName).startSpan() : null; Scope outerScope = wrap ? startedSpan.makeCurrent() : null; try { return nextFilter .apply(requestHeader) .thenApplyAsync( result -> { - Span span = wrap ? startedSpan : tracer.spanBuilder(operationName).startSpan(); + Span span = wrap ? startedSpan : tracer.spanBuilder(spanName).startSpan(); try (Scope innerScope = span.makeCurrent()) { // Yes this does no real work return result; diff --git a/dd-smoke-tests/play-2.8-otel/src/test/groovy/datadog/smoketest/Play28OTelSmokeTest.groovy b/dd-smoke-tests/play-2.8-otel/src/test/groovy/datadog/smoketest/Play28OTelSmokeTest.groovy index b2a09396abe..7ab1e90ae44 100644 --- a/dd-smoke-tests/play-2.8-otel/src/test/groovy/datadog/smoketest/Play28OTelSmokeTest.groovy +++ b/dd-smoke-tests/play-2.8-otel/src/test/groovy/datadog/smoketest/Play28OTelSmokeTest.groovy @@ -52,8 +52,8 @@ abstract class Play28OTelSmokeTest extends AbstractServerSmokeTest { + " -Dhttp.port=${httpPort}" + " -Dhttp.address=127.0.0.1" + " -Dplay.server.provider=${serverProvider()}" - + " -Ddd.writer.type=MultiWriter:TraceStructureWriter:${output.getAbsolutePath()},DDAgentWriter" - + " -Ddd.integration.opentelemetry.experimental.enabled=true" + + " -Ddd.writer.type=MultiWriter:TraceStructureWriter:${output.getAbsolutePath()}:includeresource,DDAgentWriter" + + " -Ddd.integration.opentelemetry-1.enabled=true" + " -Dclient.request.base=${clientServer.address}/hello/") return processBuilder } @@ -77,14 +77,14 @@ abstract class Play28OTelSmokeTest extends AbstractServerSmokeTest { // that is completed is completed before the span is finished, the order of those filters and the request processing // is undefined. boolean isOk = true - def allowed = /|^\[${serverProviderName()}.request - |(\[filter1(\[filter\d])(\[filter\d])(\[filter\d])])? - |\[play.request\[action1\[action2\[do-get\[play-ws.request]]]]] - |(\[filter1(\[filter\d])(\[filter\d])(\[filter\d])])? + def allowed = /|^\[${serverProviderName()}.request:GET \/welcome[sj] + |(\[otel_unknown:filter1(\[otel_unknown:filter\d])(\[otel_unknown:filter\d])(\[otel_unknown:filter\d])])? + |\[play.request:GET \/welcome[sj]\[otel_unknown:action1\[otel_unknown:action2\[otel_unknown:do-get\[play-ws.request:GET \/hello\/\?]]]]] + |(\[otel_unknown:filter1(\[otel_unknown:filter\d])(\[otel_unknown:filter\d])(\[otel_unknown:filter\d])])? |]$/.stripMargin().replaceAll("[\n\r]", "") // Ignore [command_execution], which can be generated by hostname calls from Config/Telemetry on some systems. - traceCounts = traceCounts.findAll { it.key != "[command_execution]" } + traceCounts = traceCounts.findAll { it.key != "[command_execution:hostname]" } traceCounts.entrySet().each { def matcher = (it.key =~ allowed).findAll() @@ -94,9 +94,9 @@ abstract class Play28OTelSmokeTest extends AbstractServerSmokeTest { |traceCounts=${traceCounts}""".stripMargin() def matches = matcher.head().findAll{ it != null } isOk &= matches.size() == 5 - isOk &= matches.contains("[filter2]") - isOk &= matches.contains("[filter3]") - isOk &= matches.contains("[filter4]") + isOk &= matches.contains("[otel_unknown:filter2]") + isOk &= matches.contains("[otel_unknown:filter3]") + isOk &= matches.contains("[otel_unknown:filter4]") assert isOk : """\ |Trace ${it.key} does not match allowed pattern: |pattern=${allowed} From 8f45302d6097890452da059e019c1f6a8b31bad0 Mon Sep 17 00:00:00 2001 From: Andrea Marziali Date: Fri, 10 Nov 2023 19:07:28 +0100 Subject: [PATCH 5/6] aws-sdk-1: fix latestDepTests (#6190) --- .../aws-java-sdk-1.11.0/build.gradle | 2 + .../src/test/groovy/AWS1ClientTest.groovy | 39 +++++++++++++------ .../groovy/LegacyAWS1ClientForkedTest.groovy | 38 ++++++++++++------ .../aws-java-sqs-1.0/build.gradle | 3 +- .../aws-java-sqs-2.0/build.gradle | 4 +- .../groovy/LegacySqsClientForkedTest.groovy | 10 ++--- .../src/test/groovy/SqsClientTest.groovy | 2 +- .../test/groovy/TimeInQueueForkedTest.groovy | 6 +-- dd-java-agent/instrumentation/build.gradle | 2 +- 9 files changed, 71 insertions(+), 35 deletions(-) diff --git a/dd-java-agent/instrumentation/aws-java-sdk-1.11.0/build.gradle b/dd-java-agent/instrumentation/aws-java-sdk-1.11.0/build.gradle index 2e036a050e1..6625bc50fb0 100644 --- a/dd-java-agent/instrumentation/aws-java-sdk-1.11.0/build.gradle +++ b/dd-java-agent/instrumentation/aws-java-sdk-1.11.0/build.gradle @@ -49,6 +49,8 @@ dependencies { // needed for kinesis: testImplementation group: 'com.fasterxml.jackson.dataformat', name: 'jackson-dataformat-cbor', version: versions.jackson + testImplementation group: 'org.json', name: 'json', version: '20231013' + test_before_1_11_106Implementation(group: 'com.amazonaws', name: 'aws-java-sdk-s3') { version { diff --git a/dd-java-agent/instrumentation/aws-java-sdk-1.11.0/src/test/groovy/AWS1ClientTest.groovy b/dd-java-agent/instrumentation/aws-java-sdk-1.11.0/src/test/groovy/AWS1ClientTest.groovy index 9bf73151744..9874e1a5510 100644 --- a/dd-java-agent/instrumentation/aws-java-sdk-1.11.0/src/test/groovy/AWS1ClientTest.groovy +++ b/dd-java-agent/instrumentation/aws-java-sdk-1.11.0/src/test/groovy/AWS1ClientTest.groovy @@ -39,6 +39,7 @@ import datadog.trace.api.Config import datadog.trace.api.DDSpanTypes import datadog.trace.bootstrap.instrumentation.api.Tags import datadog.trace.test.util.Flaky +import org.json.XML import spock.lang.AutoCleanup import spock.lang.Shared @@ -61,12 +62,23 @@ abstract class AWS1ClientTest extends VersionedNamingTestBase { def credentialsProvider = new AWSStaticCredentialsProvider(new AnonymousAWSCredentials()) @Shared def responseBody = new AtomicReference() + @Shared + def jsonPointer = new AtomicReference() + @AutoCleanup @Shared def server = httpServer { handlers { all { - response.status(200).send(responseBody.get()) + def body = responseBody.get() + if (request.headers.get("Content-Type")?.contains("json")) { + def json = XML.toJSONObject(body) + if (jsonPointer.get() != null) { + json = json.query(jsonPointer.get()) + } + body = json.toString() + } + response.status(200).send(body) } } } @@ -131,6 +143,7 @@ abstract class AWS1ClientTest extends VersionedNamingTestBase { def "send #operation request with mocked response"() { setup: responseBody.set(body) + jsonPointer.set(jsonPointerStr) when: def response = call.call(client) @@ -181,19 +194,22 @@ abstract class AWS1ClientTest extends VersionedNamingTestBase { server.lastRequest.headers.get("x-datadog-trace-id") == null server.lastRequest.headers.get("x-datadog-parent-id") == null + cleanup: + jsonPointer.set(null) + where: - service | operation | method | path | client | call | additionalTags | body | peerService - "S3" | "CreateBucket" | "PUT" | "/testbucket/" | AmazonS3ClientBuilder.standard().withPathStyleAccessEnabled(true).withEndpointConfiguration(endpoint).withCredentials(credentialsProvider).build() | { c -> c.createBucket("testbucket") } | ["aws.bucket.name": "testbucket", "bucketname": "testbucket"] | "" | "aws.bucket.name" - "S3" | "GetObject" | "GET" | "/someBucket/someKey" | AmazonS3ClientBuilder.standard().withPathStyleAccessEnabled(true).withEndpointConfiguration(endpoint).withCredentials(credentialsProvider).build() | { c -> c.getObject("someBucket", "someKey") } | ["aws.bucket.name": "someBucket", "bucketname": "someBucket"] | "" | "aws.bucket.name" - "DynamoDBv2" | "CreateTable" | "POST" | "/" | AmazonDynamoDBClientBuilder.standard().withEndpointConfiguration(endpoint).withCredentials(credentialsProvider).build() | { c -> c.createTable(new CreateTableRequest("sometable", null)) } | ["aws.table.name": "sometable", "tablename": "sometable"] | "" | "aws.table.name" - "Kinesis" | "DeleteStream" | "POST" | "/" | AmazonKinesisClientBuilder.standard().withEndpointConfiguration(endpoint).withCredentials(credentialsProvider).build() | { c -> c.deleteStream(new DeleteStreamRequest().withStreamName("somestream")) } | ["aws.stream.name": "somestream", "streamname": "somestream"] | "" | "aws.stream.name" + service | operation | method | path | client | call | additionalTags | body | peerService | jsonPointerStr + "S3" | "CreateBucket" | "PUT" | "/testbucket/" | AmazonS3ClientBuilder.standard().withPathStyleAccessEnabled(true).withEndpointConfiguration(endpoint).withCredentials(credentialsProvider).build() | { c -> c.createBucket("testbucket") } | ["aws.bucket.name": "testbucket", "bucketname": "testbucket"] | "" | "aws.bucket.name" | null + "S3" | "GetObject" | "GET" | "/someBucket/someKey" | AmazonS3ClientBuilder.standard().withPathStyleAccessEnabled(true).withEndpointConfiguration(endpoint).withCredentials(credentialsProvider).build() | { c -> c.getObject("someBucket", "someKey") } | ["aws.bucket.name": "someBucket", "bucketname": "someBucket"] | "" | "aws.bucket.name" | null + "DynamoDBv2" | "CreateTable" | "POST" | "/" | AmazonDynamoDBClientBuilder.standard().withEndpointConfiguration(endpoint).withCredentials(credentialsProvider).build() | { c -> c.createTable(new CreateTableRequest("sometable", null)) } | ["aws.table.name": "sometable", "tablename": "sometable"] | "" | "aws.table.name" | null + "Kinesis" | "DeleteStream" | "POST" | "/" | AmazonKinesisClientBuilder.standard().withEndpointConfiguration(endpoint).withCredentials(credentialsProvider).build() | { c -> c.deleteStream(new DeleteStreamRequest().withStreamName("somestream")) } | ["aws.stream.name": "somestream", "streamname": "somestream"] | "" | "aws.stream.name" | null "SQS" | "CreateQueue" | "POST" | "/" | AmazonSQSClientBuilder.standard().withEndpointConfiguration(endpoint).withCredentials(credentialsProvider).build() | { c -> c.createQueue(new CreateQueueRequest("somequeue")) } | ["aws.queue.name": "somequeue", "queuename": "somequeue"] | """ https://queue.amazonaws.com/123456789012/MyQueue 7a62c49f-347e-4fc4-9331-6e8e7a96aa73 - """ | "aws.queue.name" + """ | "aws.queue.name" | "/CreateQueueResponse/CreateQueueResult" "SQS" | "SendMessage" | "POST" | "/someurl" | AmazonSQSClientBuilder.standard().withEndpointConfiguration(endpoint).withCredentials(credentialsProvider).build() | { c -> c.sendMessage(new SendMessageRequest("someurl", "")) } | ["aws.queue.url": "someurl"] | """ @@ -203,7 +219,7 @@ abstract class AWS1ClientTest extends VersionedNamingTestBase { 27daac76-34dd-47df-bd01-1f6e873584a0 - """ | "aws.queue.url" + """ | "aws.queue.url" | "/SendMessageResponse/SendMessageResult" "SNS" | "Publish" | "POST" | "/" | AmazonSNSClientBuilder.standard().withEndpointConfiguration(endpoint).withCredentials(credentialsProvider).build() | { c -> c.publish(new PublishRequest("arn:aws:sns::123:some-topic", "")) } | ["aws.topic.name": "some-topic", "topicname": "some-topic"] | """ @@ -211,22 +227,21 @@ abstract class AWS1ClientTest extends VersionedNamingTestBase { d74b8436-ae13-5ab4-a9ff-ce54dfea72a0 - """ | "aws.topic.name" + """ | "aws.topic.name" | "/PublishResponse/PublishResult" "EC2" | "AllocateAddress" | "POST" | "/" | AmazonEC2ClientBuilder.standard().withEndpointConfiguration(endpoint).withCredentials(credentialsProvider).build() | { c -> c.allocateAddress() } | [:] | """ 59dbff89-35bd-4eac-99ed-be587EXAMPLE 192.0.2.1 standard - """ | null - + """ | null | "/AllocateAddressResponse" "RDS" | "DeleteOptionGroup" | "POST" | "/" | AmazonRDSClientBuilder.standard().withEndpointConfiguration(endpoint).withCredentials(credentialsProvider).build() | { c -> c.deleteOptionGroup(new DeleteOptionGroupRequest()) } | [:] | """ 0ac9cda2-bbf4-11d3-f92b-31fa5e8dbc99 - """ | null + """ | null | null } def "send #operation request to closed port"() { diff --git a/dd-java-agent/instrumentation/aws-java-sdk-1.11.0/src/test/groovy/LegacyAWS1ClientForkedTest.groovy b/dd-java-agent/instrumentation/aws-java-sdk-1.11.0/src/test/groovy/LegacyAWS1ClientForkedTest.groovy index e59540b4ab5..a03133a45c8 100644 --- a/dd-java-agent/instrumentation/aws-java-sdk-1.11.0/src/test/groovy/LegacyAWS1ClientForkedTest.groovy +++ b/dd-java-agent/instrumentation/aws-java-sdk-1.11.0/src/test/groovy/LegacyAWS1ClientForkedTest.groovy @@ -36,6 +36,7 @@ import datadog.trace.bootstrap.instrumentation.api.Tags import datadog.trace.test.util.Flaky import org.apache.http.conn.HttpHostConnectException import org.apache.http.impl.execchain.RequestAbortedException +import org.json.XML import spock.lang.AutoCleanup import spock.lang.Shared @@ -68,12 +69,23 @@ class LegacyAWS1ClientForkedTest extends AgentTestRunner { def credentialsProvider = new AWSStaticCredentialsProvider(new AnonymousAWSCredentials()) @Shared def responseBody = new AtomicReference() + @Shared + def jsonPointer = new AtomicReference() + @AutoCleanup @Shared def server = httpServer { handlers { all { - response.status(200).send(responseBody.get()) + def body = responseBody.get() + if (request.headers.get("Content-Type")?.contains("json")) { + def json = XML.toJSONObject(body) + if (jsonPointer.get() != null) { + json = json.query(jsonPointer.get()) + } + body = json.toString() + } + response.status(200).send(body) } } } @@ -124,6 +136,7 @@ class LegacyAWS1ClientForkedTest extends AgentTestRunner { def "send #operation request with mocked response"() { setup: responseBody.set(body) + jsonPointer.set(jsonPointerStr) when: def response = call.call(client) @@ -187,18 +200,21 @@ class LegacyAWS1ClientForkedTest extends AgentTestRunner { server.lastRequest.headers.get("x-datadog-trace-id") == null server.lastRequest.headers.get("x-datadog-parent-id") == null + cleanup: + jsonPointer.set(null) + where: - service | operation | ddService | method | path | client | call | additionalTags | body - "S3" | "CreateBucket" | "java-aws-sdk" | "PUT" | "/testbucket/" | AmazonS3ClientBuilder.standard().withPathStyleAccessEnabled(true).withEndpointConfiguration(endpoint).withCredentials(credentialsProvider).build() | { c -> c.createBucket("testbucket") } | ["aws.bucket.name": "testbucket", "bucketname": "testbucket"] | "" - "S3" | "GetObject" | "java-aws-sdk" | "GET" | "/someBucket/someKey" | AmazonS3ClientBuilder.standard().withPathStyleAccessEnabled(true).withEndpointConfiguration(endpoint).withCredentials(credentialsProvider).build() | { c -> c.getObject("someBucket", "someKey") } | ["aws.bucket.name": "someBucket", "bucketname": "someBucket"] | "" - "DynamoDBv2" | "CreateTable" | "java-aws-sdk" | "POST" | "/" | AmazonDynamoDBClientBuilder.standard().withEndpointConfiguration(endpoint).withCredentials(credentialsProvider).build() | { c -> c.createTable(new CreateTableRequest("sometable", null)) } | ["aws.table.name": "sometable", "tablename": "sometable"] | "" - "Kinesis" | "DeleteStream" | "java-aws-sdk" | "POST" | "/" | AmazonKinesisClientBuilder.standard().withEndpointConfiguration(endpoint).withCredentials(credentialsProvider).build() | { c -> c.deleteStream(new DeleteStreamRequest().withStreamName("somestream")) } | ["aws.stream.name": "somestream", "streamname": "somestream"] | "" + service | operation | ddService | method | path | client | call | additionalTags | body | jsonPointerStr + "S3" | "CreateBucket" | "java-aws-sdk" | "PUT" | "/testbucket/" | AmazonS3ClientBuilder.standard().withPathStyleAccessEnabled(true).withEndpointConfiguration(endpoint).withCredentials(credentialsProvider).build() | { c -> c.createBucket("testbucket") } | ["aws.bucket.name": "testbucket", "bucketname": "testbucket"] | "" | null + "S3" | "GetObject" | "java-aws-sdk" | "GET" | "/someBucket/someKey" | AmazonS3ClientBuilder.standard().withPathStyleAccessEnabled(true).withEndpointConfiguration(endpoint).withCredentials(credentialsProvider).build() | { c -> c.getObject("someBucket", "someKey") } | ["aws.bucket.name": "someBucket", "bucketname": "someBucket"] | "" | null + "DynamoDBv2" | "CreateTable" | "java-aws-sdk" | "POST" | "/" | AmazonDynamoDBClientBuilder.standard().withEndpointConfiguration(endpoint).withCredentials(credentialsProvider).build() | { c -> c.createTable(new CreateTableRequest("sometable", null)) } | ["aws.table.name": "sometable", "tablename": "sometable"] | "" | null + "Kinesis" | "DeleteStream" | "java-aws-sdk" | "POST" | "/" | AmazonKinesisClientBuilder.standard().withEndpointConfiguration(endpoint).withCredentials(credentialsProvider).build() | { c -> c.deleteStream(new DeleteStreamRequest().withStreamName("somestream")) } | ["aws.stream.name": "somestream", "streamname": "somestream"] | "" | null "SQS" | "CreateQueue" | "java-aws-sdk" | "POST" | "/" | AmazonSQSClientBuilder.standard().withEndpointConfiguration(endpoint).withCredentials(credentialsProvider).build() | { c -> c.createQueue(new CreateQueueRequest("somequeue")) } | ["aws.queue.name": "somequeue", "queuename": "somequeue"] | """ https://queue.amazonaws.com/123456789012/MyQueue 7a62c49f-347e-4fc4-9331-6e8e7a96aa73 - """ + """ | "/CreateQueueResponse/CreateQueueResult" "SQS" | "SendMessage" | "sqs" | "POST" | "/someurl" | AmazonSQSClientBuilder.standard().withEndpointConfiguration(endpoint).withCredentials(credentialsProvider).build() | { c -> c.sendMessage(new SendMessageRequest("someurl", "")) } | ["aws.queue.url": "someurl"] | """ @@ -208,7 +224,7 @@ class LegacyAWS1ClientForkedTest extends AgentTestRunner { 27daac76-34dd-47df-bd01-1f6e873584a0 - """ + """ | "/SendMessageResponse/SendMessageResult" "SNS" | "Publish" | "sns" | "POST" | "/" | AmazonSNSClientBuilder.standard().withEndpointConfiguration(endpoint).withCredentials(credentialsProvider).build() | { c -> c.publish(new PublishRequest("arn:aws:sns::123:some-topic", "")) } | ["aws.topic.name": "some-topic", "topicname": "some-topic"] | """ @@ -216,21 +232,21 @@ class LegacyAWS1ClientForkedTest extends AgentTestRunner { d74b8436-ae13-5ab4-a9ff-ce54dfea72a0 - """ + """ | "/PublishResponse/PublishResult" "EC2" | "AllocateAddress" | "java-aws-sdk" | "POST" | "/" | AmazonEC2ClientBuilder.standard().withEndpointConfiguration(endpoint).withCredentials(credentialsProvider).build() | { c -> c.allocateAddress() } | [:] | """ 59dbff89-35bd-4eac-99ed-be587EXAMPLE 192.0.2.1 standard - """ + """ | null "RDS" | "DeleteOptionGroup" | "java-aws-sdk" | "POST" | "/" | AmazonRDSClientBuilder.standard().withEndpointConfiguration(endpoint).withCredentials(credentialsProvider).build() | { c -> c.deleteOptionGroup(new DeleteOptionGroupRequest()) } | [:] | """ 0ac9cda2-bbf4-11d3-f92b-31fa5e8dbc99 - """ + """ | null } def "send #operation request to closed port"() { diff --git a/dd-java-agent/instrumentation/aws-java-sqs-1.0/build.gradle b/dd-java-agent/instrumentation/aws-java-sqs-1.0/build.gradle index 3f9a4eca2cf..e979e34e7fd 100644 --- a/dd-java-agent/instrumentation/aws-java-sqs-1.0/build.gradle +++ b/dd-java-agent/instrumentation/aws-java-sqs-1.0/build.gradle @@ -12,6 +12,7 @@ muzzle { apply from: "$rootDir/gradle/java.gradle" addTestSuiteForDir('latestDepTest', 'test') +addTestSuiteExtendingForDir('latestDepForkedTest', 'latestDepTest', 'test') dependencies { compileOnly group: 'com.amazonaws', name: 'aws-java-sdk-sqs', version: '1.11.0' @@ -23,7 +24,7 @@ dependencies { testImplementation project(':dd-java-agent:instrumentation:jms') // SQS<->JMS testing: - testImplementation group: 'org.elasticmq', name: 'elasticmq-rest-sqs_2.13', version: '1.2.3' + testImplementation group: 'org.elasticmq', name: 'elasticmq-rest-sqs_2.13', version: '1.4.7' testImplementation group: 'com.amazonaws', name: 'amazon-sqs-java-messaging-lib', version: '1.0.8' latestDepTestImplementation group: 'com.amazonaws', name: 'aws-java-sdk-sqs', version: '+' diff --git a/dd-java-agent/instrumentation/aws-java-sqs-2.0/build.gradle b/dd-java-agent/instrumentation/aws-java-sqs-2.0/build.gradle index a292ea34a70..adff4418500 100644 --- a/dd-java-agent/instrumentation/aws-java-sqs-2.0/build.gradle +++ b/dd-java-agent/instrumentation/aws-java-sqs-2.0/build.gradle @@ -13,6 +13,8 @@ muzzle { apply from: "$rootDir/gradle/java.gradle" addTestSuiteForDir('latestDepTest', 'test') +addTestSuiteExtendingForDir('latestDepForkedTest', 'latestDepTest', 'test') + dependencies { compileOnly group: 'software.amazon.awssdk', name: 'sqs', version: '2.2.0' @@ -24,7 +26,7 @@ dependencies { testImplementation project(':dd-java-agent:instrumentation:jms') // SQS<->JMS testing: - testImplementation group: 'org.elasticmq', name: 'elasticmq-rest-sqs_2.13', version: '1.2.3' + testImplementation group: 'org.elasticmq', name: 'elasticmq-rest-sqs_2.13', version: '1.4.7' testImplementation group: 'com.amazonaws', name: 'amazon-sqs-java-messaging-lib', version: '2.0.0' latestDepTestImplementation group: 'software.amazon.awssdk', name: 'sqs', version: '2.20.33' diff --git a/dd-java-agent/instrumentation/aws-java-sqs-2.0/src/test/groovy/LegacySqsClientForkedTest.groovy b/dd-java-agent/instrumentation/aws-java-sqs-2.0/src/test/groovy/LegacySqsClientForkedTest.groovy index d2692a61d0c..84ad8f22268 100644 --- a/dd-java-agent/instrumentation/aws-java-sqs-2.0/src/test/groovy/LegacySqsClientForkedTest.groovy +++ b/dd-java-agent/instrumentation/aws-java-sqs-2.0/src/test/groovy/LegacySqsClientForkedTest.groovy @@ -94,7 +94,7 @@ class LegacySqsClientForkedTest extends AgentTestRunner { "aws.operation" "SendMessage" "aws.agent" "java-aws-sdk" "aws.queue.url" "http://localhost:${address.port}/000000000000/somequeue" - "aws.requestId" "00000000-0000-0000-0000-000000000000" + "aws.requestId" { it.trim() == "00000000-0000-0000-0000-000000000000" } // the test server seem messing with request id and insert \n defaultTags() } } @@ -140,7 +140,7 @@ class LegacySqsClientForkedTest extends AgentTestRunner { "aws.operation" "ReceiveMessage" "aws.agent" "java-aws-sdk" "aws.queue.url" "http://localhost:${address.port}/000000000000/somequeue" - "aws.requestId" "00000000-0000-0000-0000-000000000000" + "aws.requestId" { it.trim() == "00000000-0000-0000-0000-000000000000" } // the test server seem messing with request id and insert \n defaultTags() } } @@ -222,7 +222,7 @@ class LegacySqsClientForkedTest extends AgentTestRunner { "aws.operation" "SendMessage" "aws.agent" "java-aws-sdk" "aws.queue.url" "http://localhost:${address.port}/000000000000/somequeue" - "aws.requestId" "00000000-0000-0000-0000-000000000000" + "aws.requestId" { it.trim() == "00000000-0000-0000-0000-000000000000" } // the test server seem messing with request id and insert \n defaultTags() } } @@ -285,7 +285,7 @@ class LegacySqsClientForkedTest extends AgentTestRunner { "aws.operation" "DeleteMessage" "aws.agent" "java-aws-sdk" "aws.queue.url" "http://localhost:${address.port}/000000000000/somequeue" - "aws.requestId" "00000000-0000-0000-0000-000000000000" + "aws.requestId" { it.trim() == "00000000-0000-0000-0000-000000000000" } // the test server seem messing with request id and insert \n defaultTags() } } @@ -330,7 +330,7 @@ class LegacySqsClientForkedTest extends AgentTestRunner { "aws.operation" "ReceiveMessage" "aws.agent" "java-aws-sdk" "aws.queue.url" "http://localhost:${address.port}/000000000000/somequeue" - "aws.requestId" "00000000-0000-0000-0000-000000000000" + "aws.requestId" { it.trim() == "00000000-0000-0000-0000-000000000000" } // the test server seem messing with request id and insert \n defaultTags() } } diff --git a/dd-java-agent/instrumentation/aws-java-sqs-2.0/src/test/groovy/SqsClientTest.groovy b/dd-java-agent/instrumentation/aws-java-sqs-2.0/src/test/groovy/SqsClientTest.groovy index 05b5b5a0971..ad1219478f5 100644 --- a/dd-java-agent/instrumentation/aws-java-sqs-2.0/src/test/groovy/SqsClientTest.groovy +++ b/dd-java-agent/instrumentation/aws-java-sqs-2.0/src/test/groovy/SqsClientTest.groovy @@ -394,7 +394,7 @@ abstract class SqsClientTest extends VersionedNamingTestBase { "aws.operation" "DeleteMessage" "aws.agent" "java-aws-sdk" "aws.queue.url" "http://localhost:${address.port}/000000000000/somequeue" - "aws.requestId" "00000000-0000-0000-0000-000000000000" + "aws.requestId" { it.trim() == "00000000-0000-0000-0000-000000000000" } // the test server seem messing with request id and insert \n defaultTags() } } diff --git a/dd-java-agent/instrumentation/aws-java-sqs-2.0/src/test/groovy/TimeInQueueForkedTest.groovy b/dd-java-agent/instrumentation/aws-java-sqs-2.0/src/test/groovy/TimeInQueueForkedTest.groovy index 7c864453979..b19b99c0c82 100644 --- a/dd-java-agent/instrumentation/aws-java-sqs-2.0/src/test/groovy/TimeInQueueForkedTest.groovy +++ b/dd-java-agent/instrumentation/aws-java-sqs-2.0/src/test/groovy/TimeInQueueForkedTest.groovy @@ -302,7 +302,7 @@ class TimeInQueueForkedTest extends AgentTestRunner { "aws.operation" "SendMessageBatch" "aws.agent" "java-aws-sdk" "aws.queue.url" "http://localhost:${address.port}/000000000000/somequeue" - "aws.requestId" "00000000-0000-0000-0000-000000000000" + "aws.requestId" { it.trim() == "00000000-0000-0000-0000-000000000000" } // the test server seem messing with request id and insert \n defaultTags() } } @@ -325,7 +325,7 @@ class TimeInQueueForkedTest extends AgentTestRunner { "aws.operation" "ReceiveMessage" "aws.agent" "java-aws-sdk" "aws.queue.url" "http://localhost:${address.port}/000000000000/somequeue" - "aws.requestId" "00000000-0000-0000-0000-000000000000" + "aws.requestId" { it.trim() == "00000000-0000-0000-0000-000000000000" } // the test server seem messing with request id and insert \n defaultTags(parent.resourceName as String == "Sqs.SendMessageBatch") } } @@ -344,7 +344,7 @@ class TimeInQueueForkedTest extends AgentTestRunner { "$Tags.COMPONENT" "java-aws-sdk" "$Tags.SPAN_KIND" Tags.SPAN_KIND_BROKER "aws.queue.url" "http://localhost:${address.port}/000000000000/somequeue" - "aws.requestId" "00000000-0000-0000-0000-000000000000" + "aws.requestId" { it.trim() == "00000000-0000-0000-0000-000000000000" } // the test server seem messing with request id and insert \n defaultTags(true) } } diff --git a/dd-java-agent/instrumentation/build.gradle b/dd-java-agent/instrumentation/build.gradle index 3847c8b9dd0..1d22aba7ebe 100644 --- a/dd-java-agent/instrumentation/build.gradle +++ b/dd-java-agent/instrumentation/build.gradle @@ -99,7 +99,7 @@ subprojects { Project subProj -> onlyIf { !project.rootProject.hasProperty("skipInstTests") } - if (subTask.name == 'latestDepTest') { + if (subTask.name in ['latestDepTest', 'latestDepForkedTest']) { subTask.jvmArgs '-Dtest.dd.latestDepTest=true' } } From a36099fef07eec1d851e80d6a9a010785a79d3f0 Mon Sep 17 00:00:00 2001 From: Nayeem Kamal Date: Fri, 10 Nov 2023 13:36:00 -0500 Subject: [PATCH 6/6] Couchbase query normalization (#6116) * added normalization * simplified normalization check and added tests * added a DDCache for previously normalized queries * cleaned test * addressed changes from PR * refactor conditional logic * fix deleted file --- .../client/CouchbaseClientDecorator.java | 16 +++++++++ .../client/DatadogRequestSpan.java | 6 +++- .../test/groovy/CouchbaseClient31Test.groovy | 27 ++++++++------ .../client/CouchbaseClientDecorator.java | 16 +++++++++ .../client/DatadogRequestSpan.java | 6 +++- .../test/groovy/CouchbaseClient32Test.groovy | 36 ++++++++++--------- 6 files changed, 78 insertions(+), 29 deletions(-) diff --git a/dd-java-agent/instrumentation/couchbase/couchbase-3.1/src/main/java/datadog/trace/instrumentation/couchbase_31/client/CouchbaseClientDecorator.java b/dd-java-agent/instrumentation/couchbase/couchbase-3.1/src/main/java/datadog/trace/instrumentation/couchbase_31/client/CouchbaseClientDecorator.java index a1e62890976..c50eceebd82 100644 --- a/dd-java-agent/instrumentation/couchbase/couchbase-3.1/src/main/java/datadog/trace/instrumentation/couchbase_31/client/CouchbaseClientDecorator.java +++ b/dd-java-agent/instrumentation/couchbase/couchbase-3.1/src/main/java/datadog/trace/instrumentation/couchbase_31/client/CouchbaseClientDecorator.java @@ -2,10 +2,15 @@ import static datadog.trace.bootstrap.instrumentation.api.Tags.DB_TYPE; +import datadog.trace.api.cache.DDCache; +import datadog.trace.api.cache.DDCaches; import datadog.trace.api.naming.SpanNaming; +import datadog.trace.api.normalize.SQLNormalizer; import datadog.trace.bootstrap.instrumentation.api.InternalSpanTypes; import datadog.trace.bootstrap.instrumentation.api.UTF8BytesString; import datadog.trace.bootstrap.instrumentation.decorator.DBTypeProcessingDatabaseClientDecorator; +import java.util.function.Function; +import java.util.function.ToIntFunction; class CouchbaseClientDecorator extends DBTypeProcessingDatabaseClientDecorator { private static final String DB_TYPE = "couchbase"; @@ -16,6 +21,13 @@ class CouchbaseClientDecorator extends DBTypeProcessingDatabaseClientDecorator { public static final CharSequence COUCHBASE_CLIENT = UTF8BytesString.create("couchbase-client"); public static final CouchbaseClientDecorator DECORATE = new CouchbaseClientDecorator(); + private static final Function NORMALIZE = SQLNormalizer::normalize; + private static final int COMBINED_STATEMENT_LIMIT = 2 * 1024 * 1024; // characters + + private static final ToIntFunction STATEMENT_WEIGHER = UTF8BytesString::length; + private static final DDCache CACHED_STATEMENTS = + DDCaches.newFixedSizeWeightedCache(512, STATEMENT_WEIGHER, COMBINED_STATEMENT_LIMIT); + @Override protected String[] instrumentationNames() { return new String[] {"couchbase"}; @@ -55,4 +67,8 @@ protected String dbInstance(final Object o) { protected String dbHostname(Object o) { return null; } + + protected static UTF8BytesString normalizedQuery(String sql) { + return CACHED_STATEMENTS.computeIfAbsent(sql, NORMALIZE); + } } diff --git a/dd-java-agent/instrumentation/couchbase/couchbase-3.1/src/main/java/datadog/trace/instrumentation/couchbase_31/client/DatadogRequestSpan.java b/dd-java-agent/instrumentation/couchbase/couchbase-3.1/src/main/java/datadog/trace/instrumentation/couchbase_31/client/DatadogRequestSpan.java index bf36d0b4ee5..0e4af4d1373 100644 --- a/dd-java-agent/instrumentation/couchbase/couchbase-3.1/src/main/java/datadog/trace/instrumentation/couchbase_31/client/DatadogRequestSpan.java +++ b/dd-java-agent/instrumentation/couchbase/couchbase-3.1/src/main/java/datadog/trace/instrumentation/couchbase_31/client/DatadogRequestSpan.java @@ -64,7 +64,11 @@ public void setAttribute(String key, String value) { // TODO when `db.statement` is set here it will be intercepted by the TagInterceptor, so any // sort of obfuscation should go in there, preferably as a lazy sort of Utf8String that does // the actual work at the end - span.setTag(key, value); + if ("db.statement".equals(key)) { + span.setTag(key, CouchbaseClientDecorator.normalizedQuery(value)); + } else { + span.setTag(key, value); + } } // This method shows up in later versions diff --git a/dd-java-agent/instrumentation/couchbase/couchbase-3.1/src/test/groovy/CouchbaseClient31Test.groovy b/dd-java-agent/instrumentation/couchbase/couchbase-3.1/src/test/groovy/CouchbaseClient31Test.groovy index afdc418332a..e4a39e73dbc 100644 --- a/dd-java-agent/instrumentation/couchbase/couchbase-3.1/src/test/groovy/CouchbaseClient31Test.groovy +++ b/dd-java-agent/instrumentation/couchbase/couchbase-3.1/src/test/groovy/CouchbaseClient31Test.groovy @@ -139,7 +139,7 @@ abstract class CouchbaseClient31Test extends VersionedNamingTestBase { assertCouchbaseCall(it, "cb.query", [ 'db.couchbase.retries' : { Long }, 'db.couchbase.service' : 'query', - ], 'select * from `test-bucket` limit 1') + ], 'select * from `test-bucket` limit ?') assertCouchbaseDispatchCall(it, span(0)) } } @@ -148,6 +148,7 @@ abstract class CouchbaseClient31Test extends VersionedNamingTestBase { def "check query spans with parent"() { setup: def query = 'select * from `test-bucket` limit 1' + def normalizedQuery = 'select * from `test-bucket` limit ?' when: runUnderTrace('query.parent') { @@ -163,7 +164,7 @@ abstract class CouchbaseClient31Test extends VersionedNamingTestBase { assertCouchbaseCall(it, "cb.query", [ 'db.couchbase.retries' : { Long }, 'db.couchbase.service' : 'query', - ], query, span(0), false) + ], normalizedQuery, span(0), false) assertCouchbaseDispatchCall(it, span(1)) } } @@ -171,6 +172,7 @@ abstract class CouchbaseClient31Test extends VersionedNamingTestBase { def "check query spans with parent and adhoc #adhoc"() { def query = 'select count(1) from `test-bucket` where (`something` = "else") limit 1' + def normalizedQuery = 'select count(?) from `test-bucket` where (`something` = "else") limit ?' int count = 0 when: @@ -192,12 +194,12 @@ abstract class CouchbaseClient31Test extends VersionedNamingTestBase { assertCouchbaseCall(it, "cb.query", [ 'db.couchbase.retries' : { Long }, 'db.couchbase.service' : 'query', - ], query, span(0), false) + ], normalizedQuery, span(0), false) if (!adhoc) { assertCouchbaseCall(it, "prepare", [ 'db.couchbase.retries' : { Long }, 'db.couchbase.service' : 'query', - ], "PREPARE $query", span(1), true) + ], "PREPARE $normalizedQuery", span(1), true) } assertCouchbaseDispatchCall(it, span(adhoc ? 1 : 2)) } @@ -209,6 +211,7 @@ abstract class CouchbaseClient31Test extends VersionedNamingTestBase { def "check multiple query spans with parent and adhoc false"() { def query = 'select count(1) from `test-bucket` where (`something` = "wonderful") limit 1' + def normalizedQuery = 'select count(?) from `test-bucket` where (`something` = "wonderful") limit ?' int count1 = 0 int count2 = 0 @@ -237,20 +240,20 @@ abstract class CouchbaseClient31Test extends VersionedNamingTestBase { assertCouchbaseCall(it, "cb.query", [ 'db.couchbase.retries' : { Long }, 'db.couchbase.service' : 'query', - ], query, span(0), false) + ], normalizedQuery, span(0), false) assertCouchbaseCall(it, "prepare", [ 'db.couchbase.retries' : { Long }, 'db.couchbase.service' : 'query', - ], "PREPARE $query", span(1), true) + ], "PREPARE $normalizedQuery", span(1), true) assertCouchbaseDispatchCall(it, span(2)) assertCouchbaseCall(it, "cb.query", [ 'db.couchbase.retries' : { Long }, 'db.couchbase.service' : 'query', - ], query, span(0), false) + ], normalizedQuery, span(0), false) assertCouchbaseCall(it, "execute", [ 'db.couchbase.retries' : { Long }, 'db.couchbase.service' : 'query', - ], query, span(4), true) + ], normalizedQuery, span(4), true) assertCouchbaseDispatchCall(it, span(5)) } } @@ -259,6 +262,7 @@ abstract class CouchbaseClient31Test extends VersionedNamingTestBase { def "check error query spans with parent"() { setup: def query = 'select * from `test-bucket` limeit 1' + def normalizedQuery = 'select * from `test-bucket` limeit ?' Throwable ex = null when: @@ -280,7 +284,7 @@ abstract class CouchbaseClient31Test extends VersionedNamingTestBase { assertCouchbaseCall(it, "cb.query", [ 'db.couchbase.retries' : { Long }, 'db.couchbase.service' : 'query', - ], query, span(0), false, ex) + ], normalizedQuery, span(0), false, ex) assertCouchbaseDispatchCall(it, span(1)) } } @@ -288,6 +292,7 @@ abstract class CouchbaseClient31Test extends VersionedNamingTestBase { def "check multiple error query spans with parent and adhoc false"() { def query = 'select count(1) from `test-bucket` where (`something` = "wonderful") limeit 1' + def normalizedQuery = 'select count(?) from `test-bucket` where (`something` = "wonderful") limeit ?' int count1 = 0 int count2 = 0 Throwable ex1 = null @@ -332,7 +337,7 @@ abstract class CouchbaseClient31Test extends VersionedNamingTestBase { assertCouchbaseCall(it, "prepare", [ 'db.couchbase.retries' : { Long }, 'db.couchbase.service' : 'query', - ], "PREPARE $query", span(1), true, ex1) + ], "PREPARE $normalizedQuery", span(1), true, ex1) assertCouchbaseDispatchCall(it, span(2)) assertCouchbaseCall(it, "cb.query", [ 'db.couchbase.retries' : { Long }, @@ -341,7 +346,7 @@ abstract class CouchbaseClient31Test extends VersionedNamingTestBase { assertCouchbaseCall(it, "prepare", [ 'db.couchbase.retries' : { Long }, 'db.couchbase.service' : 'query', - ], "PREPARE $query", span(4), true, ex2) + ], "PREPARE $normalizedQuery", span(4), true, ex2) assertCouchbaseDispatchCall(it, span(5)) } } diff --git a/dd-java-agent/instrumentation/couchbase/couchbase-3.2/src/main/java/datadog/trace/instrumentation/couchbase_32/client/CouchbaseClientDecorator.java b/dd-java-agent/instrumentation/couchbase/couchbase-3.2/src/main/java/datadog/trace/instrumentation/couchbase_32/client/CouchbaseClientDecorator.java index 2b84ad23c07..cad336cc2de 100644 --- a/dd-java-agent/instrumentation/couchbase/couchbase-3.2/src/main/java/datadog/trace/instrumentation/couchbase_32/client/CouchbaseClientDecorator.java +++ b/dd-java-agent/instrumentation/couchbase/couchbase-3.2/src/main/java/datadog/trace/instrumentation/couchbase_32/client/CouchbaseClientDecorator.java @@ -2,10 +2,15 @@ import static datadog.trace.bootstrap.instrumentation.api.Tags.DB_TYPE; +import datadog.trace.api.cache.DDCache; +import datadog.trace.api.cache.DDCaches; import datadog.trace.api.naming.SpanNaming; +import datadog.trace.api.normalize.SQLNormalizer; import datadog.trace.bootstrap.instrumentation.api.InternalSpanTypes; import datadog.trace.bootstrap.instrumentation.api.UTF8BytesString; import datadog.trace.bootstrap.instrumentation.decorator.DBTypeProcessingDatabaseClientDecorator; +import java.util.function.Function; +import java.util.function.ToIntFunction; class CouchbaseClientDecorator extends DBTypeProcessingDatabaseClientDecorator { private static final String DB_TYPE = "couchbase"; @@ -16,6 +21,13 @@ class CouchbaseClientDecorator extends DBTypeProcessingDatabaseClientDecorator { public static final CharSequence COUCHBASE_CLIENT = UTF8BytesString.create("couchbase-client"); public static final CouchbaseClientDecorator DECORATE = new CouchbaseClientDecorator(); + private static final Function NORMALIZE = SQLNormalizer::normalize; + private static final int COMBINED_STATEMENT_LIMIT = 2 * 1024 * 1024; // characters + + private static final ToIntFunction STATEMENT_WEIGHER = UTF8BytesString::length; + private static final DDCache CACHED_STATEMENTS = + DDCaches.newFixedSizeWeightedCache(512, STATEMENT_WEIGHER, COMBINED_STATEMENT_LIMIT); + @Override protected String[] instrumentationNames() { return new String[] {"couchbase"}; @@ -55,4 +67,8 @@ protected String dbInstance(final Object o) { protected String dbHostname(Object o) { return null; } + + protected static UTF8BytesString normalizedQuery(String sql) { + return CACHED_STATEMENTS.computeIfAbsent(sql, NORMALIZE); + } } diff --git a/dd-java-agent/instrumentation/couchbase/couchbase-3.2/src/main/java/datadog/trace/instrumentation/couchbase_32/client/DatadogRequestSpan.java b/dd-java-agent/instrumentation/couchbase/couchbase-3.2/src/main/java/datadog/trace/instrumentation/couchbase_32/client/DatadogRequestSpan.java index ac3b0e20b40..11d452bf45d 100644 --- a/dd-java-agent/instrumentation/couchbase/couchbase-3.2/src/main/java/datadog/trace/instrumentation/couchbase_32/client/DatadogRequestSpan.java +++ b/dd-java-agent/instrumentation/couchbase/couchbase-3.2/src/main/java/datadog/trace/instrumentation/couchbase_32/client/DatadogRequestSpan.java @@ -77,7 +77,11 @@ public void attribute(String key, String value) { // TODO when `db.statement` is set here it will be intercepted by the TagInterceptor, so any // sort of obfuscation should go in there, preferably as a lazy sort of Utf8String that does // the actual work at the end - span.setTag(key, value); + if ("db.statement".equals(key)) { + span.setTag(key, CouchbaseClientDecorator.normalizedQuery(value)); + } else { + span.setTag(key, value); + } } @Override diff --git a/dd-java-agent/instrumentation/couchbase/couchbase-3.2/src/test/groovy/CouchbaseClient32Test.groovy b/dd-java-agent/instrumentation/couchbase/couchbase-3.2/src/test/groovy/CouchbaseClient32Test.groovy index 7fb5feba1fd..d692cbe2077 100644 --- a/dd-java-agent/instrumentation/couchbase/couchbase-3.2/src/test/groovy/CouchbaseClient32Test.groovy +++ b/dd-java-agent/instrumentation/couchbase/couchbase-3.2/src/test/groovy/CouchbaseClient32Test.groovy @@ -149,7 +149,7 @@ abstract class CouchbaseClient32Test extends VersionedNamingTestBase { assertTraces(1) { sortSpansByStart() trace(2) { - assertCouchbaseCall(it, 'select * from `test-bucket` limit 1', [ + assertCouchbaseCall(it, 'select * from `test-bucket` limit ?', [ 'db.couchbase.retries' : { Long }, 'db.couchbase.service' : 'query' ]) @@ -161,7 +161,7 @@ abstract class CouchbaseClient32Test extends VersionedNamingTestBase { def "check query spans with parent"() { setup: def query = 'select * from `test-bucket` limit 1' - + def normalizedQuery = 'select * from `test-bucket` limit ?' when: runUnderTrace('query.parent') { cluster.query(query) @@ -172,7 +172,7 @@ abstract class CouchbaseClient32Test extends VersionedNamingTestBase { sortSpansByStart() trace(3) { basicSpan(it, 'query.parent') - assertCouchbaseCall(it, query, [ + assertCouchbaseCall(it, normalizedQuery, [ 'db.couchbase.retries' : { Long }, 'db.couchbase.service' : 'query' ], span(0)) @@ -184,6 +184,7 @@ abstract class CouchbaseClient32Test extends VersionedNamingTestBase { def "check async query spans with parent and adhoc #adhoc"() { setup: def query = 'select count(1) from `test-bucket` where (`something` = "else") limit 1' + def normalizedQuery = 'select count(?) from `test-bucket` where (`something` = "else") limit ?' int count = 0 when: @@ -202,12 +203,12 @@ abstract class CouchbaseClient32Test extends VersionedNamingTestBase { sortSpansByStart() trace(adhoc ? 3 : 4) { basicSpan(it, 'async.parent') - assertCouchbaseCall(it, query, [ + assertCouchbaseCall(it, normalizedQuery, [ 'db.couchbase.retries' : { Long }, 'db.couchbase.service' : 'query' ], span(0)) if (!adhoc) { - assertCouchbaseCall(it, "PREPARE $query", [ + assertCouchbaseCall(it, "PREPARE $normalizedQuery", [ 'db.couchbase.retries': { Long }, 'db.couchbase.service': 'query' ], span(1), true) @@ -223,6 +224,7 @@ abstract class CouchbaseClient32Test extends VersionedNamingTestBase { def "check multiple async query spans with parent and adhoc false"() { setup: def query = 'select count(1) from `test-bucket` where (`something` = "wonderful") limit 1' + def normalizedQuery = 'select count(?) from `test-bucket` where (`something` = "wonderful") limit ?' int count1 = 0 int count2 = 0 def extraPrepare = isLatestDepTest @@ -249,26 +251,26 @@ abstract class CouchbaseClient32Test extends VersionedNamingTestBase { sortSpansByStart() trace(extraPrepare ? 8 : 7) { basicSpan(it, 'async.multiple') - assertCouchbaseCall(it, query, [ + assertCouchbaseCall(it, normalizedQuery, [ 'db.couchbase.retries' : { Long }, 'db.couchbase.service' : 'query' ], span(0)) - assertCouchbaseCall(it, "PREPARE $query", [ + assertCouchbaseCall(it, "PREPARE $normalizedQuery", [ 'db.couchbase.retries': { Long }, 'db.couchbase.service': 'query' ], span(1), true) assertCouchbaseDispatchCall(it, span(2)) - assertCouchbaseCall(it, query, [ + assertCouchbaseCall(it, normalizedQuery, [ 'db.couchbase.retries' : { Long }, 'db.couchbase.service' : 'query' ], span(0)) if (extraPrepare) { - assertCouchbaseCall(it, "PREPARE $query", [ + assertCouchbaseCall(it, "PREPARE $normalizedQuery", [ 'db.couchbase.retries': { Long }, 'db.couchbase.service': 'query' ], span(4), true) } - assertCouchbaseCall(it, query, [ + assertCouchbaseCall(it, normalizedQuery, [ 'db.couchbase.retries': { Long }, 'db.couchbase.service': 'query' ], span(4), true) @@ -280,12 +282,13 @@ abstract class CouchbaseClient32Test extends VersionedNamingTestBase { def "check error query spans with parent"() { setup: def query = 'select * from `test-bucket` limeit 1' + def normalizedQuery = "select * from `test-bucket` limeit ?" Throwable ex = null when: runUnderTrace('query.failure') { try { - cluster.query('select * from `test-bucket` limeit 1') + cluster.query(query) } catch (ParsingFailureException expected) { ex = expected } @@ -297,7 +300,7 @@ abstract class CouchbaseClient32Test extends VersionedNamingTestBase { sortSpansByStart() trace(3) { basicSpan(it, 'query.failure') - assertCouchbaseCall(it, query, [ + assertCouchbaseCall(it, normalizedQuery, [ 'db.couchbase.retries' : { Long }, 'db.couchbase.service' : 'query', 'db.system' : 'couchbase', @@ -310,6 +313,7 @@ abstract class CouchbaseClient32Test extends VersionedNamingTestBase { def "check multiple async error query spans with parent and adhoc false"() { setup: def query = 'select count(1) from `test-bucket` where (`something` = "wonderful") limeit 1' + def normalizedQuery = 'select count(?) from `test-bucket` where (`something` = "wonderful") limeit ?' int count1 = 0 int count2 = 0 Throwable ex1 = null @@ -347,20 +351,20 @@ abstract class CouchbaseClient32Test extends VersionedNamingTestBase { sortSpansByStart() trace(7) { basicSpan(it, 'async.failure') - assertCouchbaseCall(it, query, [ + assertCouchbaseCall(it, normalizedQuery, [ 'db.couchbase.retries' : { Long }, 'db.couchbase.service' : 'query' ], span(0), false, ex1) - assertCouchbaseCall(it, "PREPARE $query", [ + assertCouchbaseCall(it, "PREPARE $normalizedQuery", [ 'db.couchbase.retries': { Long }, 'db.couchbase.service': 'query' ], span(1), true, ex1) assertCouchbaseDispatchCall(it, span(2)) - assertCouchbaseCall(it, query, [ + assertCouchbaseCall(it, normalizedQuery, [ 'db.couchbase.retries' : { Long }, 'db.couchbase.service' : 'query' ], span(0), false, ex2) - assertCouchbaseCall(it, "PREPARE $query", [ + assertCouchbaseCall(it, "PREPARE $normalizedQuery", [ 'db.couchbase.retries': { Long }, 'db.couchbase.service': 'query' ], span(4), true, ex2)