From 0cad7667a52f4fec499b4b32bf090f642499bfd4 Mon Sep 17 00:00:00 2001 From: Sylvain Juge Date: Wed, 12 May 2021 17:22:04 +0200 Subject: [PATCH 01/25] first draft for JSON spec --- .../co/elastic/apm/agent/MockReporter.java | 61 +++++- .../test/resources/json-specs/span_types.json | 200 ++++++++++++++++++ .../TransactionInstrumentationTest.java | 2 +- 3 files changed, 254 insertions(+), 9 deletions(-) create mode 100644 apm-agent-core/src/test/resources/json-specs/span_types.json diff --git a/apm-agent-core/src/test/java/co/elastic/apm/agent/MockReporter.java b/apm-agent-core/src/test/java/co/elastic/apm/agent/MockReporter.java index bf8fedcaf3..e29d02f61c 100644 --- a/apm-agent-core/src/test/java/co/elastic/apm/agent/MockReporter.java +++ b/apm-agent-core/src/test/java/co/elastic/apm/agent/MockReporter.java @@ -45,6 +45,7 @@ import com.networknt.schema.ValidationMessage; import org.awaitility.core.ThrowingRunnable; import org.stagemonitor.configuration.ConfigurationRegistry; +import specs.TestJsonSpec; import java.io.IOException; import java.util.ArrayList; @@ -52,6 +53,7 @@ import java.util.Collections; import java.util.List; import java.util.Map; +import java.util.Objects; import java.util.Optional; import java.util.Set; import java.util.concurrent.CopyOnWriteArrayList; @@ -89,6 +91,8 @@ public class MockReporter implements Reporter { private boolean checkUnknownOutcomes = true; private boolean closed; + private static final JsonNode SPAN_TYPES_SPEC = TestJsonSpec.getJson("span_types.json"); + static { transactionSchema = getSchema("/schema/transactions/transaction.json"); spanSchema = getSchema("/schema/transactions/span.json"); @@ -164,21 +168,62 @@ public synchronized void report(Span span) { if (closed) { return; } - verifySpanSchema(asJson(dslJsonSerializer.toJsonString(span))); - verifyDestinationFields(span); - String type = span.getType(); - assertThat(type).isNotNull(); + try { + verifySpanSchema(asJson(dslJsonSerializer.toJsonString(span))); + verifySpanType(span); + verifyDestinationFields(span); - if (checkUnknownOutcomes) { - assertThat(span.getOutcome()) - .describedAs("span outcome should be either success or failure for type = %s", type) - .isNotEqualTo(Outcome.UNKNOWN); + String type = span.getType(); + assertThat(type).isNotNull(); + + if (checkUnknownOutcomes) { + assertThat(span.getOutcome()) + .describedAs("span outcome should be either success or failure for type = %s", type) + .isNotEqualTo(Outcome.UNKNOWN); + } + } catch (Exception e) { + // in case a validation error occurs, ensure that it's properly logged for easier debugging + e.printStackTrace(System.err); + throw e; } spans.add(span); } + private void verifySpanType(Span span) { + String type = span.getType(); + assertThat(type) + .describedAs("span type is mandatory") + .isNotNull(); + + String subtype = Objects.toString(span.getSubtype()); + + JsonNode typeJson = getMandatoryObject(SPAN_TYPES_SPEC, type, String.format("span type %s", type)); + + String typeComment = getOptionalComment(typeJson); + + JsonNode subTypesJson = getMandatoryObject(typeJson, "subtypes", String.format("subtypes for type '%s'", type)); + + JsonNode subTypeJson = getMandatoryObject(subTypesJson, subtype, String.format("span subtype '%s' for type '%s' (%s)", subtype, type, typeComment)); + } + + private JsonNode getMandatoryObject(JsonNode json, String name, String desc) { + JsonNode jsonNode = json.get(name); + assertThat(jsonNode) + .describedAs(desc) + .isNotNull(); + assertThat(jsonNode.isObject()) + .isTrue(); + return jsonNode; + } + + private String getOptionalComment(JsonNode json) { + return Optional.ofNullable(json.get("comment")) + .map(JsonNode::asText) + .orElse(""); + } + private void verifyDestinationFields(Span span) { if (!span.isExit()) { return; diff --git a/apm-agent-core/src/test/resources/json-specs/span_types.json b/apm-agent-core/src/test/resources/json-specs/span_types.json new file mode 100644 index 0000000000..b92287247b --- /dev/null +++ b/apm-agent-core/src/test/resources/json-specs/span_types.json @@ -0,0 +1,200 @@ +{ + "app": { + "subtypes": { + "subtype": { + "comment": "TODO : fix this anomaly (test)" + }, + "": { + "comment": "TODO : fix this anomaly" + }, + "inferred": { + "comment": "Sampling profiler inferred spans" + }, + "null": { + "comment": "TODO : find if relevant" + } + } + }, + "async": { + "subtypes": { + "blocking-queue": { + "comment": "TODO : find where it's being used" + } + } + }, + "custom": { + "comment": "API custom instrumentation", + "subtypes": { + "null": { + "comment": "TODO : fix this anomaly" + } + } + }, + "db": { + "comment": "database span", + "subtypes": { + "cassandra": { + "comment": "Cassandra" + }, + "db2": { + "comment": "IBM DB2" + }, + "elasticsearch": { + "comment": "Elasticsearch" + }, + "h2": { + "comment": "H2" + }, + "hibernate-search": { + "comment": "Hibernate Search (Java only)" + }, + "mariadb": { + "comment": "MariaDB" + }, + "mongodb": { + "comment": "MongoDB" + }, + "mysql": { + "comment": "MySQL" + }, + "null": { + "comment": "TODO : fix this anomaly" + }, + "oracle": { + "comment": "Oracle Database" + }, + "postgresql": { + "comment": "PostgreSQL" + }, + "redis": { + "comment": "Redis" + }, + "sqlserver": { + "comment": "Microsoft SQL Server" + } + } + }, + "ext": { + "subtypes": { + "http": { + "comment": "TODO remove this, 'external/http' should be used instead" + } + } + }, + "ext.http": { + "comment": "TODO remove this, 'external/http' should be used instead", + "subtypes": { + "": { + "comment": "TODO remove this, 'external/http' should be used instead" + } + } + }, + "external": { + "subtypes": { + "dubbo": { + "comment": "Apache Dubbo" + }, + "grpc": { + "comment": "gRPC" + }, + "http": { + "comment": "HTTP client" + } + } + }, + "foo": { + "comment": "TODO remove this (test)", + "subtypes": { + "bar": { + "comment": "TODO remove this (test)" + }, + "null": { + "comment": "TODO remove this (test)" + } + } + }, + "foo2": { + "comment": "TODO remove this (test)", + "subtypes": { + "null": { + "comment": "TODO remove this (test)" + } + } + }, + "foo3": { + "comment": "TODO remove this (test)", + "subtypes": { + "null": { + "comment": "TODO remove this (test)" + } + } + }, + "http": { + "comment": "TODO remove this", + "subtypes": { + "get": { + "comment": "TODO remove this (tests)" + } + } + }, + "messaging": { + "subtypes": { + "jms": { + "comment": "Java Messaging Service" + }, + "kafka": { + "comment": "Apache Kafka" + }, + "rabbitmq": { + "comment": "RabbitMQ" + } + } + }, + "process": { + "comment": "External process TODO find way to allow for wildcard (or not use binary name as subtype)", + "subtypes": { + "hello": { + "comment": "TODO remove this (test)" + }, + "java": { + "comment": "TODO remove this (test)" + } + } + }, + "servlet": { + "request-dispatcher": { + "comment": "Java Servlet request dispatcher" + } + }, + "template": { + "comment": "Template engines", + "subtypes": { + "FreeMarker": { + "comment": "" + }, + "GroovyMarkup": { + "comment": "" + }, + "InternalResource": { + "comment": "" + }, + "Jade": { + "comment": "" + }, + "MappingJackson2Json": { + "comment": "" + }, + "Thymeleaf": { + "comment": "" + } + } + }, + "test": { + "subtypes": { + "null": { + "comment": "TODO remove this (test)" + } + } + } +} + diff --git a/apm-agent-plugins/apm-api-plugin/src/test/java/co/elastic/apm/agent/pluginapi/TransactionInstrumentationTest.java b/apm-agent-plugins/apm-api-plugin/src/test/java/co/elastic/apm/agent/pluginapi/TransactionInstrumentationTest.java index 6d196d2b0a..ed69857552 100644 --- a/apm-agent-plugins/apm-api-plugin/src/test/java/co/elastic/apm/agent/pluginapi/TransactionInstrumentationTest.java +++ b/apm-agent-plugins/apm-api-plugin/src/test/java/co/elastic/apm/agent/pluginapi/TransactionInstrumentationTest.java @@ -165,7 +165,7 @@ void testChaining() { } @Test - public void startSpan() throws Exception { + public void startSpan() { Span span = transaction.startSpan("foo", null, null); span.setName("bar"); Span child = span.startSpan("foo2", null, null); From 0a5716937cf109b42176ca39570c877f1baf9c6c Mon Sep 17 00:00:00 2001 From: Sylvain Juge Date: Wed, 12 May 2021 17:38:21 +0200 Subject: [PATCH 02/25] fix servlet test --- .../src/test/resources/json-specs/span_types.json | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/apm-agent-core/src/test/resources/json-specs/span_types.json b/apm-agent-core/src/test/resources/json-specs/span_types.json index b92287247b..2dc40ddccf 100644 --- a/apm-agent-core/src/test/resources/json-specs/span_types.json +++ b/apm-agent-core/src/test/resources/json-specs/span_types.json @@ -162,8 +162,10 @@ } }, "servlet": { - "request-dispatcher": { - "comment": "Java Servlet request dispatcher" + "subtypes": { + "request-dispatcher": { + "comment": "Java Servlet request dispatcher" + } } }, "template": { From 78e6cac339b6066ad607f838599a10b2511c799b Mon Sep 17 00:00:00 2001 From: Sylvain Juge Date: Wed, 12 May 2021 18:39:34 +0200 Subject: [PATCH 03/25] make opentracing tests pass --- .../src/test/java/co/elastic/apm/agent/MockReporter.java | 2 +- .../src/test/resources/json-specs/span_types.json | 7 +++++++ 2 files changed, 8 insertions(+), 1 deletion(-) diff --git a/apm-agent-core/src/test/java/co/elastic/apm/agent/MockReporter.java b/apm-agent-core/src/test/java/co/elastic/apm/agent/MockReporter.java index e29d02f61c..602bc74698 100644 --- a/apm-agent-core/src/test/java/co/elastic/apm/agent/MockReporter.java +++ b/apm-agent-core/src/test/java/co/elastic/apm/agent/MockReporter.java @@ -199,7 +199,7 @@ private void verifySpanType(Span span) { String subtype = Objects.toString(span.getSubtype()); - JsonNode typeJson = getMandatoryObject(SPAN_TYPES_SPEC, type, String.format("span type %s", type)); + JsonNode typeJson = getMandatoryObject(SPAN_TYPES_SPEC, type, String.format("span type '%s'", type)); String typeComment = getOptionalComment(typeJson); diff --git a/apm-agent-core/src/test/resources/json-specs/span_types.json b/apm-agent-core/src/test/resources/json-specs/span_types.json index 2dc40ddccf..1220971196 100644 --- a/apm-agent-core/src/test/resources/json-specs/span_types.json +++ b/apm-agent-core/src/test/resources/json-specs/span_types.json @@ -15,6 +15,13 @@ } } }, + "unknown": { + "subtypes": { + "null": { + "comment": "TODO : find if relevant" + } + } + }, "async": { "subtypes": { "blocking-queue": { From 3f48a4728fb56d8e425d5ec9f5c226759c5d91f8 Mon Sep 17 00:00:00 2001 From: Sylvain Juge Date: Wed, 12 May 2021 18:44:34 +0200 Subject: [PATCH 04/25] fix tests for OpenTracing --- .../test/resources/json-specs/span_types.json | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/apm-agent-core/src/test/resources/json-specs/span_types.json b/apm-agent-core/src/test/resources/json-specs/span_types.json index 1220971196..346fbba5ef 100644 --- a/apm-agent-core/src/test/resources/json-specs/span_types.json +++ b/apm-agent-core/src/test/resources/json-specs/span_types.json @@ -29,6 +29,13 @@ } } }, + "cache": { + "subtypes": { + "redis": { + "comment": "TODO : should we make OpenTracing use 'db.redis' instead ?" + } + } + }, "custom": { "comment": "API custom instrumentation", "subtypes": { @@ -49,6 +56,9 @@ "elasticsearch": { "comment": "Elasticsearch" }, + "foo": { + "comment": "TODO remove this (test)" + }, "h2": { "comment": "H2" }, @@ -83,6 +93,9 @@ }, "ext": { "subtypes": { + "null": { + "comment": "TODO find where it's being used in OpenTracing" + }, "http": { "comment": "TODO remove this, 'external/http' should be used instead" } @@ -195,6 +208,9 @@ }, "Thymeleaf": { "comment": "" + }, + "jsf": { + "comment": "Java Server Faces" } } }, From 692a41238e520613ffd504e788255f67d4b082c0 Mon Sep 17 00:00:00 2001 From: Sylvain Juge Date: Thu, 20 May 2021 17:02:38 +0200 Subject: [PATCH 05/25] fix external plugin tests --- .../co/elastic/apm/agent/MockReporter.java | 3 +++ .../test/resources/json-specs/span_types.json | 22 +++++++++++++------ 2 files changed, 18 insertions(+), 7 deletions(-) diff --git a/apm-agent-core/src/test/java/co/elastic/apm/agent/MockReporter.java b/apm-agent-core/src/test/java/co/elastic/apm/agent/MockReporter.java index 602bc74698..f69ffa4b9c 100644 --- a/apm-agent-core/src/test/java/co/elastic/apm/agent/MockReporter.java +++ b/apm-agent-core/src/test/java/co/elastic/apm/agent/MockReporter.java @@ -206,12 +206,15 @@ private void verifySpanType(Span span) { JsonNode subTypesJson = getMandatoryObject(typeJson, "subtypes", String.format("subtypes for type '%s'", type)); JsonNode subTypeJson = getMandatoryObject(subTypesJson, subtype, String.format("span subtype '%s' for type '%s' (%s)", subtype, type, typeComment)); + + // TODO : check for sub-type span actions also ? } private JsonNode getMandatoryObject(JsonNode json, String name, String desc) { JsonNode jsonNode = json.get(name); assertThat(jsonNode) .describedAs(desc) + .describedAs("unknown span type '%s'", name) .isNotNull(); assertThat(jsonNode.isObject()) .isTrue(); diff --git a/apm-agent-core/src/test/resources/json-specs/span_types.json b/apm-agent-core/src/test/resources/json-specs/span_types.json index 346fbba5ef..4253fdfb0f 100644 --- a/apm-agent-core/src/test/resources/json-specs/span_types.json +++ b/apm-agent-core/src/test/resources/json-specs/span_types.json @@ -15,13 +15,6 @@ } } }, - "unknown": { - "subtypes": { - "null": { - "comment": "TODO : find if relevant" - } - } - }, "async": { "subtypes": { "blocking-queue": { @@ -170,6 +163,14 @@ } } }, + "plugin": { + "comment": "TODO : only for testing ?", + "subtypes": { + "external": { + "comment": "TODO : only for testing ?" + } + } + }, "process": { "comment": "External process TODO find way to allow for wildcard (or not use binary name as subtype)", "subtypes": { @@ -220,6 +221,13 @@ "comment": "TODO remove this (test)" } } + }, + "unknown": { + "subtypes": { + "null": { + "comment": "TODO : find if relevant" + } + } } } From 71b19cb62ec895b92bba440f09b7903b97cad0fa Mon Sep 17 00:00:00 2001 From: Sylvain Juge Date: Wed, 26 May 2021 09:53:06 +0200 Subject: [PATCH 06/25] update grpc version --- .../apm/agent/grpc/latest/testapp/generated/HelloGrpc.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/apm-agent-plugins/apm-grpc/apm-grpc-test-latest/src/test/java/co/elastic/apm/agent/grpc/latest/testapp/generated/HelloGrpc.java b/apm-agent-plugins/apm-grpc/apm-grpc-test-latest/src/test/java/co/elastic/apm/agent/grpc/latest/testapp/generated/HelloGrpc.java index 370ea7f16a..095e0f6d36 100644 --- a/apm-agent-plugins/apm-grpc/apm-grpc-test-latest/src/test/java/co/elastic/apm/agent/grpc/latest/testapp/generated/HelloGrpc.java +++ b/apm-agent-plugins/apm-grpc/apm-grpc-test-latest/src/test/java/co/elastic/apm/agent/grpc/latest/testapp/generated/HelloGrpc.java @@ -29,7 +29,7 @@ /** */ @javax.annotation.Generated( - value = "by gRPC proto compiler (version 1.37.0)", + value = "by gRPC proto compiler (version 1.38.0)", comments = "Source: rpc.proto") public final class HelloGrpc { From 33e6ed3ca72a77f2e57a2b4ea726745f140fe4b5 Mon Sep 17 00:00:00 2001 From: Sylvain Juge Date: Wed, 26 May 2021 13:29:02 +0200 Subject: [PATCH 07/25] allow to opt-out check + modify tests --- .../agent/AbstractInstrumentationTest.java | 3 + .../co/elastic/apm/agent/MockReporter.java | 97 ++++++----- .../test/resources/json-specs/span_types.json | 153 +----------------- .../TransactionInstrumentationTest.java | 7 +- .../co/elastic/apm/api/AnnotationApiTest.java | 13 +- .../BlockingQueueContextPropagationTest.java | 4 + .../api/ElasticApmApiInstrumentationTest.java | 3 +- ...sticsearchRestClientInstrumentationIT.java | 19 +-- .../AbstractEsClientInstrumentationTest.java | 28 ++-- .../apm/agent/kafka/KafkaLegacyClientIT.java | 2 +- .../co/elastic/apm/agent/kafka/KafkaIT.java | 3 +- .../apm/agent/kafka/KafkaLegacyBrokerIT.java | 4 +- .../apm/agent/process/ProcessHelper.java | 2 - .../apm/agent/process/ProcessHelperTest.java | 6 +- .../agent/profiler/SamplingProfilerTest.java | 2 +- .../lettuce/Lettuce3InstrumentationTest.java | 2 +- .../lettuce/Lettuce4InstrumentationTest.java | 2 +- .../lettuce/Lettuce5InstrumentationTest.java | 2 +- .../servlet/ServletInstrumentationTest.java | 7 + .../plugin/PluginInstrumentationTest.java | 10 +- 20 files changed, 134 insertions(+), 235 deletions(-) diff --git a/apm-agent-core/src/test/java/co/elastic/apm/agent/AbstractInstrumentationTest.java b/apm-agent-core/src/test/java/co/elastic/apm/agent/AbstractInstrumentationTest.java index a38782fd24..8c732cdebf 100644 --- a/apm-agent-core/src/test/java/co/elastic/apm/agent/AbstractInstrumentationTest.java +++ b/apm-agent-core/src/test/java/co/elastic/apm/agent/AbstractInstrumentationTest.java @@ -88,6 +88,9 @@ public static ConfigurationRegistry getConfig() { @After @AfterEach public final void cleanUp() { + // re-enable all reporter checks + reporter.resetChecks(); + SpyConfiguration.reset(config); try { if (validateRecycling) { diff --git a/apm-agent-core/src/test/java/co/elastic/apm/agent/MockReporter.java b/apm-agent-core/src/test/java/co/elastic/apm/agent/MockReporter.java index f69ffa4b9c..fafdeaf7eb 100644 --- a/apm-agent-core/src/test/java/co/elastic/apm/agent/MockReporter.java +++ b/apm-agent-core/src/test/java/co/elastic/apm/agent/MockReporter.java @@ -53,7 +53,6 @@ import java.util.Collections; import java.util.List; import java.util.Map; -import java.util.Objects; import java.util.Optional; import java.util.Set; import java.util.concurrent.CopyOnWriteArrayList; @@ -80,7 +79,12 @@ public class MockReporter implements Reporter { // A map of exit span type to actions that that do not support address and port discovery private static final Map> SPAN_ACTIONS_WITHOUT_ADDRESS; // And for any case the disablement of the check cannot rely on subtype (eg Redis, where Jedis supports and Lettuce does not) - private boolean disableDestinationAddressCheck; + private boolean checkDestinationAddress = true; + // Allows optional opt-out for unknown outcome + private boolean checkUnknownOutcomes = true; + // Allows optional opt-out from strick span type/sub-type checking + private boolean checkStrictSpanType = true; + private final List transactions = Collections.synchronizedList(new ArrayList<>()); private final List spans = Collections.synchronizedList(new ArrayList<>()); @@ -88,7 +92,7 @@ public class MockReporter implements Reporter { private final List bytes = new CopyOnWriteArrayList<>(); private final ObjectMapper objectMapper; private final boolean verifyJsonSchema; - private boolean checkUnknownOutcomes = true; + private boolean closed; private static final JsonNode SPAN_TYPES_SPEC = TestJsonSpec.getJson("span_types.json"); @@ -123,22 +127,38 @@ private static JsonSchema getSchema(String resource) { } /** - * @param enable {@literal true} to enable unknown outcome check, {@literal false} to allow for unknown outcome + * Sets all optional checks to their default value (enabled), should be used as a shortcut to reset mock reporter state + * after/before using it for a single test execution */ - public void checkUnknownOutcome(boolean enable) { - checkUnknownOutcomes = enable; + public void resetChecks() { + checkDestinationAddress = true; + checkUnknownOutcomes = true; + checkStrictSpanType = true; } - public void disableDestinationAddressCheck() { - disableDestinationAddressCheck = true; + /** + * Disables unknown outcome check + */ + public void disableCheckUnknownOutcome() { + checkUnknownOutcomes = false; } - public void enableDestinationAddressCheck() { - disableDestinationAddressCheck = false; + /** + * Disables destination address check + */ + public void disableCheckDestinationAddress() { + checkDestinationAddress = false; } - public boolean isDisableDestinationAddressCheck() { - return disableDestinationAddressCheck; + public boolean checkDestinationAddress(){ + return checkDestinationAddress; + } + + /** + * Disables strict span type and sub-type check (against shared spec) + */ + public void disableCheckStrictSpanType() { + checkStrictSpanType = false; } @Override @@ -191,40 +211,27 @@ public synchronized void report(Span span) { spans.add(span); } + private void verifySpanType(Span span) { String type = span.getType(); assertThat(type) .describedAs("span type is mandatory") .isNotNull(); - String subtype = Objects.toString(span.getSubtype()); - - JsonNode typeJson = getMandatoryObject(SPAN_TYPES_SPEC, type, String.format("span type '%s'", type)); + if (checkStrictSpanType) { + JsonNode typeJson = getMandatoryJson(SPAN_TYPES_SPEC, type, String.format("span type '%s'", type)); - String typeComment = getOptionalComment(typeJson); + String typeComment = getOptionalComment(typeJson); - JsonNode subTypesJson = getMandatoryObject(typeJson, "subtypes", String.format("subtypes for type '%s'", type)); - - JsonNode subTypeJson = getMandatoryObject(subTypesJson, subtype, String.format("span subtype '%s' for type '%s' (%s)", subtype, type, typeComment)); - - // TODO : check for sub-type span actions also ? - } + String subType = span.getSubtype(); + if (subType != null) { + JsonNode subTypesJson = getMandatoryJson(typeJson, "subtypes", String.format("subtypes for type '%s'", type)); - private JsonNode getMandatoryObject(JsonNode json, String name, String desc) { - JsonNode jsonNode = json.get(name); - assertThat(jsonNode) - .describedAs(desc) - .describedAs("unknown span type '%s'", name) - .isNotNull(); - assertThat(jsonNode.isObject()) - .isTrue(); - return jsonNode; - } + JsonNode subTypeJson = getMandatoryJson(subTypesJson, subType, String.format("span subtype '%s' for type '%s' (%s)", subType, type, typeComment)); + assertThat(subTypeJson).isNotNull(); + } + } - private String getOptionalComment(JsonNode json) { - return Optional.ofNullable(json.get("comment")) - .map(JsonNode::asText) - .orElse(""); } private void verifyDestinationFields(Span span) { @@ -232,7 +239,7 @@ private void verifyDestinationFields(Span span) { return; } Destination destination = span.getContext().getDestination(); - if (!disableDestinationAddressCheck && !SPAN_TYPES_WITHOUT_ADDRESS.contains(span.getSubtype())) { + if (checkDestinationAddress && !SPAN_TYPES_WITHOUT_ADDRESS.contains(span.getSubtype())) { // see if this span's action is not supported for its subtype Collection unsupportedActions = SPAN_ACTIONS_WITHOUT_ADDRESS.getOrDefault(span.getSubtype(), Collections.emptySet()); if (!unsupportedActions.contains(span.getAction())) { @@ -534,4 +541,20 @@ public void awaitUntilAsserted(long timeoutMs, ThrowingRunnable runnable) { private static boolean hasEmptyTraceContext(AbstractSpan item) { return item.getTraceContext().getId().isEmpty(); } + + private static JsonNode getMandatoryJson(JsonNode json, String name, String desc) { + JsonNode jsonNode = json.get(name); + assertThat(jsonNode) + .describedAs(desc) + .isNotNull(); + assertThat(jsonNode.isObject()) + .isTrue(); + return jsonNode; + } + + private static String getOptionalComment(JsonNode json) { + return Optional.ofNullable(json.get("comment")) + .map(JsonNode::asText) + .orElse(""); + } } diff --git a/apm-agent-core/src/test/resources/json-specs/span_types.json b/apm-agent-core/src/test/resources/json-specs/span_types.json index 4253fdfb0f..1f2efa5628 100644 --- a/apm-agent-core/src/test/resources/json-specs/span_types.json +++ b/apm-agent-core/src/test/resources/json-specs/span_types.json @@ -1,41 +1,13 @@ { "app": { "subtypes": { - "subtype": { - "comment": "TODO : fix this anomaly (test)" - }, - "": { - "comment": "TODO : fix this anomaly" - }, "inferred": { "comment": "Sampling profiler inferred spans" - }, - "null": { - "comment": "TODO : find if relevant" - } - } - }, - "async": { - "subtypes": { - "blocking-queue": { - "comment": "TODO : find where it's being used" - } - } - }, - "cache": { - "subtypes": { - "redis": { - "comment": "TODO : should we make OpenTracing use 'db.redis' instead ?" } } }, "custom": { - "comment": "API custom instrumentation", - "subtypes": { - "null": { - "comment": "TODO : fix this anomaly" - } - } + "comment": "API custom instrumentation" }, "db": { "comment": "database span", @@ -49,9 +21,6 @@ "elasticsearch": { "comment": "Elasticsearch" }, - "foo": { - "comment": "TODO remove this (test)" - }, "h2": { "comment": "H2" }, @@ -67,9 +36,6 @@ "mysql": { "comment": "MySQL" }, - "null": { - "comment": "TODO : fix this anomaly" - }, "oracle": { "comment": "Oracle Database" }, @@ -84,24 +50,6 @@ } } }, - "ext": { - "subtypes": { - "null": { - "comment": "TODO find where it's being used in OpenTracing" - }, - "http": { - "comment": "TODO remove this, 'external/http' should be used instead" - } - } - }, - "ext.http": { - "comment": "TODO remove this, 'external/http' should be used instead", - "subtypes": { - "": { - "comment": "TODO remove this, 'external/http' should be used instead" - } - } - }, "external": { "subtypes": { "dubbo": { @@ -115,41 +63,6 @@ } } }, - "foo": { - "comment": "TODO remove this (test)", - "subtypes": { - "bar": { - "comment": "TODO remove this (test)" - }, - "null": { - "comment": "TODO remove this (test)" - } - } - }, - "foo2": { - "comment": "TODO remove this (test)", - "subtypes": { - "null": { - "comment": "TODO remove this (test)" - } - } - }, - "foo3": { - "comment": "TODO remove this (test)", - "subtypes": { - "null": { - "comment": "TODO remove this (test)" - } - } - }, - "http": { - "comment": "TODO remove this", - "subtypes": { - "get": { - "comment": "TODO remove this (tests)" - } - } - }, "messaging": { "subtypes": { "jms": { @@ -163,71 +76,11 @@ } } }, - "plugin": { - "comment": "TODO : only for testing ?", - "subtypes": { - "external": { - "comment": "TODO : only for testing ?" - } - } - }, "process": { - "comment": "External process TODO find way to allow for wildcard (or not use binary name as subtype)", - "subtypes": { - "hello": { - "comment": "TODO remove this (test)" - }, - "java": { - "comment": "TODO remove this (test)" - } - } - }, - "servlet": { - "subtypes": { - "request-dispatcher": { - "comment": "Java Servlet request dispatcher" - } - } + "comment": "External process" }, "template": { - "comment": "Template engines", - "subtypes": { - "FreeMarker": { - "comment": "" - }, - "GroovyMarkup": { - "comment": "" - }, - "InternalResource": { - "comment": "" - }, - "Jade": { - "comment": "" - }, - "MappingJackson2Json": { - "comment": "" - }, - "Thymeleaf": { - "comment": "" - }, - "jsf": { - "comment": "Java Server Faces" - } - } - }, - "test": { - "subtypes": { - "null": { - "comment": "TODO remove this (test)" - } - } - }, - "unknown": { - "subtypes": { - "null": { - "comment": "TODO : find if relevant" - } - } + "comment": "Template engines (no sub-type as really platform-specific)" } } diff --git a/apm-agent-plugins/apm-api-plugin/src/test/java/co/elastic/apm/agent/pluginapi/TransactionInstrumentationTest.java b/apm-agent-plugins/apm-api-plugin/src/test/java/co/elastic/apm/agent/pluginapi/TransactionInstrumentationTest.java index ed69857552..9c63b2047a 100644 --- a/apm-agent-plugins/apm-api-plugin/src/test/java/co/elastic/apm/agent/pluginapi/TransactionInstrumentationTest.java +++ b/apm-agent-plugins/apm-api-plugin/src/test/java/co/elastic/apm/agent/pluginapi/TransactionInstrumentationTest.java @@ -166,6 +166,9 @@ void testChaining() { @Test public void startSpan() { + // custom spans not part of shared spec + reporter.disableCheckStrictSpanType(); + Span span = transaction.startSpan("foo", null, null); span.setName("bar"); Span child = span.startSpan("foo2", null, null); @@ -222,7 +225,7 @@ public void testGetErrorIdWithTransactionCaptureException() { @Test public void testGetErrorIdWithSpanCaptureException() { String errorId = null; - Span span = transaction.startSpan("foo", null, null); + Span span = transaction.startSpan("custom", null, null); span.setName("bar"); try { throw new RuntimeException("test exception"); @@ -237,7 +240,7 @@ public void testGetErrorIdWithSpanCaptureException() { @Test void setOutcome_unknown() { - reporter.checkUnknownOutcome(false); + reporter.disableCheckUnknownOutcome(); testSetOutcome(Outcome.UNKNOWN); } diff --git a/apm-agent-plugins/apm-api-plugin/src/test/java/co/elastic/apm/api/AnnotationApiTest.java b/apm-agent-plugins/apm-api-plugin/src/test/java/co/elastic/apm/api/AnnotationApiTest.java index 303e4db64e..90fda2153b 100644 --- a/apm-agent-plugins/apm-api-plugin/src/test/java/co/elastic/apm/api/AnnotationApiTest.java +++ b/apm-agent-plugins/apm-api-plugin/src/test/java/co/elastic/apm/api/AnnotationApiTest.java @@ -93,7 +93,7 @@ private void testTransactionAndSpanTypes(boolean useLegacyTyping) { assertThat(reporter.getSpans()).hasSize(1); Span internalSpan = reporter.getFirstSpan(); assertThat(internalSpan.getNameAsString()).isEqualTo("spanWithType"); - assertThat(internalSpan.getType()).isEqualTo("ext"); + assertThat(internalSpan.getType()).isEqualTo("external"); assertThat(internalSpan.getSubtype()).isEqualTo("http"); assertThat(internalSpan.getAction()).isEqualTo("okhttp"); } @@ -104,13 +104,16 @@ void testMissingSubtype() { assertThat(reporter.getSpans()).hasSize(1); Span internalSpan = reporter.getFirstSpan(); assertThat(internalSpan.getNameAsString()).isEqualTo("spanWithMissingSubtype"); - assertThat(internalSpan.getType()).isEqualTo("ext.http"); + assertThat(internalSpan.getType()).isEqualTo("external.http"); assertThat(internalSpan.getSubtype()).isEqualTo(""); assertThat(internalSpan.getAction()).isEqualTo("okhttp"); } @Test void testTracedTransactionAndSpan() { + // type = app, subtype = subtype is not part of shared spec + reporter.disableCheckStrictSpanType(); + TracedAnnotationTest.tracedTransaction(); assertThat(reporter.getTransactions()).hasSize(1); assertThat(reporter.getSpans()).hasSize(1); @@ -180,12 +183,12 @@ static void transactionWithType(boolean useLegacyTyping) { } } - @CaptureSpan(value = "spanWithType", type = "ext.http.okhttp") + @CaptureSpan(value = "spanWithType", type = "external.http.okhttp") private static void spanWithHierarchicalType() { } - @CaptureSpan(value = "spanWithType", type = "ext", subtype = "http", action = "okhttp") + @CaptureSpan(value = "spanWithType", type = "external", subtype = "http", action = "okhttp") private static void spanWithSplitTypes() { } @@ -195,7 +198,7 @@ static void transactionForMissingSpanSubtype() { spanWithMissingSubtype(); } - @CaptureSpan(value = "spanWithMissingSubtype", type = "ext.http", action = "okhttp") + @CaptureSpan(value = "spanWithMissingSubtype", type = "external.http", action = "okhttp") private static void spanWithMissingSubtype() { } diff --git a/apm-agent-plugins/apm-api-plugin/src/test/java/co/elastic/apm/api/BlockingQueueContextPropagationTest.java b/apm-agent-plugins/apm-api-plugin/src/test/java/co/elastic/apm/api/BlockingQueueContextPropagationTest.java index 5dfcc782d2..5cc38cb6be 100644 --- a/apm-agent-plugins/apm-api-plugin/src/test/java/co/elastic/apm/api/BlockingQueueContextPropagationTest.java +++ b/apm-agent-plugins/apm-api-plugin/src/test/java/co/elastic/apm/api/BlockingQueueContextPropagationTest.java @@ -106,6 +106,9 @@ public void testAsyncTransactionDelegation() throws ExecutionException, Interrup @Test public void testAsyncSpanDelegation() throws ExecutionException, InterruptedException { + // span type/subtype 'async/blocking-queue' is not part of shared spec + reporter.disableCheckStrictSpanType(); + Transaction transaction = ElasticApm.startTransaction(); long startTime = TimeUnit.NANOSECONDS.toMicros(nanoTimeOffsetToEpoch + System.nanoTime()); transaction.setStartTimestamp(startTime); @@ -136,6 +139,7 @@ public void testAsyncSpanDelegation() throws ExecutionException, InterruptedExce assertThat(reportedSpan.getTraceContext().getTraceId()).isEqualTo(reportedTransaction.getTraceContext().getTraceId()); assertThat(reportedSpan).isNotNull(); assertThat(reportedSpan.getType()).isEqualTo("async"); + assertThat(reportedSpan.getSubtype()).isEqualTo("blocking-queue"); assertThat(reportedSpan.getTimestamp() - transactionTimestamp).isGreaterThanOrEqualTo(TimeUnit.MILLISECONDS.toMicros(100)); assertThat(reportedSpan.getDuration()).isBetween( TimeUnit.MILLISECONDS.toMicros(10), diff --git a/apm-agent-plugins/apm-api-plugin/src/test/java/co/elastic/apm/api/ElasticApmApiInstrumentationTest.java b/apm-agent-plugins/apm-api-plugin/src/test/java/co/elastic/apm/api/ElasticApmApiInstrumentationTest.java index c50b786310..54ddd574e8 100644 --- a/apm-agent-plugins/apm-api-plugin/src/test/java/co/elastic/apm/api/ElasticApmApiInstrumentationTest.java +++ b/apm-agent-plugins/apm-api-plugin/src/test/java/co/elastic/apm/api/ElasticApmApiInstrumentationTest.java @@ -29,7 +29,6 @@ import co.elastic.apm.agent.impl.TextHeaderMapAccessor; import co.elastic.apm.agent.impl.TracerInternalApiUtils; import co.elastic.apm.agent.impl.transaction.AbstractSpan; -import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; import java.util.Collections; @@ -92,7 +91,7 @@ void testCaptureExceptionNoopSpan() { @Test void testTransactionWithError() { - reporter.checkUnknownOutcome(false); + reporter.disableCheckUnknownOutcome(); final Transaction transaction = ElasticApm.startTransaction(); transaction.setType("request"); diff --git a/apm-agent-plugins/apm-es-restclient-plugin/apm-es-restclient-plugin-7_x/src/test/java/co/elastic/apm/agent/es/restclient/v7_x/ElasticsearchRestClientInstrumentationIT.java b/apm-agent-plugins/apm-es-restclient-plugin/apm-es-restclient-plugin-7_x/src/test/java/co/elastic/apm/agent/es/restclient/v7_x/ElasticsearchRestClientInstrumentationIT.java index 83eab748e3..393d7dadbe 100644 --- a/apm-agent-plugins/apm-es-restclient-plugin/apm-es-restclient-plugin-7_x/src/test/java/co/elastic/apm/agent/es/restclient/v7_x/ElasticsearchRestClientInstrumentationIT.java +++ b/apm-agent-plugins/apm-es-restclient-plugin/apm-es-restclient-plugin-7_x/src/test/java/co/elastic/apm/agent/es/restclient/v7_x/ElasticsearchRestClientInstrumentationIT.java @@ -25,6 +25,7 @@ package co.elastic.apm.agent.es.restclient.v7_x; import co.elastic.apm.agent.es.restclient.v6_4.AbstractEs6_4ClientInstrumentationTest; +import co.elastic.apm.agent.impl.transaction.Outcome; import co.elastic.apm.agent.impl.transaction.Span; import org.apache.http.HttpHost; import org.apache.http.auth.AuthScope; @@ -65,19 +66,15 @@ import org.junit.Test; import org.junit.runner.RunWith; import org.junit.runners.Parameterized; -import org.slf4j.Logger; -import org.slf4j.LoggerFactory; import java.io.IOException; import java.util.List; -import java.util.concurrent.CompletableFuture; import java.util.concurrent.ExecutionException; import static org.assertj.core.api.Assertions.assertThat; @RunWith(Parameterized.class) public class ElasticsearchRestClientInstrumentationIT extends AbstractEs6_4ClientInstrumentationTest { - private static final Logger logger = LoggerFactory.getLogger(ElasticsearchRestClientInstrumentationIT.class); private static final String ELASTICSEARCH_CONTAINER_VERSION = "docker.elastic.co/elasticsearch/elasticsearch:7.11.0"; @@ -111,9 +108,9 @@ public static void stopElasticsearchContainerAndClient() throws IOException { @Test public void testCancelScenario() throws InterruptedException, ExecutionException, IOException { // When spans are cancelled, we can't know the actual address, because there is no response, and we set the outcome as UNKNOWN - reporter.disableDestinationAddressCheck(); - reporter.checkUnknownOutcome(false); - super.disableHttpUrlCheck(); + reporter.disableCheckDestinationAddress(); + reporter.disableCheckUnknownOutcome(); + disableHttpUrlCheck(); createDocument(); reporter.reset(); @@ -139,11 +136,11 @@ public void onFailure(Exception e) { Span searchSpan = spans.get(0); validateSpanContent(searchSpan, String.format("Elasticsearch: POST /%s/_search", INDEX), -1, "POST"); - deleteDocument(); + assertThat(searchSpan.getOutcome()) + .describedAs("span outcome should be unknown when cancelled") + .isEqualTo(Outcome.UNKNOWN); - reporter.enableDestinationAddressCheck(); - reporter.checkUnknownOutcome(true); - super.enableHttpUrlCheck(); + deleteDocument(); } @Override diff --git a/apm-agent-plugins/apm-es-restclient-plugin/apm-es-restclient-plugin-common/src/test/java/co/elastic/apm/agent/es/restclient/AbstractEsClientInstrumentationTest.java b/apm-agent-plugins/apm-es-restclient-plugin/apm-es-restclient-plugin-common/src/test/java/co/elastic/apm/agent/es/restclient/AbstractEsClientInstrumentationTest.java index ff1735680c..bb9375ea47 100644 --- a/apm-agent-plugins/apm-es-restclient-plugin/apm-es-restclient-plugin-common/src/test/java/co/elastic/apm/agent/es/restclient/AbstractEsClientInstrumentationTest.java +++ b/apm-agent-plugins/apm-es-restclient-plugin/apm-es-restclient-plugin-common/src/test/java/co/elastic/apm/agent/es/restclient/AbstractEsClientInstrumentationTest.java @@ -63,14 +63,13 @@ public abstract class AbstractEsClientInstrumentationTest extends AbstractInstru protected boolean async; - private boolean disableHttpUrlCheck = false; + private boolean checkHttpUrl = true; + /** + * Disables HTTP URL check for the current test method + */ public void disableHttpUrlCheck() { - disableHttpUrlCheck = true; - } - - public void enableHttpUrlCheck() { - disableHttpUrlCheck = false; + checkHttpUrl = false; } @Parameterized.Parameters(name = "Async={0}") @@ -93,6 +92,12 @@ public static void stopContainer() { @Before public void startTransaction() { + // While JUnit does not recycle test class instances between method invocations by default + // this test should not be required, but it allows to ensure proper correctness even if that changes + assertThat(checkHttpUrl) + .describedAs("checking HTTP URLs should be enabled by default") + .isTrue(); + startTestRootTransaction("ES Transaction"); } @@ -105,7 +110,6 @@ public void endTransaction() { } public void assertThatErrorsExistWhenDeleteNonExistingIndex() { - List errorCaptures = reporter.getErrors(); assertThat(errorCaptures).hasSize(1); ErrorCapture errorCapture = errorCaptures.get(0); @@ -130,7 +134,6 @@ protected void validateDbContextContent(Span span, String statement) { assertThat(db.getStatementBuffer().toString()).isEqualTo(statement); } - protected void validateSpanContent(Span span, String expectedName, int statusCode, String method) { validateSpanContentWithoutContext(span, expectedName, statusCode, method); validateHttpContextContent(span.getContext().getHttp(), statusCode, method); @@ -139,10 +142,12 @@ protected void validateSpanContent(Span span, String expectedName, int statusCod private void validateDestinationContextContent(Destination destination) { assertThat(destination).isNotNull(); - if (!reporter.isDisableDestinationAddressCheck()) { + + if (reporter.checkDestinationAddress()) { assertThat(destination.getAddress().toString()).isEqualTo(container.getContainerIpAddress()); assertThat(destination.getPort()).isEqualTo(container.getMappedPort(9200)); } + assertThat(destination.getService().getName().toString()).isEqualTo(ELASTICSEARCH); assertThat(destination.getService().getResource().toString()).isEqualTo(ELASTICSEARCH); assertThat(destination.getService().getType()).isEqualTo(SPAN_TYPE); @@ -152,27 +157,24 @@ private void validateHttpContextContent(Http http, int statusCode, String method assertThat(http).isNotNull(); assertThat(http.getMethod()).isEqualTo(method); assertThat(http.getStatusCode()).isEqualTo(statusCode); - if (!disableHttpUrlCheck) { + if (checkHttpUrl) { assertThat(http.getUrl()).isEqualTo("http://" + container.getHttpHostAddress()); } } protected void validateSpanContentAfterIndexCreateRequest() { - List spans = reporter.getSpans(); assertThat(spans).hasSize(1); validateSpanContent(spans.get(0), String.format("Elasticsearch: PUT /%s", SECOND_INDEX), 200, "PUT"); } protected void validateSpanContentAfterIndexDeleteRequest() { - List spans = reporter.getSpans(); assertThat(spans).hasSize(1); validateSpanContent(spans.get(0), String.format("Elasticsearch: DELETE /%s", SECOND_INDEX), 200, "DELETE"); } protected void validateSpanContentAfterBulkRequest() { - List spans = reporter.getSpans(); assertThat(spans).hasSize(1); assertThat(spans.get(0).getNameAsString()).isEqualTo("Elasticsearch: POST /_bulk"); diff --git a/apm-agent-plugins/apm-kafka-plugin/apm-kafka-base-plugin/src/test/java/co/elastic/apm/agent/kafka/KafkaLegacyClientIT.java b/apm-agent-plugins/apm-kafka-plugin/apm-kafka-base-plugin/src/test/java/co/elastic/apm/agent/kafka/KafkaLegacyClientIT.java index 50e1874090..a45b33da08 100644 --- a/apm-agent-plugins/apm-kafka-plugin/apm-kafka-base-plugin/src/test/java/co/elastic/apm/agent/kafka/KafkaLegacyClientIT.java +++ b/apm-agent-plugins/apm-kafka-plugin/apm-kafka-base-plugin/src/test/java/co/elastic/apm/agent/kafka/KafkaLegacyClientIT.java @@ -241,7 +241,7 @@ public void testDestinationAddressCollectionDisabled() { when(messagingConfiguration.shouldCollectQueueAddress()).thenReturn(false); testScenario = TestScenario.TOPIC_ADDRESS_COLLECTION_DISABLED; consumerThread.setIterationMode(RecordIterationMode.ITERABLE_FOR); - reporter.disableDestinationAddressCheck(); + reporter.disableCheckDestinationAddress(); sendTwoRecordsAndConsumeReplies(); verifyTracing(); } diff --git a/apm-agent-plugins/apm-kafka-plugin/apm-kafka-headers-plugin/src/test/java/co/elastic/apm/agent/kafka/KafkaIT.java b/apm-agent-plugins/apm-kafka-plugin/apm-kafka-headers-plugin/src/test/java/co/elastic/apm/agent/kafka/KafkaIT.java index 8ab377b654..b8a9642885 100644 --- a/apm-agent-plugins/apm-kafka-plugin/apm-kafka-headers-plugin/src/test/java/co/elastic/apm/agent/kafka/KafkaIT.java +++ b/apm-agent-plugins/apm-kafka-plugin/apm-kafka-headers-plugin/src/test/java/co/elastic/apm/agent/kafka/KafkaIT.java @@ -57,7 +57,6 @@ import org.junit.BeforeClass; import org.junit.Ignore; import org.junit.Test; -import org.testcontainers.containers.GenericContainer; import org.testcontainers.containers.KafkaContainer; import org.testcontainers.shaded.com.google.common.collect.ImmutableMap; @@ -264,7 +263,7 @@ public void testDestinationAddressCollectionDisabled() { when(messagingConfiguration.shouldCollectQueueAddress()).thenReturn(false); testScenario = TestScenario.TOPIC_ADDRESS_COLLECTION_DISABLED; consumerThread.setIterationMode(RecordIterationMode.ITERABLE_FOR); - reporter.disableDestinationAddressCheck(); + reporter.disableCheckDestinationAddress(); sendTwoRecordsAndConsumeReplies(); verifyTracing(); } diff --git a/apm-agent-plugins/apm-kafka-plugin/apm-kafka-headers-plugin/src/test/java/co/elastic/apm/agent/kafka/KafkaLegacyBrokerIT.java b/apm-agent-plugins/apm-kafka-plugin/apm-kafka-headers-plugin/src/test/java/co/elastic/apm/agent/kafka/KafkaLegacyBrokerIT.java index cdc2dbff92..6e6df6a584 100644 --- a/apm-agent-plugins/apm-kafka-plugin/apm-kafka-headers-plugin/src/test/java/co/elastic/apm/agent/kafka/KafkaLegacyBrokerIT.java +++ b/apm-agent-plugins/apm-kafka-plugin/apm-kafka-headers-plugin/src/test/java/co/elastic/apm/agent/kafka/KafkaLegacyBrokerIT.java @@ -107,7 +107,7 @@ public KafkaLegacyBrokerIT() { @BeforeClass public static void setup() { - reporter.disableDestinationAddressCheck(); + reporter.disableCheckDestinationAddress(); // confluent versions 3.2.x correspond Kafka versions 0.10.2.2 - // https://docs.confluent.io/current/installation/versions-interoperability.html#cp-and-apache-ak-compatibility @@ -225,7 +225,7 @@ public void testDestinationAddressCollectionDisabled() { when(messagingConfiguration.shouldCollectQueueAddress()).thenReturn(false); testScenario = TestScenario.TOPIC_ADDRESS_COLLECTION_DISABLED; consumerThread.setIterationMode(RecordIterationMode.ITERABLE_FOR); - reporter.disableDestinationAddressCheck(); + reporter.disableCheckDestinationAddress(); sendTwoRecordsAndConsumeReplies(); verifyTracing(); } diff --git a/apm-agent-plugins/apm-process-plugin/src/main/java/co/elastic/apm/agent/process/ProcessHelper.java b/apm-agent-plugins/apm-process-plugin/src/main/java/co/elastic/apm/agent/process/ProcessHelper.java index 7aa3e44cf7..d8606bc364 100644 --- a/apm-agent-plugins/apm-process-plugin/src/main/java/co/elastic/apm/agent/process/ProcessHelper.java +++ b/apm-agent-plugins/apm-process-plugin/src/main/java/co/elastic/apm/agent/process/ProcessHelper.java @@ -72,8 +72,6 @@ void doStartProcess(@Nonnull AbstractSpan parentContext, @Nonnull Process pro Span span = parentContext.createSpan() .withType("process") - .withSubtype(binaryName) - .withAction("execute") .withName(binaryName); // We don't require span to be activated as the background process is not really linked to current thread diff --git a/apm-agent-plugins/apm-process-plugin/src/test/java/co/elastic/apm/agent/process/ProcessHelperTest.java b/apm-agent-plugins/apm-process-plugin/src/test/java/co/elastic/apm/agent/process/ProcessHelperTest.java index 0d8bf9bbc3..b1b75b4613 100644 --- a/apm-agent-plugins/apm-process-plugin/src/test/java/co/elastic/apm/agent/process/ProcessHelperTest.java +++ b/apm-agent-plugins/apm-process-plugin/src/test/java/co/elastic/apm/agent/process/ProcessHelperTest.java @@ -80,8 +80,8 @@ void checkSpanNaming() { assertThat(span.getNameAsString()).isEqualTo(binaryName); assertThat(span.getType()).isEqualTo("process"); - assertThat(span.getSubtype()).isEqualTo(binaryName); - assertThat(span.getAction()).isEqualTo("execute"); + assertThat(span.getSubtype()).isNull(); + assertThat(span.getAction()).isNull(); assertThat(span.getOutcome()).isEqualTo(Outcome.SUCCESS); } @@ -178,7 +178,7 @@ void destroyWithoutProcessTerminatedShouldEndSpan() { verifyNoMoreInteractions(process); // we should not even use any method of process // we have to opt-in to allow unknown outcome - reporter.checkUnknownOutcome(false); + reporter.disableCheckUnknownOutcome(); helper.doStartProcess(transaction, process, "hello"); diff --git a/apm-agent-plugins/apm-profiling-plugin/src/test/java/co/elastic/apm/agent/profiler/SamplingProfilerTest.java b/apm-agent-plugins/apm-profiling-plugin/src/test/java/co/elastic/apm/agent/profiler/SamplingProfilerTest.java index a9851894d3..f47c9a275d 100644 --- a/apm-agent-plugins/apm-profiling-plugin/src/test/java/co/elastic/apm/agent/profiler/SamplingProfilerTest.java +++ b/apm-agent-plugins/apm-profiling-plugin/src/test/java/co/elastic/apm/agent/profiler/SamplingProfilerTest.java @@ -216,7 +216,7 @@ void testPostProcessingDisabled() throws Exception { } private void aInferred(Transaction transaction) throws Exception { - Span span = transaction.createSpan().withName("bExplicit").withType("test"); + Span span = transaction.createSpan().withName("bExplicit").withType("custom"); try (Scope spanScope = span.activateInScope()) { cInferred(); } finally { diff --git a/apm-agent-plugins/apm-redis-plugin/apm-lettuce-3-tests/src/test/java/co/elastic/apm/agent/redis/lettuce/Lettuce3InstrumentationTest.java b/apm-agent-plugins/apm-redis-plugin/apm-lettuce-3-tests/src/test/java/co/elastic/apm/agent/redis/lettuce/Lettuce3InstrumentationTest.java index 8d174ef289..c9efb9efd6 100644 --- a/apm-agent-plugins/apm-redis-plugin/apm-lettuce-3-tests/src/test/java/co/elastic/apm/agent/redis/lettuce/Lettuce3InstrumentationTest.java +++ b/apm-agent-plugins/apm-redis-plugin/apm-lettuce-3-tests/src/test/java/co/elastic/apm/agent/redis/lettuce/Lettuce3InstrumentationTest.java @@ -44,7 +44,7 @@ public class Lettuce3InstrumentationTest extends AbstractRedisInstrumentationTes public void setUpLettuce() { client = new RedisClient("localhost", redisPort); connection = client.connect(); - reporter.disableDestinationAddressCheck(); + reporter.disableCheckDestinationAddress(); } @Test diff --git a/apm-agent-plugins/apm-redis-plugin/apm-lettuce-plugin/src/test/java/co/elastic/apm/agent/redis/lettuce/Lettuce4InstrumentationTest.java b/apm-agent-plugins/apm-redis-plugin/apm-lettuce-plugin/src/test/java/co/elastic/apm/agent/redis/lettuce/Lettuce4InstrumentationTest.java index d41ea88336..44e401c0f3 100644 --- a/apm-agent-plugins/apm-redis-plugin/apm-lettuce-plugin/src/test/java/co/elastic/apm/agent/redis/lettuce/Lettuce4InstrumentationTest.java +++ b/apm-agent-plugins/apm-redis-plugin/apm-lettuce-plugin/src/test/java/co/elastic/apm/agent/redis/lettuce/Lettuce4InstrumentationTest.java @@ -44,7 +44,7 @@ public class Lettuce4InstrumentationTest extends AbstractRedisInstrumentationTes public void setUpLettuce() { client = RedisClient.create("redis://localhost:" + redisPort); connection = client.connect(); - reporter.disableDestinationAddressCheck(); + reporter.disableCheckDestinationAddress(); } @Test diff --git a/apm-agent-plugins/apm-redis-plugin/apm-lettuce-plugin/src/test/java/co/elastic/apm/agent/redis/lettuce/Lettuce5InstrumentationTest.java b/apm-agent-plugins/apm-redis-plugin/apm-lettuce-plugin/src/test/java/co/elastic/apm/agent/redis/lettuce/Lettuce5InstrumentationTest.java index 8c4a14777a..491dbf4c5e 100644 --- a/apm-agent-plugins/apm-redis-plugin/apm-lettuce-plugin/src/test/java/co/elastic/apm/agent/redis/lettuce/Lettuce5InstrumentationTest.java +++ b/apm-agent-plugins/apm-redis-plugin/apm-lettuce-plugin/src/test/java/co/elastic/apm/agent/redis/lettuce/Lettuce5InstrumentationTest.java @@ -50,7 +50,7 @@ public class Lettuce5InstrumentationTest extends AbstractRedisInstrumentationTes public void setUpLettuce() { RedisClient client = RedisClient.create(RedisURI.create("localhost", redisPort)); connection = client.connect(); - reporter.disableDestinationAddressCheck(); + reporter.disableCheckDestinationAddress(); } @Test diff --git a/apm-agent-plugins/apm-servlet-plugin/src/test/java/co/elastic/apm/agent/servlet/ServletInstrumentationTest.java b/apm-agent-plugins/apm-servlet-plugin/src/test/java/co/elastic/apm/agent/servlet/ServletInstrumentationTest.java index 84540989c9..e18b2a1e54 100644 --- a/apm-agent-plugins/apm-servlet-plugin/src/test/java/co/elastic/apm/agent/servlet/ServletInstrumentationTest.java +++ b/apm-agent-plugins/apm-servlet-plugin/src/test/java/co/elastic/apm/agent/servlet/ServletInstrumentationTest.java @@ -31,6 +31,7 @@ import okhttp3.Response; import org.eclipse.jetty.servlet.ErrorPageErrorHandler; import org.eclipse.jetty.servlet.ServletContextHandler; +import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; import javax.servlet.DispatcherType; @@ -61,6 +62,12 @@ class ServletInstrumentationTest extends AbstractServletTest { + @BeforeEach + void beforeEach() { + // servlet type & subtype are nor part of shared spec + reporter.disableCheckStrictSpanType(); + } + @Override protected void setUpHandler(ServletContextHandler handler) { handler.setDisplayName(getClass().getSimpleName()); diff --git a/integration-tests/external-plugin-test/external-plugin/src/test/java/co/elastic/apm/agent/plugin/PluginInstrumentationTest.java b/integration-tests/external-plugin-test/external-plugin/src/test/java/co/elastic/apm/agent/plugin/PluginInstrumentationTest.java index 49553253fc..3b1c5c2065 100644 --- a/integration-tests/external-plugin-test/external-plugin/src/test/java/co/elastic/apm/agent/plugin/PluginInstrumentationTest.java +++ b/integration-tests/external-plugin-test/external-plugin/src/test/java/co/elastic/apm/agent/plugin/PluginInstrumentationTest.java @@ -25,6 +25,7 @@ package co.elastic.apm.agent.plugin; import co.elastic.apm.agent.AbstractInstrumentationTest; +import co.elastic.apm.agent.impl.transaction.Span; import co.elastic.apm.agent.impl.transaction.Transaction; import co.elastic.apm.plugin.test.TestClass; import org.junit.jupiter.api.Test; @@ -57,6 +58,9 @@ void testTransactionCreation() { @Test void testSpanCreation() { + // using custom span type/sub-type not part of shared spec + reporter.disableCheckStrictSpanType(); + Transaction transaction = tracer.startRootTransaction(null); Objects.requireNonNull(transaction).activate() .withName("Plugin test transaction") @@ -65,7 +69,11 @@ void testSpanCreation() { transaction.deactivate().end(); assertThat(reporter.getTransactions()).hasSize(1); assertThat(reporter.getSpans()).hasSize(1); - assertThat(reporter.getFirstSpan().getNameAsString()).isEqualTo("traceMe"); + Span span = reporter.getFirstSpan(); + assertThat(span.getNameAsString()).isEqualTo("traceMe"); + assertThat(span.getType()).isEqualTo("plugin"); + assertThat(span.getSubtype()).isEqualTo("external"); + assertThat(span.getAction()).isEqualTo("trace"); assertThat(reporter.getErrors()).isEmpty(); } From c45915118fe6d76376a59be57a1ab1c53c454d28 Mon Sep 17 00:00:00 2001 From: Sylvain Juge Date: Wed, 26 May 2021 13:35:34 +0200 Subject: [PATCH 08/25] fix span tests --- .../apm/agent/pluginapi/SpanInstrumentationTest.java | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/apm-agent-plugins/apm-api-plugin/src/test/java/co/elastic/apm/agent/pluginapi/SpanInstrumentationTest.java b/apm-agent-plugins/apm-api-plugin/src/test/java/co/elastic/apm/agent/pluginapi/SpanInstrumentationTest.java index 29438046b4..4c06c198dd 100644 --- a/apm-agent-plugins/apm-api-plugin/src/test/java/co/elastic/apm/agent/pluginapi/SpanInstrumentationTest.java +++ b/apm-agent-plugins/apm-api-plugin/src/test/java/co/elastic/apm/agent/pluginapi/SpanInstrumentationTest.java @@ -67,7 +67,10 @@ void testSetName() { } @Test + @SuppressWarnings("deprecation") void testLegacyAPIs() { + reporter.disableCheckStrictSpanType(); + Span span = transaction.createSpan(); span.setType("foo.bar.baz"); endSpan(span); @@ -79,6 +82,8 @@ void testLegacyAPIs() { @Test void testTypes() { + reporter.disableCheckStrictSpanType(); + Span span = transaction.startSpan("foo", "bar", "baz"); endSpan(span); co.elastic.apm.agent.impl.transaction.Span internalSpan = reporter.getFirstSpan(); @@ -89,6 +94,8 @@ void testTypes() { @Test void testChaining() { + reporter.disableCheckStrictSpanType(); + int randomInt = random.nextInt(); boolean randomBoolean = random.nextBoolean(); String randomString = RandomStringUtils.randomAlphanumeric(3); From 3b84f9cbf91eb28c157fe399fddf69a1f5ae1c73 Mon Sep 17 00:00:00 2001 From: Sylvain Juge Date: Wed, 26 May 2021 14:39:05 +0200 Subject: [PATCH 09/25] normalize span type/subtype/action + fix tests --- .../apm/agent/impl/transaction/Span.java | 17 +++++++++++------ .../report/serialize/DslJsonSerializer.java | 6 +++--- .../apm/agent/impl/transaction/SpanTest.java | 11 ++++++++++- .../pluginapi/CaptureSpanInstrumentation.java | 6 +++--- .../co/elastic/apm/api/AnnotationApiTest.java | 8 ++++++-- 5 files changed, 33 insertions(+), 15 deletions(-) diff --git a/apm-agent-core/src/main/java/co/elastic/apm/agent/impl/transaction/Span.java b/apm-agent-core/src/main/java/co/elastic/apm/agent/impl/transaction/Span.java index b42d2ce5e3..240ea99ca1 100644 --- a/apm-agent-core/src/main/java/co/elastic/apm/agent/impl/transaction/Span.java +++ b/apm-agent-core/src/main/java/co/elastic/apm/agent/impl/transaction/Span.java @@ -157,7 +157,7 @@ public SpanContext getContext() { * Keywords of specific relevance in the span's domain (eg: 'db', 'template', 'ext', etc) */ public Span withType(@Nullable String type) { - this.type = type; + this.type = normalizeEmpty(type); return this; } @@ -165,7 +165,7 @@ public Span withType(@Nullable String type) { * Sets the span's subtype, related to the (eg: 'mysql', 'postgresql', 'jsf' etc) */ public Span withSubtype(@Nullable String subtype) { - this.subtype = subtype; + this.subtype = normalizeEmpty(subtype); return this; } @@ -173,10 +173,15 @@ public Span withSubtype(@Nullable String subtype) { * Action related to this span (eg: 'query', 'render' etc) */ public Span withAction(@Nullable String action) { - this.action = action; + this.action = normalizeEmpty(action); return this; } + @Nullable + private static String normalizeEmpty(@Nullable String value) { + return value == null || value.isEmpty() ? null : value; + } + /** * Sets span.type, span.subtype and span.action. If no subtype and action are provided, assumes the legacy usage of hierarchical * typing system and attempts to split the type by dots to discover subtype and action. @@ -199,9 +204,9 @@ public void setType(@Nullable String type, @Nullable String subtype, @Nullable S } } } - this.type = type; - this.subtype = subtype; - this.action = action; + withType(type); + withSubtype(subtype); + withAction(action); } @Nullable diff --git a/apm-agent-core/src/main/java/co/elastic/apm/agent/report/serialize/DslJsonSerializer.java b/apm-agent-core/src/main/java/co/elastic/apm/agent/report/serialize/DslJsonSerializer.java index f79760c8e5..fbaa81f8a1 100644 --- a/apm-agent-core/src/main/java/co/elastic/apm/agent/report/serialize/DslJsonSerializer.java +++ b/apm-agent-core/src/main/java/co/elastic/apm/agent/report/serialize/DslJsonSerializer.java @@ -705,14 +705,14 @@ private void serializeSpanType(Span span) { replace(replaceBuilder, ".", "_", 0); String subtype = span.getSubtype(); String action = span.getAction(); - if ((subtype != null && !subtype.isEmpty()) || (action != null && !action.isEmpty())) { + if (subtype != null || action != null) { replaceBuilder.append('.'); int replaceStartIndex = replaceBuilder.length() + 1; - if (subtype != null && !subtype.isEmpty()) { + if (subtype != null) { replaceBuilder.append(subtype); replace(replaceBuilder, ".", "_", replaceStartIndex); } - if (action != null && !action.isEmpty()) { + if (action != null) { replaceBuilder.append('.'); replaceStartIndex = replaceBuilder.length() + 1; replaceBuilder.append(action); diff --git a/apm-agent-core/src/test/java/co/elastic/apm/agent/impl/transaction/SpanTest.java b/apm-agent-core/src/test/java/co/elastic/apm/agent/impl/transaction/SpanTest.java index b40d93354b..1dbb408e8b 100644 --- a/apm-agent-core/src/test/java/co/elastic/apm/agent/impl/transaction/SpanTest.java +++ b/apm-agent-core/src/test/java/co/elastic/apm/agent/impl/transaction/SpanTest.java @@ -26,7 +26,6 @@ import co.elastic.apm.agent.MockTracer; import org.junit.jupiter.api.Test; -import org.mockito.Mock; import static org.assertj.core.api.Assertions.assertThat; @@ -69,4 +68,14 @@ void testOutcomeExplicitlyToUnknown() { assertThat(span.getOutcome()).isEqualTo(Outcome.UNKNOWN); } + + @Test + void normalizeEmptyFields() { + Span span = new Span(MockTracer.create()) + .withName("span"); + + assertThat(span.withType("").getType()).isNull(); + assertThat(span.withSubtype("").getSubtype()).isNull(); + assertThat(span.withAction("").getAction()).isNull(); + } } diff --git a/apm-agent-plugins/apm-api-plugin/src/main/java/co/elastic/apm/agent/pluginapi/CaptureSpanInstrumentation.java b/apm-agent-plugins/apm-api-plugin/src/main/java/co/elastic/apm/agent/pluginapi/CaptureSpanInstrumentation.java index cf32e698f9..cfc7b22b80 100644 --- a/apm-agent-plugins/apm-api-plugin/src/main/java/co/elastic/apm/agent/pluginapi/CaptureSpanInstrumentation.java +++ b/apm-agent-plugins/apm-api-plugin/src/main/java/co/elastic/apm/agent/pluginapi/CaptureSpanInstrumentation.java @@ -73,10 +73,10 @@ public static Object onMethodEnter( @Nullable @AnnotationValueOffsetMappingFactory.AnnotationValueExtractor(annotationClassName = "co.elastic.apm.api.CaptureSpan", method = "action") String action) { final AbstractSpan parent = tracer.getActive(); if (parent != null) { - Span span = parent.createSpan(); - span.setType(type, subtype, action); - span.withName(spanName.isEmpty() ? signature : spanName) + Span span = parent.createSpan() + .withName(spanName.isEmpty() ? signature : spanName) .activate(); + span.setType(type, subtype, action); return span; } else { logger.debug("Not creating span for {} because there is no currently active span.", signature); diff --git a/apm-agent-plugins/apm-api-plugin/src/test/java/co/elastic/apm/api/AnnotationApiTest.java b/apm-agent-plugins/apm-api-plugin/src/test/java/co/elastic/apm/api/AnnotationApiTest.java index 90fda2153b..7d21d5c634 100644 --- a/apm-agent-plugins/apm-api-plugin/src/test/java/co/elastic/apm/api/AnnotationApiTest.java +++ b/apm-agent-plugins/apm-api-plugin/src/test/java/co/elastic/apm/api/AnnotationApiTest.java @@ -27,7 +27,6 @@ import co.elastic.apm.agent.AbstractInstrumentationTest; import co.elastic.apm.agent.impl.TracerInternalApiUtils; import co.elastic.apm.agent.impl.transaction.Span; -import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; import static org.assertj.core.api.Assertions.assertThat; @@ -83,6 +82,9 @@ void testType() { } private void testTransactionAndSpanTypes(boolean useLegacyTyping) { + + reporter.disableCheckStrictSpanType(); + reporter.reset(); AnnotationTestClass.transactionWithType(useLegacyTyping); assertThat(reporter.getTransactions()).hasSize(1); @@ -100,12 +102,14 @@ private void testTransactionAndSpanTypes(boolean useLegacyTyping) { @Test void testMissingSubtype() { + reporter.disableCheckStrictSpanType(); + AnnotationTestClass.transactionForMissingSpanSubtype(); assertThat(reporter.getSpans()).hasSize(1); Span internalSpan = reporter.getFirstSpan(); assertThat(internalSpan.getNameAsString()).isEqualTo("spanWithMissingSubtype"); assertThat(internalSpan.getType()).isEqualTo("external.http"); - assertThat(internalSpan.getSubtype()).isEqualTo(""); + assertThat(internalSpan.getSubtype()).isNull(); assertThat(internalSpan.getAction()).isEqualTo("okhttp"); } From 749dd2083b8e71b7d6cc0307fa4abdc4b2e82852 Mon Sep 17 00:00:00 2001 From: Sylvain Juge Date: Wed, 26 May 2021 14:39:35 +0200 Subject: [PATCH 10/25] add aws/azure services to shared spec --- .../test/resources/json-specs/span_types.json | 29 +++++++++++++++++++ 1 file changed, 29 insertions(+) diff --git a/apm-agent-core/src/test/resources/json-specs/span_types.json b/apm-agent-core/src/test/resources/json-specs/span_types.json index 1f2efa5628..9df7d10e32 100644 --- a/apm-agent-core/src/test/resources/json-specs/span_types.json +++ b/apm-agent-core/src/test/resources/json-specs/span_types.json @@ -15,9 +15,15 @@ "cassandra": { "comment": "Cassandra" }, + "cosmosdb": { + "comment": "Azure CosmosDB" + }, "db2": { "comment": "IBM DB2" }, + "dynamodb": { + "comment": "AWS DynamoDB" + }, "elasticsearch": { "comment": "Elasticsearch" }, @@ -65,6 +71,12 @@ }, "messaging": { "subtypes": { + "azurequeue": { + "comment": "Azure Queue" + }, + "azureservicebus": { + "comment": "Azure Service Bus" + }, "jms": { "comment": "Java Messaging Service" }, @@ -73,12 +85,29 @@ }, "rabbitmq": { "comment": "RabbitMQ" + }, + "sns": { + "comment": "AWS Simple Notification Service" + }, + "sqs": { + "comment": "AWS Simple Queue Service" } } }, "process": { "comment": "External process" }, + "storage": { + "azurefile": { + "comment": "Azure Files" + }, + "azuresblob": { + "comment": "Azure Blob Storage" + }, + "s3": { + "comment": "AWS S3" + } + }, "template": { "comment": "Template engines (no sub-type as really platform-specific)" } From 9facbab6cbc4a101013ba7fc8ac3b9112952f815 Mon Sep 17 00:00:00 2001 From: Sylvain Juge Date: Wed, 26 May 2021 15:52:56 +0200 Subject: [PATCH 11/25] fix storage sub-types & remove hibernate --- .../test/resources/json-specs/span_types.json | 21 +++++++++---------- .../HibernateSearch5InstrumentationTest.java | 2 ++ 2 files changed, 12 insertions(+), 11 deletions(-) diff --git a/apm-agent-core/src/test/resources/json-specs/span_types.json b/apm-agent-core/src/test/resources/json-specs/span_types.json index 9df7d10e32..0cb0fe9fb8 100644 --- a/apm-agent-core/src/test/resources/json-specs/span_types.json +++ b/apm-agent-core/src/test/resources/json-specs/span_types.json @@ -30,9 +30,6 @@ "h2": { "comment": "H2" }, - "hibernate-search": { - "comment": "Hibernate Search (Java only)" - }, "mariadb": { "comment": "MariaDB" }, @@ -98,14 +95,16 @@ "comment": "External process" }, "storage": { - "azurefile": { - "comment": "Azure Files" - }, - "azuresblob": { - "comment": "Azure Blob Storage" - }, - "s3": { - "comment": "AWS S3" + "subtypes": { + "azurefile": { + "comment": "Azure Files" + }, + "azuresblob": { + "comment": "Azure Blob Storage" + }, + "s3": { + "comment": "AWS S3" + } } }, "template": { diff --git a/apm-agent-plugins/apm-hibernate-search-plugin/apm-hibernate-search-plugin-5_x/src/test/java/co/elastic/apm/agent/hibernatesearch/HibernateSearch5InstrumentationTest.java b/apm-agent-plugins/apm-hibernate-search-plugin/apm-hibernate-search-plugin-5_x/src/test/java/co/elastic/apm/agent/hibernatesearch/HibernateSearch5InstrumentationTest.java index 986f44797b..6fbca3325e 100644 --- a/apm-agent-plugins/apm-hibernate-search-plugin/apm-hibernate-search-plugin-5_x/src/test/java/co/elastic/apm/agent/hibernatesearch/HibernateSearch5InstrumentationTest.java +++ b/apm-agent-plugins/apm-hibernate-search-plugin/apm-hibernate-search-plugin-5_x/src/test/java/co/elastic/apm/agent/hibernatesearch/HibernateSearch5InstrumentationTest.java @@ -74,6 +74,8 @@ static void setUp() throws IOException { @BeforeEach void initSingleTest() { + // hibernate is not in the shared span type/subtype specification + reporter.disableCheckStrictSpanType(); tracer.startRootTransaction(null).activate(); } From 0ef9b33207f071699ff61cf9ccc51340e3ca1521 Mon Sep 17 00:00:00 2001 From: Sylvain Juge Date: Wed, 26 May 2021 16:08:26 +0200 Subject: [PATCH 12/25] make rabbitmq tests fit spec --- .../src/test/java/co/elastic/apm/agent/MockReporter.java | 9 +++++---- .../elastic/apm/agent/rabbitmq/AbstractRabbitMqTest.java | 3 ++- .../apm/agent/rabbitmq/config/BaseConfiguration.java | 2 +- 3 files changed, 8 insertions(+), 6 deletions(-) diff --git a/apm-agent-core/src/test/java/co/elastic/apm/agent/MockReporter.java b/apm-agent-core/src/test/java/co/elastic/apm/agent/MockReporter.java index fafdeaf7eb..f2128d67e9 100644 --- a/apm-agent-core/src/test/java/co/elastic/apm/agent/MockReporter.java +++ b/apm-agent-core/src/test/java/co/elastic/apm/agent/MockReporter.java @@ -225,10 +225,11 @@ private void verifySpanType(Span span) { String subType = span.getSubtype(); if (subType != null) { - JsonNode subTypesJson = getMandatoryJson(typeJson, "subtypes", String.format("subtypes for type '%s'", type)); - - JsonNode subTypeJson = getMandatoryJson(subTypesJson, subType, String.format("span subtype '%s' for type '%s' (%s)", subType, type, typeComment)); - assertThat(subTypeJson).isNotNull(); + JsonNode subTypesJson = typeJson.get("subtypes"); + if (null != subTypesJson) { + JsonNode subTypeJson = getMandatoryJson(subTypesJson, subType, String.format("span subtype '%s' for type '%s' (%s)", subType, type, typeComment)); + assertThat(subTypeJson).isNotNull(); + } } } diff --git a/apm-agent-plugins/apm-rabbitmq/apm-rabbitmq-spring/src/test/java/co/elastic/apm/agent/rabbitmq/AbstractRabbitMqTest.java b/apm-agent-plugins/apm-rabbitmq/apm-rabbitmq-spring/src/test/java/co/elastic/apm/agent/rabbitmq/AbstractRabbitMqTest.java index 3534d09cf7..0af12bc4e2 100644 --- a/apm-agent-plugins/apm-rabbitmq/apm-rabbitmq-spring/src/test/java/co/elastic/apm/agent/rabbitmq/AbstractRabbitMqTest.java +++ b/apm-agent-plugins/apm-rabbitmq/apm-rabbitmq-spring/src/test/java/co/elastic/apm/agent/rabbitmq/AbstractRabbitMqTest.java @@ -48,6 +48,7 @@ public void verifyThatTransactionWithSpanCreated() { checkTransaction(receiveTransaction, TOPIC_EXCHANGE_NAME, "Spring AMQP"); Span testSpan = reporter.getFirstSpan(1000); assertThat(testSpan.getNameAsString()).isEqualTo("testSpan"); + assertThat(testSpan.getType()).isEqualTo("custom"); checkParentChild(receiveTransaction, testSpan); } @@ -69,7 +70,7 @@ public void verifyThatTransactionWithSpanCreated_DistributedTracing() { checkSendSpan(sendSpan, TOPIC_EXCHANGE_NAME, LOCALHOST_ADDRESS, container.getAmqpPort()); checkParentChild(sendSpan, receiveTransaction); - Span testSpan = spans.stream().filter(span -> span.getType().equals("http")).findFirst().get(); + Span testSpan = spans.stream().filter(span -> span.getType().equals("custom")).findFirst().get(); assertThat(testSpan.getNameAsString()).isEqualTo("testSpan"); checkParentChild(receiveTransaction, testSpan); } diff --git a/apm-agent-plugins/apm-rabbitmq/apm-rabbitmq-spring/src/test/java/co/elastic/apm/agent/rabbitmq/config/BaseConfiguration.java b/apm-agent-plugins/apm-rabbitmq/apm-rabbitmq-spring/src/test/java/co/elastic/apm/agent/rabbitmq/config/BaseConfiguration.java index 7fbc84da06..29d1f7c628 100644 --- a/apm-agent-plugins/apm-rabbitmq/apm-rabbitmq-spring/src/test/java/co/elastic/apm/agent/rabbitmq/config/BaseConfiguration.java +++ b/apm-agent-plugins/apm-rabbitmq/apm-rabbitmq-spring/src/test/java/co/elastic/apm/agent/rabbitmq/config/BaseConfiguration.java @@ -69,7 +69,7 @@ public RabbitTemplate rabbitTemplate(final ConnectionFactory connectionFactory) return rabbitTemplate; } - @CaptureSpan(value = "testSpan", type = "http", subtype = "get", action = "test") + @CaptureSpan(value = "testSpan", type = "custom", subtype = "anything", action = "test") public void testSpan() { } } From 94ecb24354efe614fe859c84293f29eb52fd6bb8 Mon Sep 17 00:00:00 2001 From: Sylvain Juge Date: Wed, 26 May 2021 20:55:53 +0200 Subject: [PATCH 13/25] disable span type check for hibernate-search 6.x --- .../hibernatesearch/HibernateSearch6InstrumentationTest.java | 3 +++ 1 file changed, 3 insertions(+) diff --git a/apm-agent-plugins/apm-hibernate-search-plugin/apm-hibernate-search-plugin-6_x/src/test/java/co/elastic/apm/agent/hibernatesearch/HibernateSearch6InstrumentationTest.java b/apm-agent-plugins/apm-hibernate-search-plugin/apm-hibernate-search-plugin-6_x/src/test/java/co/elastic/apm/agent/hibernatesearch/HibernateSearch6InstrumentationTest.java index 16bf99ae6b..889c0ccc5b 100644 --- a/apm-agent-plugins/apm-hibernate-search-plugin/apm-hibernate-search-plugin-6_x/src/test/java/co/elastic/apm/agent/hibernatesearch/HibernateSearch6InstrumentationTest.java +++ b/apm-agent-plugins/apm-hibernate-search-plugin/apm-hibernate-search-plugin-6_x/src/test/java/co/elastic/apm/agent/hibernatesearch/HibernateSearch6InstrumentationTest.java @@ -67,6 +67,9 @@ static void setUp() throws IOException { @BeforeEach void initSingleTest() { + // hibernate search is not part of shared spec + reporter.disableCheckStrictSpanType(); + tracer.startRootTransaction(null).activate(); } From ae625207aaf22d35ff06cfc04a02ea93275c2d41 Mon Sep 17 00:00:00 2001 From: Sylvain Juge Date: Thu, 27 May 2021 15:25:24 +0200 Subject: [PATCH 14/25] make opentracing bridge fit spec --- .../ApmSpanInstrumentation.java | 32 ++++++------------- 1 file changed, 9 insertions(+), 23 deletions(-) diff --git a/apm-agent-plugins/apm-opentracing-plugin/src/main/java/co/elastic/apm/agent/opentracingimpl/ApmSpanInstrumentation.java b/apm-agent-plugins/apm-opentracing-plugin/src/main/java/co/elastic/apm/agent/opentracingimpl/ApmSpanInstrumentation.java index 60c6a1c267..8452d1313f 100644 --- a/apm-agent-plugins/apm-opentracing-plugin/src/main/java/co/elastic/apm/agent/opentracingimpl/ApmSpanInstrumentation.java +++ b/apm-agent-plugins/apm-opentracing-plugin/src/main/java/co/elastic/apm/agent/opentracingimpl/ApmSpanInstrumentation.java @@ -71,7 +71,7 @@ public FinishInstrumentation() { @Advice.OnMethodEnter(suppress = Throwable.class, inline = false) public static void finishInternal(@Advice.FieldValue(value = "dispatcher", typing = Assigner.Typing.DYNAMIC) @Nullable Object context, - @Advice.Argument(0) long finishMicros) { + @Advice.Argument(0) long finishMicros) { if (context instanceof AbstractSpan) { doFinishInternal((AbstractSpan) context, finishMicros); } @@ -84,21 +84,14 @@ public static void doFinishInternal(AbstractSpan abstractSpan, long finishMic if (transaction.getType() == null) { if (transaction.getContext().getRequest().hasContent()) { transaction.withType(Transaction.TYPE_REQUEST); - } else { - transaction.withType("unknown"); } } - } else { - Span span = (Span) abstractSpan; - if (span.getType() == null) { - span.withType("unknown"); - } - } - if (finishMicros >= 0) { - abstractSpan.end(finishMicros); - } else { - abstractSpan.end(); + if (finishMicros >= 0) { + abstractSpan.end(finishMicros); + } else { + abstractSpan.end(); + } } } } @@ -256,11 +249,7 @@ private static boolean handleSpecialSpanTag(Span span, String key, Object value) return true; } else if ("db.type".equals(key)) { span.getContext().getDb().withType(value.toString()); - if (isCache(value)) { - span.withType("cache").withSubtype(value.toString()); - } else { - span.withType("db").withSubtype(value.toString()); - } + span.withType("db").withSubtype(value.toString()); return true; } else if ("db.instance".equals(key)) { span.getContext().getDb().withInstance(value.toString()); @@ -271,11 +260,11 @@ private static boolean handleSpecialSpanTag(Span span, String key, Object value) return true; } else if ("span.kind".equals(key)) { if (span.getType() == null && ("producer".equals(value) || "client".equals(value))) { - span.withType("ext"); + span.withType("external"); } return true; } else if ("http.status_code".equals(key) && value instanceof Number) { - int status =((Number) value).intValue(); + int status = ((Number) value).intValue(); span.getContext().getHttp().withStatusCode(status); span.withOutcome(ResultUtil.getOutcomeByHttpClientStatus(status)); return true; @@ -289,9 +278,6 @@ private static boolean handleSpecialSpanTag(Span span, String key, Object value) return false; } - private static boolean isCache(Object dbType) { - return "redis".equals(dbType); - } } public static class GetTraceContextInstrumentation extends ApmSpanInstrumentation { From bc1a8632f7d3302d374a2639ebb5b1564ff4d9e9 Mon Sep 17 00:00:00 2001 From: Sylvain Juge Date: Thu, 27 May 2021 16:40:26 +0200 Subject: [PATCH 15/25] make opentracing fit the spec --- .../agent/opentracingimpl/ApmSpanInstrumentation.java | 11 +++++------ .../apm/opentracing/OpenTracingBridgeTest.java | 10 +++++----- 2 files changed, 10 insertions(+), 11 deletions(-) diff --git a/apm-agent-plugins/apm-opentracing-plugin/src/main/java/co/elastic/apm/agent/opentracingimpl/ApmSpanInstrumentation.java b/apm-agent-plugins/apm-opentracing-plugin/src/main/java/co/elastic/apm/agent/opentracingimpl/ApmSpanInstrumentation.java index 8452d1313f..2033943923 100644 --- a/apm-agent-plugins/apm-opentracing-plugin/src/main/java/co/elastic/apm/agent/opentracingimpl/ApmSpanInstrumentation.java +++ b/apm-agent-plugins/apm-opentracing-plugin/src/main/java/co/elastic/apm/agent/opentracingimpl/ApmSpanInstrumentation.java @@ -86,12 +86,11 @@ public static void doFinishInternal(AbstractSpan abstractSpan, long finishMic transaction.withType(Transaction.TYPE_REQUEST); } } - - if (finishMicros >= 0) { - abstractSpan.end(finishMicros); - } else { - abstractSpan.end(); - } + } + if (finishMicros >= 0) { + abstractSpan.end(finishMicros); + } else { + abstractSpan.end(); } } } diff --git a/apm-opentracing/src/test/java/co/elastic/apm/opentracing/OpenTracingBridgeTest.java b/apm-opentracing/src/test/java/co/elastic/apm/opentracing/OpenTracingBridgeTest.java index 68ab7552b1..575d887e94 100644 --- a/apm-opentracing/src/test/java/co/elastic/apm/opentracing/OpenTracingBridgeTest.java +++ b/apm-opentracing/src/test/java/co/elastic/apm/opentracing/OpenTracingBridgeTest.java @@ -301,11 +301,11 @@ public void testAgentPaused() { @Test void testResolveClientType() { assertSoftly(softly -> { - softly.assertThat(createSpanFromOtTags(Map.of("span.kind", "client")).getType()).isEqualTo("ext"); - softly.assertThat(createSpanFromOtTags(Map.of("span.kind", "producer")).getType()).isEqualTo("ext"); + softly.assertThat(createSpanFromOtTags(Map.of("span.kind", "client")).getType()).isEqualTo("external"); + softly.assertThat(createSpanFromOtTags(Map.of("span.kind", "producer")).getType()).isEqualTo("external"); softly.assertThat(createSpanFromOtTags(Map.of("span.kind", "client", "db.type", "mysql")).getType()).isEqualTo("db"); - softly.assertThat(createSpanFromOtTags(Map.of("span.kind", "client", "db.type", "foo")).getType()).isEqualTo("db"); - softly.assertThat(createSpanFromOtTags(Map.of("span.kind", "client", "db.type", "redis")).getType()).isEqualTo("cache"); + softly.assertThat(createSpanFromOtTags(Map.of("span.kind", "client", "db.type", "sqlserver")).getType()).isEqualTo("db"); + softly.assertThat(createSpanFromOtTags(Map.of("span.kind", "client", "db.type", "redis")).getType()).isEqualTo("db"); }); } @@ -330,7 +330,7 @@ void testTypeTags() { @Test void testResolveServerType() { assertSoftly(softly -> { - softly.assertThat(createTransactionFromOtTags(Map.of("span.kind", "server")).getType()).isEqualTo("unknown"); + softly.assertThat(createTransactionFromOtTags(Map.of("span.kind", "server")).getType()).isEqualTo("custom"); softly.assertThat(createTransactionFromOtTags(Map.of("span.kind", "server", "http.url", "http://localhost:8080", "http.method", "GET")).getType()).isEqualTo("request"); From 6d1b48f63426aef1349ef01f476864a257c7c699 Mon Sep 17 00:00:00 2001 From: Sylvain Juge Date: Thu, 27 May 2021 20:26:40 +0200 Subject: [PATCH 16/25] fix integration test for process spans --- .../java/co/elastic/apm/servlet/tests/ServletApiTestApp.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/integration-tests/application-server-integration-tests/src/test/java/co/elastic/apm/servlet/tests/ServletApiTestApp.java b/integration-tests/application-server-integration-tests/src/test/java/co/elastic/apm/servlet/tests/ServletApiTestApp.java index 9bdda805a8..5fa05b5c0c 100644 --- a/integration-tests/application-server-integration-tests/src/test/java/co/elastic/apm/servlet/tests/ServletApiTestApp.java +++ b/integration-tests/application-server-integration-tests/src/test/java/co/elastic/apm/servlet/tests/ServletApiTestApp.java @@ -124,7 +124,7 @@ private void testExecuteCommand(AbstractServletContainerIntegrationTest test, St for (JsonNode span : spans) { assertThat(span.get("parent_id").textValue()).isEqualTo(transactionId); assertThat(span.get("name").asText()).isEqualTo("java"); - assertThat(span.get("type").asText()).isEqualTo("process.java.execute"); + assertThat(span.get("type").asText()).isEqualTo("process"); } } From 74e5bce01bbdc1bf2c909fa289ea933276b22c82 Mon Sep 17 00:00:00 2001 From: Sylvain Juge Date: Tue, 1 Jun 2021 14:26:46 +0200 Subject: [PATCH 17/25] make subtypes less optional --- .../co/elastic/apm/agent/MockReporter.java | 39 ++++++++++++------ .../apm/agent/impl/SpanTypeBreakdownTest.java | 13 +++++- .../test/resources/json-specs/span_types.json | 17 +++++++- .../api/ElasticApmApiInstrumentationTest.java | 2 +- .../apm/agent/jdbc/helper/JdbcHelper.java | 10 +++-- .../jdbc/AbstractJdbcInstrumentationTest.java | 6 ++- .../jdbc/helper/ConnectionMetaDataTest.java | 40 +++++++++++++++++-- 7 files changed, 100 insertions(+), 27 deletions(-) diff --git a/apm-agent-core/src/test/java/co/elastic/apm/agent/MockReporter.java b/apm-agent-core/src/test/java/co/elastic/apm/agent/MockReporter.java index f2128d67e9..156f7f3128 100644 --- a/apm-agent-core/src/test/java/co/elastic/apm/agent/MockReporter.java +++ b/apm-agent-core/src/test/java/co/elastic/apm/agent/MockReporter.java @@ -150,8 +150,8 @@ public void disableCheckDestinationAddress() { checkDestinationAddress = false; } - public boolean checkDestinationAddress(){ - return checkDestinationAddress; + public boolean checkDestinationAddress() { + return checkDestinationAddress; } /** @@ -162,7 +162,8 @@ public void disableCheckStrictSpanType() { } @Override - public void start() {} + public void start() { + } @Override public synchronized void report(Transaction transaction) { @@ -194,12 +195,9 @@ public synchronized void report(Span span) { verifySpanType(span); verifyDestinationFields(span); - String type = span.getType(); - assertThat(type).isNotNull(); - if (checkUnknownOutcomes) { assertThat(span.getOutcome()) - .describedAs("span outcome should be either success or failure for type = %s", type) + .describedAs("span outcome should be either success or failure for type = %s", span.getType()) .isNotEqualTo(Outcome.UNKNOWN); } } catch (Exception e) { @@ -219,18 +217,33 @@ private void verifySpanType(Span span) { .isNotNull(); if (checkStrictSpanType) { - JsonNode typeJson = getMandatoryJson(SPAN_TYPES_SPEC, type, String.format("span type '%s'", type)); + JsonNode typeJson = getMandatoryJson(SPAN_TYPES_SPEC, type, String.format("span type '%s' is not allowed by the spec", type)); String typeComment = getOptionalComment(typeJson); + JsonNode jsonOptionalSubtype = typeJson.get("optional_subtype"); + boolean optionalSubtype = jsonOptionalSubtype != null && jsonOptionalSubtype.isBoolean() && jsonOptionalSubtype.asBoolean(); + String subType = span.getSubtype(); - if (subType != null) { + + if (subType == null) { + // span does not have a sub-type, make sure that it's only when spec allows for it + assertThat(optionalSubtype) + .describedAs("span type '%s' subtype is not optional by the spec (optional_subtype=false)", type) + .isTrue(); + } else { + assertThat(subType) + .describedAs("span subtype is required by the spec for type '%s'", type) + .isNotNull(); + + // we have a sub-type, make sure that the sub-type matches the spec JsonNode subTypesJson = typeJson.get("subtypes"); - if (null != subTypesJson) { - JsonNode subTypeJson = getMandatoryJson(subTypesJson, subType, String.format("span subtype '%s' for type '%s' (%s)", subType, type, typeComment)); - assertThat(subTypeJson).isNotNull(); + if (subTypesJson != null) { + + getMandatoryJson(subTypesJson, subType, String.format("span subtype '%s' is now allowed by the spec for type '%s' (%s)", subType, type, typeComment)); } } + } } @@ -477,7 +490,7 @@ public synchronized void assertRecycledAfterDecrementingReferences() { List spans = getSpans(); List spansToFlush = spans.stream() - .filter(s-> !hasEmptyTraceContext(s)) + .filter(s -> !hasEmptyTraceContext(s)) .collect(Collectors.toList()); transactionsToFlush.forEach(Transaction::decrementReferences); diff --git a/apm-agent-core/src/test/java/co/elastic/apm/agent/impl/SpanTypeBreakdownTest.java b/apm-agent-core/src/test/java/co/elastic/apm/agent/impl/SpanTypeBreakdownTest.java index 688b31b231..53dd482fc7 100644 --- a/apm-agent-core/src/test/java/co/elastic/apm/agent/impl/SpanTypeBreakdownTest.java +++ b/apm-agent-core/src/test/java/co/elastic/apm/agent/impl/SpanTypeBreakdownTest.java @@ -377,7 +377,12 @@ void testBreakdown_spanStartedAfterParentEnded() { } private void assertThatTransactionBreakdownCounterCreated(Map metricSets) { - assertThat(metricSets.get(Labels.Mutable.of().transactionName("test").transactionType("request")).getCounters().get("transaction.breakdown.count").get()).isEqualTo(1); + assertThat(metricSets.get(Labels.Mutable.of() + .transactionName("test") + .transactionType("request")) + .getCounters().get("transaction.breakdown.count") + .get()) + .isEqualTo(1); } private Transaction createTransaction() { @@ -388,7 +393,11 @@ private Transaction createTransaction() { @Nullable private Timer getTimer(Map metricSets, String timerName, @Nullable String spanType, @Nullable String spanSubType) { - final MetricSet metricSet = metricSets.get(Labels.Mutable.of().transactionName("test").transactionType("request").spanType(spanType).spanSubType(spanSubType)); + final MetricSet metricSet = metricSets.get(Labels.Mutable.of() + .transactionName("test") + .transactionType("request") + .spanType(spanType) + .spanSubType(spanSubType)); if (metricSet == null) { return null; } diff --git a/apm-agent-core/src/test/resources/json-specs/span_types.json b/apm-agent-core/src/test/resources/json-specs/span_types.json index 0cb0fe9fb8..4c762f9a2f 100644 --- a/apm-agent-core/src/test/resources/json-specs/span_types.json +++ b/apm-agent-core/src/test/resources/json-specs/span_types.json @@ -1,5 +1,6 @@ { "app": { + "optional_subtype": true, "subtypes": { "inferred": { "comment": "Sampling profiler inferred spans" @@ -7,6 +8,7 @@ } }, "custom": { + "optional_subtype": true, "comment": "API custom instrumentation" }, "db": { @@ -21,6 +23,9 @@ "db2": { "comment": "IBM DB2" }, + "derby": { + "comment": "Apache Derby" + }, "dynamodb": { "comment": "AWS DynamoDB" }, @@ -30,6 +35,12 @@ "h2": { "comment": "H2" }, + "hsqldb": { + "comment": "HSQLDB" + }, + "ingres": { + "comment": "Ingres" + }, "mariadb": { "comment": "MariaDB" }, @@ -50,6 +61,9 @@ }, "sqlserver": { "comment": "Microsoft SQL Server" + }, + "unknown": { + "comment": "Unknown database" } } }, @@ -67,6 +81,7 @@ } }, "messaging": { + "comment": "Messaging", "subtypes": { "azurequeue": { "comment": "Azure Queue" @@ -108,7 +123,7 @@ } }, "template": { - "comment": "Template engines (no sub-type as really platform-specific)" + "comment": "Template engines (no sub-type for now as really platform-specific)" } } diff --git a/apm-agent-plugins/apm-api-plugin/src/test/java/co/elastic/apm/api/ElasticApmApiInstrumentationTest.java b/apm-agent-plugins/apm-api-plugin/src/test/java/co/elastic/apm/api/ElasticApmApiInstrumentationTest.java index 54ddd574e8..b409d7071c 100644 --- a/apm-agent-plugins/apm-api-plugin/src/test/java/co/elastic/apm/api/ElasticApmApiInstrumentationTest.java +++ b/apm-agent-plugins/apm-api-plugin/src/test/java/co/elastic/apm/api/ElasticApmApiInstrumentationTest.java @@ -157,7 +157,7 @@ void testGetId_distributedTracingEnabled() { assertThat(ElasticApm.currentTransaction().getTraceId()).isEqualTo(transaction.getTraceContext().getTraceId().toString()); assertThat(ElasticApm.currentSpan().getId()).isEqualTo(transaction.getTraceContext().getId().toString()); assertThat(ElasticApm.currentSpan().getTraceId()).isEqualTo(transaction.getTraceContext().getTraceId().toString()); - co.elastic.apm.agent.impl.transaction.Span span = transaction.createSpan().withType("db").withName("SELECT"); + co.elastic.apm.agent.impl.transaction.Span span = transaction.createSpan().withType("db").withSubtype("mysql").withName("SELECT"); try (Scope spanScope = span.activateInScope()) { assertThat(ElasticApm.currentSpan().getId()).isEqualTo(span.getTraceContext().getId().toString()); assertThat(ElasticApm.currentSpan().getTraceId()).isEqualTo(span.getTraceContext().getTraceId().toString()); diff --git a/apm-agent-plugins/apm-jdbc-plugin/src/main/java/co/elastic/apm/agent/jdbc/helper/JdbcHelper.java b/apm-agent-plugins/apm-jdbc-plugin/src/main/java/co/elastic/apm/agent/jdbc/helper/JdbcHelper.java index b0dde1cada..645cb68c71 100644 --- a/apm-agent-plugins/apm-jdbc-plugin/src/main/java/co/elastic/apm/agent/jdbc/helper/JdbcHelper.java +++ b/apm-agent-plugins/apm-jdbc-plugin/src/main/java/co/elastic/apm/agent/jdbc/helper/JdbcHelper.java @@ -117,19 +117,21 @@ public Span createJdbcSpan(@Nullable String sql, Object statement, @Nullable Abs Connection connection = safeGetConnection((Statement) statement); ConnectionMetaData connectionMetaData = connection == null ? null : getConnectionMetaData(connection); + + String vendor = "unknown"; if (connectionMetaData != null) { - span.withSubtype(connectionMetaData.getDbVendor()) - .withAction(DB_SPAN_ACTION); + vendor = connectionMetaData.getDbVendor();; span.getContext().getDb() .withUser(connectionMetaData.getUser()); Destination destination = span.getContext().getDestination() .withAddress(connectionMetaData.getHost()) .withPort(connectionMetaData.getPort()); destination.getService() - .withName(connectionMetaData.getDbVendor()) - .withResource(connectionMetaData.getDbVendor()) + .withName(vendor) + .withResource(vendor) .withType(DB_SPAN_TYPE); } + span.withSubtype(vendor).withAction(DB_SPAN_ACTION); return span; } diff --git a/apm-agent-plugins/apm-jdbc-plugin/src/test/java/co/elastic/apm/agent/jdbc/AbstractJdbcInstrumentationTest.java b/apm-agent-plugins/apm-jdbc-plugin/src/test/java/co/elastic/apm/agent/jdbc/AbstractJdbcInstrumentationTest.java index 6bf902964d..d6d1fa91b9 100644 --- a/apm-agent-plugins/apm-jdbc-plugin/src/test/java/co/elastic/apm/agent/jdbc/AbstractJdbcInstrumentationTest.java +++ b/apm-agent-plugins/apm-jdbc-plugin/src/test/java/co/elastic/apm/agent/jdbc/AbstractJdbcInstrumentationTest.java @@ -223,6 +223,7 @@ private void checkWithoutConnectionMetadata(TestStatement statement, ThrownCount final String sql = "UPDATE ELASTIC_APM SET BAR='AFTER1' WHERE FOO=11"; boolean isResultSet = statement.execute(sql); + assertThat(check.getThrownCount()).isEqualTo(1); assertThat(isResultSet).isFalse(); @@ -230,6 +231,7 @@ private void checkWithoutConnectionMetadata(TestStatement statement, ThrownCount // try to execute statement again, should not throw again statement.execute(sql); + assertThat(check.getThrownCount()) .describedAs("unsupported exception should only be thrown once") .isEqualTo(1); @@ -442,8 +444,8 @@ private void assertSpanRecordedWithoutConnection(String rawSql, boolean prepared signatureParser.querySignature(rawSql, processedSql, preparedStatement); assertThat(jdbcSpan.getNameAsString()).isEqualTo(processedSql.toString()); assertThat(jdbcSpan.getType()).isEqualTo(DB_SPAN_TYPE); - assertThat(jdbcSpan.getSubtype()).isNull(); - assertThat(jdbcSpan.getAction()).isNull(); + assertThat(jdbcSpan.getSubtype()).isEqualTo("unknown"); + assertThat(jdbcSpan.getAction()).isEqualTo(DB_SPAN_ACTION); Db db = jdbcSpan.getContext().getDb(); assertThat(db.getStatement()).isEqualTo(rawSql); diff --git a/apm-agent-plugins/apm-jdbc-plugin/src/test/java/co/elastic/apm/agent/jdbc/helper/ConnectionMetaDataTest.java b/apm-agent-plugins/apm-jdbc-plugin/src/test/java/co/elastic/apm/agent/jdbc/helper/ConnectionMetaDataTest.java index 0957fc7fcc..5fcebe7920 100644 --- a/apm-agent-plugins/apm-jdbc-plugin/src/test/java/co/elastic/apm/agent/jdbc/helper/ConnectionMetaDataTest.java +++ b/apm-agent-plugins/apm-jdbc-plugin/src/test/java/co/elastic/apm/agent/jdbc/helper/ConnectionMetaDataTest.java @@ -24,11 +24,15 @@ */ package co.elastic.apm.agent.jdbc.helper; +import co.elastic.apm.agent.MockReporter; +import co.elastic.apm.agent.MockTracer; +import co.elastic.apm.agent.impl.ElasticApmTracer; +import co.elastic.apm.agent.impl.transaction.Transaction; import org.junit.jupiter.api.Test; import javax.annotation.Nullable; -import static org.junit.jupiter.api.Assertions.*; +import static org.assertj.core.api.Assertions.assertThat; class ConnectionMetaDataTest { @@ -317,10 +321,38 @@ void testInvalid() { testUrl("postgresql://myhost:666/database", "unknown", null, -1); } + private static final MockTracer.MockInstrumentationSetup mockSetup = MockTracer.createMockInstrumentationSetup(); + private static MockReporter reporter = mockSetup.getReporter(); + private static ElasticApmTracer tracer = mockSetup.getTracer(); + private void testUrl(String url, String expectedVendor, @Nullable String expectedHost, int expectedPort) { ConnectionMetaData metadata = ConnectionMetaData.create(url, "TEST_USER"); - assertEquals(metadata.getDbVendor(), expectedVendor); - assertEquals(metadata.getHost(), expectedHost); - assertEquals(metadata.getPort(), expectedPort); + assertThat(metadata.getDbVendor()).isEqualTo(expectedVendor); + assertThat(metadata.getHost()).isEqualTo(expectedHost); + assertThat(metadata.getPort()).isEqualTo(expectedPort); + + if ("arbitrary".equals(expectedVendor)) { + // known case where the jdbc url is valid, but has an unknwon value in shared spec + return; + } + + // try to create a DB span in order to trigger shared span type validation logic + // given not all JDBC urls listed here will be explicitly tested with a real database, that allows + // to make sure that at least the known ones always fit + try { + + Transaction transaction = tracer.startRootTransaction(this.getClass().getClassLoader()); + assertThat(transaction).isNotNull(); + transaction.createSpan() + .withType("db") + .withSubtype(metadata.getDbVendor()).end(); + transaction.end(); + + assertThat(reporter.getNumReportedSpans()).isEqualTo(1); + assertThat(reporter.getNumReportedTransactions()).isEqualTo(1); + } finally { + reporter.reset(); + } + } } From 1daef8b602fdfa31a12572a7df8509a68ccdb98c Mon Sep 17 00:00:00 2001 From: Sylvain Juge Date: Tue, 1 Jun 2021 15:12:57 +0200 Subject: [PATCH 18/25] enforce null subtype when no subtype attribute in spec --- .../java/co/elastic/apm/agent/MockReporter.java | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/apm-agent-core/src/test/java/co/elastic/apm/agent/MockReporter.java b/apm-agent-core/src/test/java/co/elastic/apm/agent/MockReporter.java index 156f7f3128..ee724fff8b 100644 --- a/apm-agent-core/src/test/java/co/elastic/apm/agent/MockReporter.java +++ b/apm-agent-core/src/test/java/co/elastic/apm/agent/MockReporter.java @@ -226,7 +226,13 @@ private void verifySpanType(Span span) { String subType = span.getSubtype(); - if (subType == null) { + JsonNode subTypesJson = typeJson.get("subtypes"); + + if (subTypesJson == null) { + assertThat(subType) + .describedAs("span type '%s' does not allow for subtypes", type) + .isNull(); + } else if (subType == null) { // span does not have a sub-type, make sure that it's only when spec allows for it assertThat(optionalSubtype) .describedAs("span type '%s' subtype is not optional by the spec (optional_subtype=false)", type) @@ -237,11 +243,7 @@ private void verifySpanType(Span span) { .isNotNull(); // we have a sub-type, make sure that the sub-type matches the spec - JsonNode subTypesJson = typeJson.get("subtypes"); - if (subTypesJson != null) { - - getMandatoryJson(subTypesJson, subType, String.format("span subtype '%s' is now allowed by the spec for type '%s' (%s)", subType, type, typeComment)); - } + getMandatoryJson(subTypesJson, subType, String.format("span subtype '%s' is now allowed by the spec for type '%s' (%s)", subType, type, typeComment)); } } From 9802357000aa1f414f65d33edc41f2a5ca252d1a Mon Sep 17 00:00:00 2001 From: Sylvain Juge Date: Tue, 1 Jun 2021 15:27:50 +0200 Subject: [PATCH 19/25] add description to json spec --- .../src/test/java/co/elastic/apm/agent/MockReporter.java | 2 +- .../src/test/resources/json-specs/span_types.json | 8 ++++++++ 2 files changed, 9 insertions(+), 1 deletion(-) diff --git a/apm-agent-core/src/test/java/co/elastic/apm/agent/MockReporter.java b/apm-agent-core/src/test/java/co/elastic/apm/agent/MockReporter.java index ee724fff8b..0bc3843953 100644 --- a/apm-agent-core/src/test/java/co/elastic/apm/agent/MockReporter.java +++ b/apm-agent-core/src/test/java/co/elastic/apm/agent/MockReporter.java @@ -228,7 +228,7 @@ private void verifySpanType(Span span) { JsonNode subTypesJson = typeJson.get("subtypes"); - if (subTypesJson == null) { + if (subTypesJson == null || subTypesJson.isEmpty()) { assertThat(subType) .describedAs("span type '%s' does not allow for subtypes", type) .isNull(); diff --git a/apm-agent-core/src/test/resources/json-specs/span_types.json b/apm-agent-core/src/test/resources/json-specs/span_types.json index 4c762f9a2f..a90d9aac54 100644 --- a/apm-agent-core/src/test/resources/json-specs/span_types.json +++ b/apm-agent-core/src/test/resources/json-specs/span_types.json @@ -1,4 +1,12 @@ { + "__description": { + "": "root element for type identified by ''", + ".comment": "description for '' (optional)", + ".optional_subtype": "true to allow null subtype, false by default if omitted", + ".subtypes": "root element for sub-types of type '.subtypes.": "sub-type element for ", + ".subtypes..comment": "description of subtype (optional)" + }, "app": { "optional_subtype": true, "subtypes": { From e3b8ba138a66c8c4fc75bdf6f9c6a0ca13ad8f81 Mon Sep 17 00:00:00 2001 From: Sylvain Juge Date: Tue, 1 Jun 2021 17:29:59 +0200 Subject: [PATCH 20/25] split subtype matching & null subtype --- .../co/elastic/apm/agent/MockReporter.java | 44 ++++++++++--------- .../test/resources/json-specs/span_types.json | 14 +++--- 2 files changed, 31 insertions(+), 27 deletions(-) diff --git a/apm-agent-core/src/test/java/co/elastic/apm/agent/MockReporter.java b/apm-agent-core/src/test/java/co/elastic/apm/agent/MockReporter.java index 0bc3843953..f669422a13 100644 --- a/apm-agent-core/src/test/java/co/elastic/apm/agent/MockReporter.java +++ b/apm-agent-core/src/test/java/co/elastic/apm/agent/MockReporter.java @@ -219,37 +219,39 @@ private void verifySpanType(Span span) { if (checkStrictSpanType) { JsonNode typeJson = getMandatoryJson(SPAN_TYPES_SPEC, type, String.format("span type '%s' is not allowed by the spec", type)); - String typeComment = getOptionalComment(typeJson); - - JsonNode jsonOptionalSubtype = typeJson.get("optional_subtype"); - boolean optionalSubtype = jsonOptionalSubtype != null && jsonOptionalSubtype.isBoolean() && jsonOptionalSubtype.asBoolean(); + boolean allowNullSubtype = getBooleanJson(typeJson, "allow_null_subtype"); + boolean allowUnlistedSubtype = getBooleanJson(typeJson, "allow_unlisted_subtype"); String subType = span.getSubtype(); - - JsonNode subTypesJson = typeJson.get("subtypes"); - - if (subTypesJson == null || subTypesJson.isEmpty()) { - assertThat(subType) - .describedAs("span type '%s' does not allow for subtypes", type) - .isNull(); - } else if (subType == null) { - // span does not have a sub-type, make sure that it's only when spec allows for it - assertThat(optionalSubtype) - .describedAs("span type '%s' subtype is not optional by the spec (optional_subtype=false)", type) + if (null == subType) { + assertThat(allowNullSubtype) + .describedAs("span type '%s' requires non-null subtype (allow_null_subtype=false)") .isTrue(); } else { - assertThat(subType) - .describedAs("span subtype is required by the spec for type '%s'", type) - .isNotNull(); - - // we have a sub-type, make sure that the sub-type matches the spec - getMandatoryJson(subTypesJson, subType, String.format("span subtype '%s' is now allowed by the spec for type '%s' (%s)", subType, type, typeComment)); + if (!allowUnlistedSubtype) { + JsonNode subTypesJson = typeJson.get("subtypes"); + if (subTypesJson != null && !subTypesJson.isEmpty()) { + getMandatoryJson(subTypesJson, subType, String.format("span subtype '%s' is not allowed by the sped for type '%s'", subType, type)); + } + } } } } + private boolean getBooleanJson(JsonNode json, String name) { + JsonNode jsonValue = json.get(name); + boolean value = false; + if (jsonValue != null) { + assertThat(jsonValue.isBoolean()) + .describedAs("property %s should be a boolean", name) + .isTrue(); + value = jsonValue.asBoolean(); + } + return value; + } + private void verifyDestinationFields(Span span) { if (!span.isExit()) { return; diff --git a/apm-agent-core/src/test/resources/json-specs/span_types.json b/apm-agent-core/src/test/resources/json-specs/span_types.json index a90d9aac54..042a17ab6a 100644 --- a/apm-agent-core/src/test/resources/json-specs/span_types.json +++ b/apm-agent-core/src/test/resources/json-specs/span_types.json @@ -2,13 +2,14 @@ "__description": { "": "root element for type identified by ''", ".comment": "description for '' (optional)", - ".optional_subtype": "true to allow null subtype, false by default if omitted", - ".subtypes": "root element for sub-types of type '.allow_null_subtype": "true to allow null subtype, false by default if omitted", + ".allow_unlisted_subtype": "true to allow unlisted subtypes, false by default if omitted", + ".subtypes": "root element for sub-types of type '.subtypes.": "sub-type element for ", ".subtypes..comment": "description of subtype (optional)" }, "app": { - "optional_subtype": true, + "allow_null_subtype": true, "subtypes": { "inferred": { "comment": "Sampling profiler inferred spans" @@ -16,8 +17,8 @@ } }, "custom": { - "optional_subtype": true, - "comment": "API custom instrumentation" + "comment": "API custom instrumentation", + "allow_null_subtype": true }, "db": { "comment": "database span", @@ -131,7 +132,8 @@ } }, "template": { - "comment": "Template engines (no sub-type for now as really platform-specific)" + "comment": "Template engines (no sub-type for now as really platform-specific)", + "allow_unlisted_subtype": true } } From f76683500d05360f3c72bc662473342822c998e7 Mon Sep 17 00:00:00 2001 From: Sylvain Juge Date: Tue, 1 Jun 2021 20:55:48 +0200 Subject: [PATCH 21/25] another attempt to properly check --- .../co/elastic/apm/agent/MockReporter.java | 19 +++++++++++-------- .../test/resources/json-specs/span_types.json | 2 +- 2 files changed, 12 insertions(+), 9 deletions(-) diff --git a/apm-agent-core/src/test/java/co/elastic/apm/agent/MockReporter.java b/apm-agent-core/src/test/java/co/elastic/apm/agent/MockReporter.java index f669422a13..3f7df7e3e9 100644 --- a/apm-agent-core/src/test/java/co/elastic/apm/agent/MockReporter.java +++ b/apm-agent-core/src/test/java/co/elastic/apm/agent/MockReporter.java @@ -223,16 +223,19 @@ private void verifySpanType(Span span) { boolean allowUnlistedSubtype = getBooleanJson(typeJson, "allow_unlisted_subtype"); String subType = span.getSubtype(); + + JsonNode subTypesJson = typeJson.get("subtypes"); + boolean hasSubtypes = subTypesJson != null && !subTypesJson.isEmpty(); + if (null == subType) { - assertThat(allowNullSubtype) - .describedAs("span type '%s' requires non-null subtype (allow_null_subtype=false)") - .isTrue(); + if (hasSubtypes) { + assertThat(allowNullSubtype) + .describedAs("span type '%s' requires non-null subtype (allow_null_subtype=false)") + .isTrue(); + } } else { - if (!allowUnlistedSubtype) { - JsonNode subTypesJson = typeJson.get("subtypes"); - if (subTypesJson != null && !subTypesJson.isEmpty()) { - getMandatoryJson(subTypesJson, subType, String.format("span subtype '%s' is not allowed by the sped for type '%s'", subType, type)); - } + if (!allowUnlistedSubtype && hasSubtypes) { + getMandatoryJson(subTypesJson, subType, String.format("span subtype '%s' is not allowed by the sped for type '%s'", subType, type)); } } diff --git a/apm-agent-core/src/test/resources/json-specs/span_types.json b/apm-agent-core/src/test/resources/json-specs/span_types.json index 042a17ab6a..73b899ba9b 100644 --- a/apm-agent-core/src/test/resources/json-specs/span_types.json +++ b/apm-agent-core/src/test/resources/json-specs/span_types.json @@ -4,7 +4,7 @@ ".comment": "description for '' (optional)", ".allow_null_subtype": "true to allow null subtype, false by default if omitted", ".allow_unlisted_subtype": "true to allow unlisted subtypes, false by default if omitted", - ".subtypes": "root element for sub-types of type '.subtypes": "root element for sub-types of type '.subtypes.": "sub-type element for ", ".subtypes..comment": "description of subtype (optional)" }, From 5126d37420fa3d171107d560c02d92017d91831f Mon Sep 17 00:00:00 2001 From: Sylvain Juge Date: Wed, 2 Jun 2021 10:03:26 +0200 Subject: [PATCH 22/25] fit OpenTracing bridge tests --- .../src/test/java/co/elastic/apm/agent/MockReporter.java | 2 +- .../apm/agent/opentracingimpl/ApmSpanInstrumentation.java | 3 ++- .../java/co/elastic/apm/opentracing/OpenTracingBridgeTest.java | 3 +++ 3 files changed, 6 insertions(+), 2 deletions(-) diff --git a/apm-agent-core/src/test/java/co/elastic/apm/agent/MockReporter.java b/apm-agent-core/src/test/java/co/elastic/apm/agent/MockReporter.java index 3f7df7e3e9..93efa1e753 100644 --- a/apm-agent-core/src/test/java/co/elastic/apm/agent/MockReporter.java +++ b/apm-agent-core/src/test/java/co/elastic/apm/agent/MockReporter.java @@ -230,7 +230,7 @@ private void verifySpanType(Span span) { if (null == subType) { if (hasSubtypes) { assertThat(allowNullSubtype) - .describedAs("span type '%s' requires non-null subtype (allow_null_subtype=false)") + .describedAs("span type '%s' requires non-null subtype (allow_null_subtype=false)", type) .isTrue(); } } else { diff --git a/apm-agent-plugins/apm-opentracing-plugin/src/main/java/co/elastic/apm/agent/opentracingimpl/ApmSpanInstrumentation.java b/apm-agent-plugins/apm-opentracing-plugin/src/main/java/co/elastic/apm/agent/opentracingimpl/ApmSpanInstrumentation.java index 2033943923..279e2e27af 100644 --- a/apm-agent-plugins/apm-opentracing-plugin/src/main/java/co/elastic/apm/agent/opentracingimpl/ApmSpanInstrumentation.java +++ b/apm-agent-plugins/apm-opentracing-plugin/src/main/java/co/elastic/apm/agent/opentracingimpl/ApmSpanInstrumentation.java @@ -265,7 +265,8 @@ private static boolean handleSpecialSpanTag(Span span, String key, Object value) } else if ("http.status_code".equals(key) && value instanceof Number) { int status = ((Number) value).intValue(); span.getContext().getHttp().withStatusCode(status); - span.withOutcome(ResultUtil.getOutcomeByHttpClientStatus(status)); + span.withSubtype("http") + .withOutcome(ResultUtil.getOutcomeByHttpClientStatus(status)); return true; } else if ("http.url".equals(key) && value instanceof String) { span.getContext().getHttp().withUrl((String) value); diff --git a/apm-opentracing/src/test/java/co/elastic/apm/opentracing/OpenTracingBridgeTest.java b/apm-opentracing/src/test/java/co/elastic/apm/opentracing/OpenTracingBridgeTest.java index 575d887e94..724d8499d2 100644 --- a/apm-opentracing/src/test/java/co/elastic/apm/opentracing/OpenTracingBridgeTest.java +++ b/apm-opentracing/src/test/java/co/elastic/apm/opentracing/OpenTracingBridgeTest.java @@ -300,6 +300,9 @@ public void testAgentPaused() { @Test void testResolveClientType() { + // does not fit the shared type/subtype spec + reporter.disableCheckStrictSpanType(); + assertSoftly(softly -> { softly.assertThat(createSpanFromOtTags(Map.of("span.kind", "client")).getType()).isEqualTo("external"); softly.assertThat(createSpanFromOtTags(Map.of("span.kind", "producer")).getType()).isEqualTo("external"); From c3188032ff972bd58f5c2da42bc825b640abe0c9 Mon Sep 17 00:00:00 2001 From: Sylvain Juge Date: Wed, 2 Jun 2021 10:30:21 +0200 Subject: [PATCH 23/25] comment -> __description --- .../test/resources/json-specs/span_types.json | 77 +++++++++---------- 1 file changed, 38 insertions(+), 39 deletions(-) diff --git a/apm-agent-core/src/test/resources/json-specs/span_types.json b/apm-agent-core/src/test/resources/json-specs/span_types.json index 73b899ba9b..32f2321cf1 100644 --- a/apm-agent-core/src/test/resources/json-specs/span_types.json +++ b/apm-agent-core/src/test/resources/json-specs/span_types.json @@ -1,139 +1,138 @@ { "__description": { "": "root element for type identified by ''", - ".comment": "description for '' (optional)", + ".__description": "description for '' (optional)", ".allow_null_subtype": "true to allow null subtype, false by default if omitted", ".allow_unlisted_subtype": "true to allow unlisted subtypes, false by default if omitted", ".subtypes": "root element for sub-types of type '.subtypes.": "sub-type element for ", - ".subtypes..comment": "description of subtype (optional)" + ".subtypes..__description": "description of subtype (optional)" }, "app": { "allow_null_subtype": true, "subtypes": { "inferred": { - "comment": "Sampling profiler inferred spans" + "__description": "Sampling profiler inferred spans" } } }, "custom": { - "comment": "API custom instrumentation", + "__description": "API custom instrumentation", "allow_null_subtype": true }, "db": { - "comment": "database span", + "__description": "database span", "subtypes": { "cassandra": { - "comment": "Cassandra" + "__description": "Cassandra" }, "cosmosdb": { - "comment": "Azure CosmosDB" + "__description": "Azure CosmosDB" }, "db2": { - "comment": "IBM DB2" + "__description": "IBM DB2" }, "derby": { - "comment": "Apache Derby" + "__description": "Apache Derby" }, "dynamodb": { - "comment": "AWS DynamoDB" + "__description": "AWS DynamoDB" }, "elasticsearch": { - "comment": "Elasticsearch" + "__description": "Elasticsearch" }, "h2": { - "comment": "H2" + "__description": "H2" }, "hsqldb": { - "comment": "HSQLDB" + "__description": "HSQLDB" }, "ingres": { - "comment": "Ingres" + "__description": "Ingres" }, "mariadb": { - "comment": "MariaDB" + "__description": "MariaDB" }, "mongodb": { - "comment": "MongoDB" + "__description": "MongoDB" }, "mysql": { - "comment": "MySQL" + "__description": "MySQL" }, "oracle": { - "comment": "Oracle Database" + "__description": "Oracle Database" }, "postgresql": { - "comment": "PostgreSQL" + "__description": "PostgreSQL" }, "redis": { - "comment": "Redis" + "__description": "Redis" }, "sqlserver": { - "comment": "Microsoft SQL Server" + "__description": "Microsoft SQL Server" }, "unknown": { - "comment": "Unknown database" + "__description": "Unknown database" } } }, "external": { "subtypes": { "dubbo": { - "comment": "Apache Dubbo" + "__description": "Apache Dubbo" }, "grpc": { - "comment": "gRPC" + "__description": "gRPC" }, "http": { - "comment": "HTTP client" + "__description": "HTTP client" } } }, "messaging": { - "comment": "Messaging", + "__description": "Messaging", "subtypes": { "azurequeue": { - "comment": "Azure Queue" + "__description": "Azure Queue" }, "azureservicebus": { - "comment": "Azure Service Bus" + "__description": "Azure Service Bus" }, "jms": { - "comment": "Java Messaging Service" + "__description": "Java Messaging Service" }, "kafka": { - "comment": "Apache Kafka" + "__description": "Apache Kafka" }, "rabbitmq": { - "comment": "RabbitMQ" + "__description": "RabbitMQ" }, "sns": { - "comment": "AWS Simple Notification Service" + "__description": "AWS Simple Notification Service" }, "sqs": { - "comment": "AWS Simple Queue Service" + "__description": "AWS Simple Queue Service" } } }, "process": { - "comment": "External process" + "__description": "External process" }, "storage": { "subtypes": { "azurefile": { - "comment": "Azure Files" + "__description": "Azure Files" }, "azuresblob": { - "comment": "Azure Blob Storage" + "__description": "Azure Blob Storage" }, "s3": { - "comment": "AWS S3" + "__description": "AWS S3" } } }, "template": { - "comment": "Template engines (no sub-type for now as really platform-specific)", + "__description": "Template engines (no sub-type for now as really platform-specific)", "allow_unlisted_subtype": true } } - From 4d3668cfcd62949e8bf08fde04c705717faaf73c Mon Sep 17 00:00:00 2001 From: Sylvain Juge Date: Wed, 2 Jun 2021 11:39:40 +0200 Subject: [PATCH 24/25] code cleanup --- .../co/elastic/apm/agent/MockReporter.java | 27 +++++++------------ 1 file changed, 10 insertions(+), 17 deletions(-) diff --git a/apm-agent-core/src/test/java/co/elastic/apm/agent/MockReporter.java b/apm-agent-core/src/test/java/co/elastic/apm/agent/MockReporter.java index 93efa1e753..27fb5ea31d 100644 --- a/apm-agent-core/src/test/java/co/elastic/apm/agent/MockReporter.java +++ b/apm-agent-core/src/test/java/co/elastic/apm/agent/MockReporter.java @@ -243,18 +243,6 @@ private void verifySpanType(Span span) { } - private boolean getBooleanJson(JsonNode json, String name) { - JsonNode jsonValue = json.get(name); - boolean value = false; - if (jsonValue != null) { - assertThat(jsonValue.isBoolean()) - .describedAs("property %s should be a boolean", name) - .isTrue(); - value = jsonValue.asBoolean(); - } - return value; - } - private void verifyDestinationFields(Span span) { if (!span.isExit()) { return; @@ -558,7 +546,6 @@ public void awaitUntilAsserted(long timeoutMs, ThrowingRunnable runnable) { } } - private static boolean hasEmptyTraceContext(AbstractSpan item) { return item.getTraceContext().getId().isEmpty(); } @@ -573,9 +560,15 @@ private static JsonNode getMandatoryJson(JsonNode json, String name, String desc return jsonNode; } - private static String getOptionalComment(JsonNode json) { - return Optional.ofNullable(json.get("comment")) - .map(JsonNode::asText) - .orElse(""); + private static boolean getBooleanJson(JsonNode json, String name) { + JsonNode jsonValue = json.get(name); + boolean value = false; + if (jsonValue != null) { + assertThat(jsonValue.isBoolean()) + .describedAs("property %s should be a boolean", name) + .isTrue(); + value = jsonValue.asBoolean(); + } + return value; } } From 899215d39d0219106d457e541fddc074576d25df Mon Sep 17 00:00:00 2001 From: SylvainJuge Date: Wed, 2 Jun 2021 15:20:40 +0200 Subject: [PATCH 25/25] Update apm-agent-core/src/test/resources/json-specs/span_types.json Co-authored-by: eyalkoren <41850454+eyalkoren@users.noreply.github.com> --- apm-agent-core/src/test/resources/json-specs/span_types.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/apm-agent-core/src/test/resources/json-specs/span_types.json b/apm-agent-core/src/test/resources/json-specs/span_types.json index 32f2321cf1..847872d8df 100644 --- a/apm-agent-core/src/test/resources/json-specs/span_types.json +++ b/apm-agent-core/src/test/resources/json-specs/span_types.json @@ -4,7 +4,7 @@ ".__description": "description for '' (optional)", ".allow_null_subtype": "true to allow null subtype, false by default if omitted", ".allow_unlisted_subtype": "true to allow unlisted subtypes, false by default if omitted", - ".subtypes": "root element for sub-types of type '.subtypes": "root element for sub-types of type '', if omitted or empty subtype must be null, unless 'allow_unlisted_subtype' is set to true", ".subtypes.": "sub-type element for ", ".subtypes..__description": "description of subtype (optional)" },