Skip to content

Commit 61fbcf5

Browse files
JonasKunzJonasKunz
authored
Fix inferred span cycles (#201)
1 parent 121afa0 commit 61fbcf5

File tree

6 files changed

+251
-42
lines changed

6 files changed

+251
-42
lines changed

‎inferred-spans/src/main/java/co/elastic/otel/profiler/CallTree.java‎

Lines changed: 35 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,6 @@
2424
import co.elastic.otel.common.ElasticAttributes;
2525
import co.elastic.otel.common.util.HexUtils;
2626
import co.elastic.otel.profiler.collections.LongHashSet;
27-
import co.elastic.otel.profiler.collections.LongList;
2827
import co.elastic.otel.profiler.pooling.ObjectPool;
2928
import co.elastic.otel.profiler.pooling.Recyclable;
3029
import io.opentelemetry.api.common.Attributes;
@@ -84,9 +83,9 @@ public class CallTree implements Recyclable {
8483
private boolean isSpan;
8584
private int depth;
8685

87-
@Nullable private LongList childIds;
86+
@Nullable private ChildList childIds;
8887

89-
@Nullable private LongList maybeChildIds;
88+
@Nullable private ChildList maybeChildIds;
9089

9190
public CallTree() {}
9291

@@ -504,7 +503,8 @@ protected Span asSpan(
504503
.setStartTimestamp(
505504
clock.toEpochNanos(parentContext.getClockAnchor(), this.start),
506505
TimeUnit.NANOSECONDS);
507-
insertChildIdLinks(spanBuilder, Span.fromContext(parentOtelCtx).getSpanContext(), tempBuilder);
506+
insertChildIdLinks(
507+
spanBuilder, Span.fromContext(parentOtelCtx).getSpanContext(), parentContext, tempBuilder);
508508

509509
// we're not interested in the very bottom of the stack which contains things like accepting and
510510
// handling connections
@@ -524,20 +524,27 @@ protected Span asSpan(
524524
}
525525

526526
private void insertChildIdLinks(
527-
SpanBuilder span, SpanContext parentContext, StringBuilder tempBuilder) {
527+
SpanBuilder span,
528+
SpanContext parentContext,
529+
TraceContext nonInferredParent,
530+
StringBuilder tempBuilder) {
528531
if (childIds == null || childIds.isEmpty()) {
529532
return;
530533
}
531534
for (int i = 0; i < childIds.getSize(); i++) {
532-
tempBuilder.setLength(0);
533-
HexUtils.appendLongAsHex(childIds.get(i), tempBuilder);
534-
SpanContext spanContext =
535-
SpanContext.create(
536-
parentContext.getTraceId(),
537-
tempBuilder.toString(),
538-
parentContext.getTraceFlags(),
539-
parentContext.getTraceState());
540-
span.addLink(spanContext, CHILD_LINK_ATTRIBUTES);
535+
// to avoid cycles, we only insert child-ids if the parent of the child is also
536+
// the parent of the stack of inferred spans inserted
537+
if (nonInferredParent.getSpanId() == childIds.getParentId(i)) {
538+
tempBuilder.setLength(0);
539+
HexUtils.appendLongAsHex(childIds.getId(i), tempBuilder);
540+
SpanContext spanContext =
541+
SpanContext.create(
542+
parentContext.getTraceId(),
543+
tempBuilder.toString(),
544+
parentContext.getTraceFlags(),
545+
parentContext.getTraceState());
546+
span.addLink(spanContext, CHILD_LINK_ATTRIBUTES);
547+
}
541548
}
542549
}
543550

@@ -623,18 +630,18 @@ public void resetState() {
623630
*
624631
* @param id the child span id to add to this call tree element
625632
*/
626-
public void addMaybeChildId(long id) {
633+
public void addMaybeChildId(long id, long parentId) {
627634
if (maybeChildIds == null) {
628-
maybeChildIds = new LongList();
635+
maybeChildIds = new ChildList();
629636
}
630-
maybeChildIds.add(id);
637+
maybeChildIds.add(id, parentId);
631638
}
632639

633-
public void addChildId(long id) {
640+
public void addChildId(long id, long parentId) {
634641
if (childIds == null) {
635-
childIds = new LongList();
642+
childIds = new ChildList();
636643
}
637-
childIds.add(id);
644+
childIds.add(id, parentId);
638645
}
639646

640647
public boolean hasChildIds() {
@@ -664,7 +671,11 @@ void giveChildIdsTo(CallTree giveTo) {
664671

665672
void giveLastChildIdTo(CallTree giveTo) {
666673
if (childIds != null && !childIds.isEmpty()) {
667-
giveTo.addChildId(childIds.remove(childIds.getSize() - 1));
674+
int size = childIds.getSize();
675+
long id = childIds.getId(size - 1);
676+
long parentId = childIds.getParentId(size - 1);
677+
giveTo.addChildId(id, parentId);
678+
childIds.removeLast();
668679
}
669680
}
670681

@@ -743,7 +754,7 @@ public void onActivation(byte[] active, long timestamp) {
743754
long spanId = TraceContext.getSpanId(active);
744755
activeSet.add(spanId);
745756
if (!isNestedActivation(topOfStack)) {
746-
topOfStack.addMaybeChildId(spanId);
757+
topOfStack.addMaybeChildId(spanId, TraceContext.getParentId(active));
747758
}
748759
}
749760
}
@@ -752,12 +763,12 @@ private boolean isNestedActivation(CallTree topOfStack) {
752763
return isAnyActive(topOfStack.childIds) || isAnyActive(topOfStack.maybeChildIds);
753764
}
754765

755-
private boolean isAnyActive(@Nullable LongList spanIds) {
766+
private boolean isAnyActive(@Nullable ChildList spanIds) {
756767
if (spanIds == null) {
757768
return false;
758769
}
759770
for (int i = 0, size = spanIds.getSize(); i < size; i++) {
760-
if (activeSet.contains(spanIds.get(i))) {
771+
if (activeSet.contains(spanIds.getId(i))) {
761772
return true;
762773
}
763774
}

‎inferred-spans/src/main/java/co/elastic/otel/profiler/ChildList.java‎

Lines changed: 63 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,63 @@
1+
/*
2+
* Licensed to Elasticsearch B.V. under one or more contributor
3+
* license agreements. See the NOTICE file distributed with
4+
* this work for additional information regarding copyright
5+
* ownership. Elasticsearch B.V. licenses this file to you under
6+
* the Apache License, Version 2.0 (the "License"); you may
7+
* not use this file except in compliance with the License.
8+
* You may obtain a copy of the License at
9+
*
10+
* http://www.apache.org/licenses/LICENSE-2.0
11+
*
12+
* Unless required by applicable law or agreed to in writing,
13+
* software distributed under the License is distributed on an
14+
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
15+
* KIND, either express or implied. See the License for the
16+
* specific language governing permissions and limitations
17+
* under the License.
18+
*/
19+
package co.elastic.otel.profiler;
20+
21+
import co.elastic.otel.profiler.collections.LongList;
22+
23+
/** List for maintaining pairs of (spanId,parentIds) both represented as longs. */
24+
public class ChildList {
25+
26+
// this list contains the (spanId,parentIds) flattened
27+
private LongList idsWithParentIds = new LongList();
28+
29+
public void add(long id, long parentId) {
30+
idsWithParentIds.add(id);
31+
idsWithParentIds.add(parentId);
32+
}
33+
34+
public long getId(int index) {
35+
return idsWithParentIds.get(index * 2);
36+
}
37+
38+
public long getParentId(int index) {
39+
return idsWithParentIds.get(index * 2 + 1);
40+
}
41+
42+
public int getSize() {
43+
return idsWithParentIds.getSize() / 2;
44+
}
45+
46+
public void addAll(ChildList other) {
47+
idsWithParentIds.addAll(other.idsWithParentIds);
48+
}
49+
50+
public void clear() {
51+
idsWithParentIds.clear();
52+
}
53+
54+
public boolean isEmpty() {
55+
return getSize() == 0;
56+
}
57+
58+
public void removeLast() {
59+
int size = idsWithParentIds.getSize();
60+
idsWithParentIds.remove(size - 1);
61+
idsWithParentIds.remove(size - 2);
62+
}
63+
}

‎inferred-spans/src/main/java/co/elastic/otel/profiler/SamplingProfiler.java‎

Lines changed: 2 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -862,15 +862,12 @@ private void set(
862862
@Nullable Span previousContext,
863863
long nanoTime,
864864
SpanAnchoredClock clock) {
865-
TraceContext.serialize(
866-
traceContext.getSpanContext(), clock.getAnchor(traceContext), traceContextBuffer);
865+
TraceContext.serialize(traceContext, clock.getAnchor(traceContext), traceContextBuffer);
867866
this.threadId = threadId;
868867
this.activation = activation;
869868
if (previousContext != null) {
870869
TraceContext.serialize(
871-
previousContext.getSpanContext(),
872-
clock.getAnchor(previousContext),
873-
previousContextBuffer);
870+
previousContext, clock.getAnchor(previousContext), previousContextBuffer);
874871
rootContext = false;
875872
} else {
876873
rootContext = true;

‎inferred-spans/src/main/java/co/elastic/otel/profiler/TraceContext.java‎

Lines changed: 53 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,7 @@
2525
import io.opentelemetry.api.trace.SpanContext;
2626
import io.opentelemetry.api.trace.TraceFlags;
2727
import io.opentelemetry.api.trace.TraceState;
28+
import io.opentelemetry.sdk.trace.ReadableSpan;
2829
import javax.annotation.Nullable;
2930

3031
/**
@@ -34,28 +35,38 @@
3435
*/
3536
public class TraceContext implements Recyclable {
3637

37-
public static final int SERIALIZED_LENGTH = 16 + 8 + 1 + 8;
38+
public static final int SERIALIZED_LENGTH = 16 + 8 + 1 + 1 + 8 + 8;
3839
private long traceIdLow;
3940
private long traceIdHigh;
4041
private long id;
42+
43+
private boolean hasParentId;
44+
private long parentId;
4145
private byte flags;
4246

4347
private long clockAnchor;
4448

4549
public TraceContext() {}
4650

4751
// For testing only
48-
static TraceContext fromSpanContextWithZeroClockAnchor(SpanContext ctx) {
52+
static TraceContext fromSpanContextWithZeroClockAnchor(
53+
SpanContext ctx, @Nullable String parentSpanId) {
4954
TraceContext result = new TraceContext();
50-
result.filLFromSpanContext(ctx);
55+
result.fillFromSpanContext(ctx, parentSpanId);
5156
result.clockAnchor = 0L;
5257
return result;
5358
}
5459

55-
private void filLFromSpanContext(SpanContext ctx) {
60+
private void fillFromSpanContext(SpanContext ctx, @Nullable String parentSpanId) {
5661
id = HexUtils.hexToLong(ctx.getSpanId(), 0);
5762
traceIdHigh = HexUtils.hexToLong(ctx.getTraceId(), 0);
5863
traceIdLow = HexUtils.hexToLong(ctx.getTraceId(), 16);
64+
if (parentSpanId != null) {
65+
hasParentId = true;
66+
parentId = HexUtils.hexToLong(parentSpanId, 0);
67+
} else {
68+
hasParentId = false;
69+
}
5970
flags = ctx.getTraceFlags().asByte();
6071
}
6172

@@ -73,6 +84,10 @@ public SpanContext toOtelSpanContext(StringBuilder temporaryBuilder) {
7384
traceIdStr, idStr, TraceFlags.fromByte(flags), TraceState.getDefault());
7485
}
7586

87+
public long getSpanId() {
88+
return id;
89+
}
90+
7691
public boolean idEquals(@Nullable TraceContext o) {
7792
if (o == null) {
7893
return false;
@@ -89,7 +104,17 @@ public void deserialize(byte[] serialized) {
89104
traceIdHigh = ByteUtils.getLong(serialized, 8);
90105
id = ByteUtils.getLong(serialized, 16);
91106
flags = serialized[24];
92-
clockAnchor = ByteUtils.getLong(serialized, 25);
107+
hasParentId = serialized[25] != 0;
108+
parentId = ByteUtils.getLong(serialized, 26);
109+
clockAnchor = ByteUtils.getLong(serialized, 34);
110+
}
111+
112+
public static long getParentId(byte[] serializedTraceContext) {
113+
boolean hasParent = serializedTraceContext[25] != 0;
114+
if (!hasParent) {
115+
return 0L;
116+
}
117+
return ByteUtils.getLong(serializedTraceContext, 26);
93118
}
94119

95120
public boolean traceIdAndIdEquals(byte[] otherSerialized) {
@@ -105,7 +130,13 @@ public boolean traceIdAndIdEquals(byte[] otherSerialized) {
105130
return id == otherId;
106131
}
107132

108-
public static void serialize(SpanContext ctx, long clockAnchor, byte[] buffer) {
133+
public static void serialize(Span span, long clockAnchor, byte[] buffer) {
134+
SpanContext ctx = span.getSpanContext();
135+
SpanContext parentSpanCtx = SpanContext.getInvalid();
136+
if (span instanceof ReadableSpan) {
137+
parentSpanCtx = ((ReadableSpan) span).getParentSpanContext();
138+
}
139+
109140
long id = HexUtils.hexToLong(ctx.getSpanId(), 0);
110141
long traceIdHigh = HexUtils.hexToLong(ctx.getTraceId(), 0);
111142
long traceIdLow = HexUtils.hexToLong(ctx.getTraceId(), 16);
@@ -114,15 +145,29 @@ public static void serialize(SpanContext ctx, long clockAnchor, byte[] buffer) {
114145
ByteUtils.putLong(buffer, 8, traceIdHigh);
115146
ByteUtils.putLong(buffer, 16, id);
116147
buffer[24] = flags;
117-
ByteUtils.putLong(buffer, 25, clockAnchor);
148+
if (parentSpanCtx.isValid()) {
149+
buffer[25] = 1;
150+
ByteUtils.putLong(buffer, 26, HexUtils.hexToLong(parentSpanCtx.getSpanId(), 0));
151+
} else {
152+
buffer[25] = 0;
153+
ByteUtils.putLong(buffer, 26, 0);
154+
}
155+
ByteUtils.putLong(buffer, 34, clockAnchor);
118156
}
119157

120158
public void serialize(byte[] buffer) {
121159
ByteUtils.putLong(buffer, 0, traceIdLow);
122160
ByteUtils.putLong(buffer, 8, traceIdHigh);
123161
ByteUtils.putLong(buffer, 16, id);
124162
buffer[24] = flags;
125-
ByteUtils.putLong(buffer, 25, clockAnchor);
163+
if (hasParentId) {
164+
buffer[25] = 1;
165+
ByteUtils.putLong(buffer, 26, parentId);
166+
} else {
167+
buffer[25] = 0;
168+
ByteUtils.putLong(buffer, 26, 0);
169+
}
170+
ByteUtils.putLong(buffer, 34, clockAnchor);
126171
}
127172

128173
public byte[] serialize() {

0 commit comments

Comments
 (0)