Shared JSON spec for span types/subtypes

apm-agent-core/src/main/java/co/elastic/apm/agent/impl/transaction/Span.java

@@ -157,26 +157,31 @@ 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;
     }
 
     /**
      * 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;
     }
 
     /**
      * 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

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);

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) {

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;
@@ -78,17 +79,24 @@ 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<String, Collection<String>> 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<Transaction> transactions = Collections.synchronizedList(new ArrayList<>());
     private final List<Span> spans = Collections.synchronizedList(new ArrayList<>());
     private final List<ErrorCapture> errors = Collections.synchronizedList(new ArrayList<>());
     private final List<byte[]> 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");
+
     static {
         transactionSchema = getSchema("/schema/transactions/transaction.json");
         spanSchema = getSchema("/schema/transactions/span.json");
@@ -119,26 +127,43 @@ 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;
+    }
+
+    /**
+     * Disables destination address check
+     */
+    public void disableCheckDestinationAddress() {
+        checkDestinationAddress = false;
     }
 
-    public void enableDestinationAddressCheck() {
-        disableDestinationAddressCheck = false;
+    public boolean checkDestinationAddress() {
+        return checkDestinationAddress;
     }
 
-    public boolean isDisableDestinationAddressCheck() {
-        return disableDestinationAddressCheck;
+    /**
+     * Disables strict span type and sub-type check (against shared spec)
+     */
+    public void disableCheckStrictSpanType() {
+        checkStrictSpanType = false;
     }
 
     @Override
-    public void start() {}
+    public void start() {
+    }
 
     @Override
     public synchronized void report(Transaction transaction) {
@@ -164,27 +189,73 @@ public synchronized void report(Span span) {
         if (closed) {
             return;
         }
-        verifySpanSchema(asJson(dslJsonSerializer.toJsonString(span)));
-        verifyDestinationFields(span);
 
+        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", span.getType())
+                    .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).isNotNull();
+        assertThat(type)
+            .describedAs("span type is mandatory")
+            .isNotNull();
+
+        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();
+
+            String subType = span.getSubtype();
+
+            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)
+                    .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 (checkUnknownOutcomes) {
-            assertThat(span.getOutcome())
-                .describedAs("span outcome should be either success or failure for type = %s", type)
-                .isNotEqualTo(Outcome.UNKNOWN);
         }
 
-        spans.add(span);
     }
 
     private void verifyDestinationFields(Span span) {
         if (!span.isExit()) {
             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<String> unsupportedActions = SPAN_ACTIONS_WITHOUT_ADDRESS.getOrDefault(span.getSubtype(), Collections.emptySet());
             if (!unsupportedActions.contains(span.getAction())) {
@@ -421,7 +492,7 @@ public synchronized void assertRecycledAfterDecrementingReferences() {
 
         List<Span> spans = getSpans();
         List<Span> spansToFlush = spans.stream()
-            .filter(s-> !hasEmptyTraceContext(s))
+            .filter(s -> !hasEmptyTraceContext(s))
             .collect(Collectors.toList());
 
         transactionsToFlush.forEach(Transaction::decrementReferences);
@@ -486,4 +557,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("");
+    }
 }

apm-agent-core/src/test/java/co/elastic/apm/agent/impl/SpanTypeBreakdownTest.java

@@ -377,7 +377,12 @@ void testBreakdown_spanStartedAfterParentEnded() {
     }
 
     private void assertThatTransactionBreakdownCounterCreated(Map<? extends Labels, MetricSet> 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<? extends Labels, MetricSet> 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;
         }

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();
+    }
 }