-
Notifications
You must be signed in to change notification settings - Fork 17
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Capture span stack trace #21
Changes from all commits
1e8a830
d41665f
04d243a
b4e7d71
78f017a
9902f4a
3486d52
ea2db77
32d54b4
aa4542b
5cbc0f8
7cf32fc
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -37,21 +37,21 @@ | |
|
||
public class ElasticBreakdownMetrics { | ||
|
||
private final ConcurrentHashMap<SpanContext, SpanContextData> elasticSpanData; | ||
private final ConcurrentHashMap<SpanContext, BreakdownData> elasticSpanData; | ||
|
||
private ElasticSpanExporter spanExporter; | ||
|
||
private LongCounter breakDownCounter; | ||
|
||
// sidecar object we store for every span | ||
public static class SpanContextData { | ||
private static class BreakdownData { | ||
|
||
private ReadableSpan localRoot; | ||
|
||
private final ChildDuration childDuration; | ||
private long selfTime; | ||
|
||
public SpanContextData(ReadableSpan localRoot, long start) { | ||
public BreakdownData(ReadableSpan localRoot, long start) { | ||
this.localRoot = localRoot; | ||
this.childDuration = new ChildDuration(start); | ||
this.selfTime = Long.MIN_VALUE; | ||
|
@@ -108,7 +108,7 @@ public void onSpanStart(Context parentContext, ReadWriteSpan span) { | |
// the span is a local root span | ||
localRootSpanContext = spanContext; | ||
|
||
elasticSpanData.put(spanContext, new SpanContextData(span, spanStart)); | ||
elasticSpanData.put(spanContext, new BreakdownData(span, spanStart)); | ||
|
||
} else { | ||
ReadableSpan parentSpan = getReadableSpanFromContext(parentContext); | ||
|
@@ -118,7 +118,7 @@ public void onSpanStart(Context parentContext, ReadWriteSpan span) { | |
ReadableSpan localRoot = lookupLocalRootSpan(parentSpan); | ||
localRootSpanContext = localRoot.getSpanContext(); | ||
if (localRootSpanContext.isValid()) { | ||
elasticSpanData.put(spanContext, new SpanContextData(localRoot, spanStart)); | ||
elasticSpanData.put(spanContext, new BreakdownData(localRoot, spanStart)); | ||
} | ||
|
||
// update direct parent span child durations for self-time | ||
|
@@ -157,11 +157,11 @@ public void onSpanEnd(ReadableSpan span) { | |
SpanData spanData = span.toSpanData(); | ||
|
||
// children duration for current span | ||
SpanContextData spanContextData = elasticSpanData.get(spanContext); | ||
BreakdownData spanContextData = elasticSpanData.get(spanContext); | ||
Objects.requireNonNull(spanContextData, "missing elastic span data"); | ||
|
||
// update children duration for direct parent | ||
SpanContextData parentSpanContextData = elasticSpanData.get(span.getParentSpanContext()); | ||
BreakdownData parentSpanContextData = elasticSpanData.get(span.getParentSpanContext()); | ||
|
||
if (parentSpanContextData != null) { // parent might be already terminated | ||
parentSpanContextData.childDuration.endChild(spanData.getEndEpochNanos()); | ||
|
@@ -176,14 +176,15 @@ public void onSpanEnd(ReadableSpan span) { | |
// put measured metric as span attribute to allow using an ingest pipeline to alter | ||
// storage | ||
// ingest pipelines do not have access to _source and thus can't read the metric as-is. | ||
.put(ElasticAttributes.SELF_TIME_ATTRIBUTE, selfTime); | ||
.put(ElasticAttributes.SELF_TIME, selfTime); | ||
|
||
// unfortunately here we get a read-only span that has already been ended, thus even a cast to | ||
// ReadWriteSpan | ||
// does not allow us from adding extra span attributes | ||
if (spanExporter != null) { | ||
spanContextData.setSelfTime(selfTime); | ||
spanExporter.report(spanContext, spanContextData); | ||
spanExporter.addAttribute( | ||
spanContext, ElasticAttributes.SELF_TIME, spanContextData.getSelfTime()); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [for reviewer] this is the only change in this file, the rest is mostly renaming things. |
||
} | ||
|
||
breakDownCounter.add(selfTime, metricAttributes.build()); | ||
|
@@ -194,25 +195,25 @@ private static AttributesBuilder buildCounterAttributes(Attributes spanAttribute | |
AttributesBuilder builder = | ||
Attributes.builder() | ||
// default to app/internal unless other span attributes | ||
.put(ElasticAttributes.ELASTIC_SPAN_TYPE, "app") | ||
.put(ElasticAttributes.ELASTIC_SPAN_SUBTYPE, "internal"); | ||
.put(ElasticAttributes.SPAN_TYPE, "app") | ||
.put(ElasticAttributes.SPAN_SUBTYPE, "internal"); | ||
|
||
spanAttributes.forEach( | ||
(k, v) -> { | ||
String key = k.getKey(); | ||
if (AttributeType.STRING.equals(k.getType())) { | ||
int index = key.indexOf(".system"); | ||
if (index > 0) { | ||
builder.put(ElasticAttributes.ELASTIC_SPAN_TYPE, key.substring(0, index)); | ||
builder.put(ElasticAttributes.ELASTIC_SPAN_SUBTYPE, v.toString()); | ||
builder.put(ElasticAttributes.SPAN_TYPE, key.substring(0, index)); | ||
builder.put(ElasticAttributes.SPAN_SUBTYPE, v.toString()); | ||
} | ||
} | ||
}); | ||
return builder; | ||
} | ||
|
||
private ReadableSpan lookupLocalRootSpan(ReadableSpan span) { | ||
SpanContextData spanContextData = elasticSpanData.get(span.getSpanContext()); | ||
BreakdownData spanContextData = elasticSpanData.get(span.getSpanContext()); | ||
return spanContextData != null ? spanContextData.localRoot : (ReadableSpan) Span.getInvalid(); | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -23,11 +23,14 @@ | |
import io.opentelemetry.sdk.trace.ReadWriteSpan; | ||
import io.opentelemetry.sdk.trace.ReadableSpan; | ||
import io.opentelemetry.sdk.trace.SpanProcessor; | ||
import java.io.PrintWriter; | ||
import java.io.StringWriter; | ||
|
||
public class ElasticSpanProcessor implements SpanProcessor { | ||
|
||
private final ElasticProfiler profiler; | ||
private final ElasticBreakdownMetrics breakdownMetrics; | ||
private ElasticSpanExporter spanExporter; | ||
|
||
public ElasticSpanProcessor(ElasticProfiler profiler, ElasticBreakdownMetrics breakdownMetrics) { | ||
this.profiler = profiler; | ||
|
@@ -49,6 +52,8 @@ public boolean isStartRequired() { | |
public void onEnd(ReadableSpan span) { | ||
profiler.onSpanEnd(span); | ||
breakdownMetrics.onSpanEnd(span); | ||
|
||
captureStackTrace(span); | ||
} | ||
|
||
@Override | ||
|
@@ -61,4 +66,28 @@ public CompletableResultCode shutdown() { | |
profiler.shutdown(); | ||
return CompletableResultCode.ofSuccess(); | ||
} | ||
|
||
public void registerSpanExporter(ElasticSpanExporter spanExporter) { | ||
this.spanExporter = spanExporter; | ||
} | ||
|
||
private void captureStackTrace(ReadableSpan span) { | ||
if (spanExporter == null) { | ||
return; | ||
} | ||
|
||
// do not overwrite stacktrace if present | ||
if (span.getAttribute(ElasticAttributes.SPAN_STACKTRACE) == null) { | ||
Throwable exception = new Throwable(); | ||
StringWriter stringWriter = new StringWriter(); | ||
try (PrintWriter printWriter = new PrintWriter(stringWriter)) { | ||
exception.printStackTrace(printWriter); | ||
} | ||
|
||
// TODO should we filter-out the calling code that is within the agent: at least onEnd + | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [for reviewer] I don't know if any filtering on the stack trace is applied here, thus I'll leave it as-it for now and wait to have the kibana implementation to see if any filtering is applied on the UI side. |
||
// captureStackTrace will be included here | ||
spanExporter.addAttribute( | ||
span.getSpanContext(), ElasticAttributes.SPAN_STACKTRACE, stringWriter.toString()); | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,91 @@ | ||
/* | ||
* 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.otel; | ||
|
||
import static io.opentelemetry.sdk.testing.assertj.OpenTelemetryAssertions.assertThat; | ||
import static io.opentelemetry.sdk.testing.assertj.OpenTelemetryAssertions.satisfies; | ||
import static org.mockito.Mockito.mock; | ||
|
||
import io.opentelemetry.api.trace.Tracer; | ||
import io.opentelemetry.sdk.testing.exporter.InMemorySpanExporter; | ||
import io.opentelemetry.sdk.trace.SdkTracerProvider; | ||
import io.opentelemetry.sdk.trace.export.SimpleSpanProcessor; | ||
import org.assertj.core.api.AbstractCharSequenceAssert; | ||
import org.junit.jupiter.api.BeforeEach; | ||
import org.junit.jupiter.api.Test; | ||
|
||
public class ElasticSpanProcessorTest { | ||
|
||
private static final Tracer tracer; | ||
private static final InMemorySpanExporter testExporter; | ||
|
||
static { | ||
ElasticSpanProcessor elasticSpanProcessor = | ||
new ElasticSpanProcessor(mock(ElasticProfiler.class), mock(ElasticBreakdownMetrics.class)); | ||
|
||
testExporter = InMemorySpanExporter.create(); | ||
ElasticSpanExporter elasticSpanExporter = new ElasticSpanExporter(testExporter); | ||
elasticSpanProcessor.registerSpanExporter(elasticSpanExporter); | ||
|
||
tracer = | ||
SdkTracerProvider.builder() | ||
.addSpanProcessor(elasticSpanProcessor) | ||
.addSpanProcessor(SimpleSpanProcessor.create(elasticSpanExporter)) | ||
Comment on lines
+48
to
+49
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [for reviewer] this emulates how our exporter is usually set up in the agent: first our exporter that modifies spans, then another that exports it. Here we are using the simple exporter that does not use batching and is synchronous. |
||
.build() | ||
.get("for-testing"); | ||
} | ||
|
||
@BeforeEach | ||
public void before() { | ||
testExporter.reset(); | ||
} | ||
|
||
@Test | ||
void spanStackTraceCapture() { | ||
tracer.spanBuilder("span").startSpan().end(); | ||
|
||
assertThat(testExporter.getFinishedSpanItems()) | ||
.hasSize(1) | ||
.first() | ||
.satisfies( | ||
spanData -> | ||
assertThat(spanData) | ||
.hasAttributesSatisfying( | ||
satisfies( | ||
ElasticAttributes.SPAN_STACKTRACE, | ||
AbstractCharSequenceAssert::isNotEmpty))); | ||
Comment on lines
+63
to
+72
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [for reviewer] otel test utilities provide the assertj extension for easily testing for custom attributes. |
||
} | ||
|
||
@Test | ||
void spanStackTraceCaptureDoesNotOverwrite() { | ||
String value = "dummy"; | ||
tracer | ||
.spanBuilder("span") | ||
.setAttribute(ElasticAttributes.SPAN_STACKTRACE, value) | ||
.startSpan() | ||
.end(); | ||
|
||
assertThat(testExporter.getFinishedSpanItems()) | ||
.hasSize(1) | ||
.first() | ||
.satisfies( | ||
spanData -> | ||
assertThat(spanData).hasAttribute(ElasticAttributes.SPAN_STACKTRACE, value)); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[for reviewer] this will likely be handled with #8