Prevent cycles in inferred spans (#3588)

Author: JonasKunz

CHANGELOG.asciidoc

@@ -31,6 +31,10 @@ Use subheadings with the "=====" level for adding notes for unreleased changes:
 
 === Unreleased
 
+[float]
+===== Bug fixes
+* Fixed edge case where inferred spans could cause cycles in the trace parent-child relationships, subsequently resulting in the UI crashing - {pull}3588[#3588]
+
 [[release-notes-1.x]]
 === Java Agent version 1.x
 

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

@@ -64,7 +64,7 @@ public class TraceContext implements Recyclable, co.elastic.apm.agent.tracer.Tra
  public static final String ELASTIC_TRACE_PARENT_TEXTUAL_HEADER_NAME = "elastic-apm-traceparent";
  public static final String W3C_TRACE_PARENT_TEXTUAL_HEADER_NAME = "traceparent";
  public static final String TRACESTATE_HEADER_NAME = "tracestate";
- public static final int SERIALIZED_LENGTH = 42;
+ public static final int SERIALIZED_LENGTH = 51;
  private static final int TEXT_HEADER_EXPECTED_LENGTH = 55;
  private static final int TEXT_HEADER_TRACE_ID_OFFSET = 3;
  private static final int TEXT_HEADER_PARENT_ID_OFFSET = 36;
@@ -227,6 +227,7 @@ private TraceContext(ElasticApmTracer tracer, Id id) {
  * <p>
  * Note: the {@link #traceId} will still be 128 bit
  * </p>
+ *
  * @param tracer a valid tracer
  */
  public static TraceContext with64BitId(ElasticApmTracer tracer) {
@@ -621,7 +622,7 @@ public boolean equals(Object o) {
  if (o == null || getClass() != o.getClass()) return false;
  TraceContext that = (TraceContext) o;
  return id.equals(that.id) &&
- traceId.equals(that.traceId);
+  traceId.equals(that.traceId);
  }
 
  public boolean idEquals(@Nullable TraceContext o) {
@@ -650,6 +651,8 @@ public void serialize(byte[] buffer) {
  offset = traceId.toBytes(buffer, offset);
  offset = id.toBytes(buffer, offset);
  offset = transactionId.toBytes(buffer, offset);
+ buffer[offset++] = parentId.isEmpty() ? (byte) 0 : (byte) 1;
+ offset = parentId.toBytes(buffer, offset);
  buffer[offset++] = flags;
  buffer[offset++] = (byte) (discardable ? 1 : 0);
  ByteUtils.putLong(buffer, offset, clock.getOffset());
@@ -660,6 +663,12 @@ public void deserialize(byte[] buffer, @Nullable String serviceName, @Nullable S
  offset += traceId.fromBytes(buffer, offset);
  offset += id.fromBytes(buffer, offset);
  offset += transactionId.fromBytes(buffer, offset);
+ if (buffer[offset++] != 0) {
+ offset += parentId.fromBytes(buffer, offset);
+ } else {
+ parentId.resetState();
+ offset += 8;
+ }
  flags = buffer[offset++];
  discardable = buffer[offset++] == (byte) 1;
  clock.init(ByteUtils.getLong(buffer, offset));
@@ -672,6 +681,10 @@ public static long getSpanId(byte[] serializedTraceContext) {
  return ByteUtils.getLong(serializedTraceContext, 16);
  }
 
+ public static long getParentId(byte[] serializedTraceContext) {
+ return ByteUtils.getLong(serializedTraceContext, 33);
+ }
+
  public boolean traceIdAndIdEquals(byte[] serialized) {
  return id.dataEquals(serialized, traceId.getLength()) && traceId.dataEquals(serialized, 0);
  }

apm-agent-plugins/apm-profiling-plugin/src/main/java/co/elastic/apm/agent/profiler/CallTree.java

@@ -79,9 +79,9 @@ public class CallTree implements Recyclable {
  * @see co.elastic.apm.agent.impl.transaction.AbstractSpan#childIds
  */
  @Nullable
- private LongList childIds;
+ private ChildList childIds;
  @Nullable
- private LongList maybeChildIds;
+ private ChildList maybeChildIds;
 
  public CallTree() {
  }
@@ -375,28 +375,29 @@ private void toString(Appendable out, int level) throws IOException {
  }
  }
 
- int spanify(CallTree.Root root, TraceContext parentContext) {
+ int spanify(CallTree.Root root, TraceContext parentContext, TraceContext nonInferredParentContext) {
  int createdSpans = 0;
  if (activeContextOfDirectParent != null) {
  parentContext = activeContextOfDirectParent;
+ nonInferredParentContext = activeContextOfDirectParent;
  }
  Span span = null;
  if (!isPillar() || isLeaf()) {
  createdSpans++;
- span = asSpan(root, parentContext);
+ span = asSpan(root, parentContext, nonInferredParentContext);
  this.isSpan = true;
  }
  List<CallTree> children = getChildren();
  for (int i = 0, size = children.size(); i < size; i++) {
- createdSpans += children.get(i).spanify(root, span != null ? span.getTraceContext() : parentContext);
+ createdSpans += children.get(i).spanify(root, span != null ? span.getTraceContext() : parentContext, nonInferredParentContext);
  }
  if (span != null) {
  span.end(span.getTimestamp() + getDurationUs());
  }
  return createdSpans;
  }
 
- protected Span asSpan(Root root, TraceContext parentContext) {
+ protected Span asSpan(Root root, TraceContext parentContext, TraceContext nonInferredParentContext) {
  transferMaybeChildIdsToChildIds();
  Span span = parentContext.createSpan(root.getEpochMicros(this.start))
  .withType("app")
@@ -410,7 +411,21 @@ protected Span asSpan(Root root, TraceContext parentContext) {
  }
  span.appendToName("#");
  span.appendToName(frame.getMethodName());
- span.withChildIds(childIds);
+
+ LongList childSpanIds = null;
+ if (childIds != null) {
+ long expectedParent = nonInferredParentContext.getId().readLong(0);
+ childSpanIds = new LongList(childIds.getSize());
+ for (int i = 0; i < childIds.getSize(); i++) {
+ // to avoid cycles, we only insert child-ids if the parent of the child is also
+ // the parent of the stack of inferred spans inserted
+ if (childIds.getParentId(i) == expectedParent) {
+ childSpanIds.add(childIds.getId(i));
+ }
+ }
+ }
+
+ span.withChildIds(childSpanIds);
 
  // we're not interested in the very bottom of the stack which contains things like accepting and handling connections
  if (!root.rootContext.idEquals(parentContext)) {
@@ -498,19 +513,20 @@ public void resetState() {
  * </p>
  *
  * @param id the child span id to add to this call tree element
+ * @param parentId the parent id of the span represented by the id parameter
  */
- public void addMaybeChildId(long id) {
+ public void addMaybeChildId(long id, long parentId) {
  if (maybeChildIds == null) {
- maybeChildIds = new LongList();
+ maybeChildIds = new ChildList();
  }
- maybeChildIds.add(id);
+ maybeChildIds.add(id, parentId);
  }
 
- public void addChildId(long id) {
+ public void addChildId(long id, long parentId) {
  if (childIds == null) {
- childIds = new LongList();
+ childIds = new ChildList();
  }
- childIds.add(id);
+ childIds.add(id, parentId);
  }
 
  public boolean hasChildIds() {
@@ -541,7 +557,11 @@ void giveChildIdsTo(CallTree giveTo) {
 
  void giveLastChildIdTo(CallTree giveTo) {
  if (childIds != null && !childIds.isEmpty()) {
- giveTo.addChildId(childIds.remove(childIds.getSize() - 1));
+ int size = childIds.getSize();
+ long id = childIds.getId(size - 1);
+ long parentId = childIds.getParentId(size - 1);
+ giveTo.addChildId(id, parentId);
+ childIds.removeLast();
  }
  }
 
@@ -619,7 +639,7 @@ public void onActivation(byte[] active, long timestamp) {
  long spanId = TraceContext.getSpanId(active);
  activeSet.add(spanId);
  if (!isNestedActivation(topOfStack)) {
- topOfStack.addMaybeChildId(spanId);
+ topOfStack.addMaybeChildId(spanId, TraceContext.getParentId(active));
  }
  }
  }
@@ -628,12 +648,12 @@ private boolean isNestedActivation(CallTree topOfStack) {
  return isAnyActive(topOfStack.childIds) || isAnyActive(topOfStack.maybeChildIds);
  }
 
- private boolean isAnyActive(@Nullable LongList spanIds) {
+ private boolean isAnyActive(@Nullable ChildList spanIds) {
  if (spanIds == null) {
  return false;
  }
  for (int i = 0, size = spanIds.getSize(); i < size; i++) {
- if (activeSet.contains(spanIds.get(i))) {
+ if (activeSet.contains(spanIds.getId(i))) {
  return true;
  }
  }
@@ -719,7 +739,7 @@ public int spanify() {
  int createdSpans = 0;
  List<CallTree> callTrees = getChildren();
  for (int i = 0, size = callTrees.size(); i < size; i++) {
- createdSpans += callTrees.get(i).spanify(this, rootContext);
+ createdSpans += callTrees.get(i).spanify(this, rootContext, rootContext);
  }
  return createdSpans;
  }

apm-agent-plugins/apm-profiling-plugin/src/main/java/co/elastic/apm/agent/profiler/ChildList.java

@@ -0,0 +1,64 @@
+/*
+ * Licensed to Elasticsearch B.V. under one or more contributor
+ * license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright
+ * ownership. Elasticsearch B.V. licenses this file to you under
+ * the Apache License, Version 2.0 (the "License"); you may
+ * not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package co.elastic.apm.agent.profiler;
+
+
+import co.elastic.apm.agent.sdk.internal.collections.LongList;
+
+/** List for maintaining pairs of (spanId,parentIds) both represented as longs. */
+public class ChildList {
+
+ // this list contains the (spanId,parentIds) flattened
+ private final LongList idsWithParentIds = new LongList();
+
+ public void add(long id, long parentId) {
+ idsWithParentIds.add(id);
+ idsWithParentIds.add(parentId);
+ }
+
+ public long getId(int index) {
+ return idsWithParentIds.get(index * 2);
+ }
+
+ public long getParentId(int index) {
+ return idsWithParentIds.get(index * 2 + 1);
+ }
+
+ public int getSize() {
+ return idsWithParentIds.getSize() / 2;
+ }
+
+ public void addAll(ChildList other) {
+ idsWithParentIds.addAll(other.idsWithParentIds);
+ }
+
+ public void clear() {
+ idsWithParentIds.clear();
+ }
+
+ public boolean isEmpty() {
+ return getSize() == 0;
+ }
+
+ public void removeLast() {
+ int size = idsWithParentIds.getSize();
+ idsWithParentIds.remove(size - 1);
+ idsWithParentIds.remove(size - 2);
+ }
+}

apm-agent-plugins/apm-profiling-plugin/src/main/java/co/elastic/apm/agent/profiler/SamplingProfiler.java

@@ -106,7 +106,7 @@
  * and at least one stack trace.
  * Once {@linkplain ActivationEvent#handleDeactivationEvent(SamplingProfiler) handling the deactivation event} of the root span in a thread
  * (after which {@link ElasticApmTracer#getActive()} would return {@code null}),
- * the {@link CallTree} is {@linkplain CallTree#spanify(CallTree.Root, TraceContext) converted into regular spans}.
+ * the {@link CallTree} is {@linkplain CallTree#spanify(CallTree.Root, TraceContext, TraceContext) converted into regular spans}.
  * </p>
  * <p>
  * Overall, the allocation rate does not depend on the number of {@link ActivationEvent}s but only on

apm-agent-plugins/apm-profiling-plugin/src/test/java/co/elastic/apm/agent/profiler/CallTreeSpanifyTest.java

@@ -35,6 +35,7 @@
 import org.stagemonitor.configuration.ConfigurationRegistry;
 
 import java.io.IOException;
+import java.util.Collections;
 import java.util.List;
 import java.util.Objects;
 import java.util.concurrent.TimeUnit;
@@ -141,8 +142,62 @@ void testCallTreeWithActiveSpan() {
  root.spanify();
 
  assertThat(reporter.getSpans()).hasSize(2);
- assertThat(reporter.getSpans().get(1).getTraceContext().isChildOf(spanContext));
- assertThat(reporter.getSpans().get(0).getTraceContext().isChildOf(rootContext));
+ assertThat(reporter.getSpans().get(0).getTraceContext().isChildOf(spanContext)).isTrue();
+ assertThat(reporter.getSpans().get(1).getTraceContext().isChildOf(rootContext)).isTrue();
+ }
+
+
+ @Test
+ void testSpanWithInvertedActivation() {
+ TraceContext rootContext = CallTreeTest.rootTraceContext(tracer);
+
+ TraceContext childSpanContext = TraceContext.with64BitId(tracer);
+ TraceContext.fromParentContext().asChildOf(childSpanContext, rootContext);
+
+ NoopObjectPool<CallTree.Root> rootPool = NoopObjectPool.ofRecyclable(() -> new CallTree.Root(tracer));
+ NoopObjectPool<CallTree> childPool = NoopObjectPool.ofRecyclable(CallTree::new);
+
+ CallTree.Root root = CallTree.createRoot(rootPool, childSpanContext.serialize(), rootContext.getServiceName(), rootContext.getServiceVersion(), 0);
+ root.addStackTrace(tracer, Collections.singletonList(StackFrame.of("A", "a")), 10_000, childPool, 0);
+
+ root.onActivation(rootContext.serialize(), 20_000);
+ root.onDeactivation(rootContext.serialize(), childSpanContext.serialize(), 30_000);
+
+ root.addStackTrace(tracer, Collections.singletonList(StackFrame.of("A", "a")), 40_000, childPool, 0);
+ root.end(childPool, 0);
+
+ root.spanify();
+
+ assertThat(reporter.getSpans()).hasSize(1);
+ assertThat(reporter.getSpans().get(0).getTraceContext().isChildOf(childSpanContext)).isTrue();
+ // the inferred span should not have any span links because this
+ // span link would cause a cycle in the trace
+ assertThat(reporter.getSpans().get(0).getChildIds().getSize()).isEqualTo(0L);
+ }
+
+ @Test
+ void testSpanWithNestedActivation() {
+ TraceContext rootContext = CallTreeTest.rootTraceContext(tracer);
+
+ NoopObjectPool<CallTree.Root> rootPool = NoopObjectPool.ofRecyclable(() -> new CallTree.Root(tracer));
+ NoopObjectPool<CallTree> childPool = NoopObjectPool.ofRecyclable(CallTree::new);
+
+ CallTree.Root root = CallTree.createRoot(rootPool, rootContext.serialize(), rootContext.getServiceName(), rootContext.getServiceVersion(), 0);
+ root.addStackTrace(tracer, Collections.singletonList(StackFrame.of("A", "a")), 10_000, childPool, 0);
+
+ root.onActivation(rootContext.serialize(), 20_000);
+ root.onDeactivation(rootContext.serialize(), rootContext.serialize(), 30_000);
+
+ root.addStackTrace(tracer, Collections.singletonList(StackFrame.of("A", "a")), 40_000, childPool, 0);
+ root.end(childPool, 0);
+
+ root.spanify();
+
+ assertThat(reporter.getSpans()).hasSize(1);
+ assertThat(reporter.getSpans().get(0).getTraceContext().isChildOf(rootContext)).isTrue();
+ // the inferred span should not have any span links because this
+ // span link would cause a cycle in the trace
+ assertThat(reporter.getSpans().get(0).getChildIds().getSize()).isEqualTo(0L);
  }
 
 }

apm-agent-plugins/apm-profiling-plugin/src/test/java/co/elastic/apm/agent/profiler/CallTreeTest.java

@@ -120,7 +120,7 @@ void testCallTree() {
  @Test
  void testGiveEmptyChildIdsTo() {
  CallTree rich = new CallTree();
- rich.addChildId(42);
+ rich.addChildId(42, 0L);
  CallTree robinHood = new CallTree();
  CallTree poor = new CallTree();