Skip to content
Open
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
import java.util.Map;
import java.util.function.BiConsumer;
import java.util.function.Predicate;
import javax.annotation.Nullable;

/**
* This class is internal and is hence not for public use. Its APIs are unstable and can change at
Expand Down Expand Up @@ -107,13 +108,19 @@ public static Object applyAttributeLengthLimit(Object value, int lengthLimit) {
}

public static void addExceptionAttributes(
Throwable exception, BiConsumer<AttributeKey<String>, String> attributeConsumer) {
BiConsumer<AttributeKey<String>, String> attributeConsumer, Throwable exception) {
addExceptionAttributes(attributeConsumer, exception, exception.getMessage());
}

public static void addExceptionAttributes(
BiConsumer<AttributeKey<String>, String> attributeConsumer,
Throwable exception,
@Nullable String exceptionMessage) {
String exceptionType = exception.getClass().getCanonicalName();
if (exceptionType != null) {
attributeConsumer.accept(EXCEPTION_TYPE, exceptionType);
}

String exceptionMessage = exception.getMessage();
if (exceptionMessage != null) {
attributeConsumer.accept(EXCEPTION_MESSAGE, exceptionMessage);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ SdkLogRecordBuilder setException(Throwable throwable) {
return this;
}

AttributeUtil.addExceptionAttributes(throwable, this::setAttribute);
AttributeUtil.addExceptionAttributes(this::setAttribute, throwable);

return this;
}
Expand Down
14 changes: 3 additions & 11 deletions sdk/trace/src/main/java/io/opentelemetry/sdk/trace/SdkSpan.java
Original file line number Diff line number Diff line change
Expand Up @@ -20,11 +20,11 @@
import io.opentelemetry.sdk.internal.InstrumentationScopeUtil;
import io.opentelemetry.sdk.resources.Resource;
import io.opentelemetry.sdk.trace.data.EventData;
import io.opentelemetry.sdk.trace.data.ExceptionEventData;
import io.opentelemetry.sdk.trace.data.LinkData;
import io.opentelemetry.sdk.trace.data.SpanData;
import io.opentelemetry.sdk.trace.data.StatusData;
import io.opentelemetry.sdk.trace.internal.ExtendedSpanProcessor;
import io.opentelemetry.sdk.trace.internal.data.LazyExceptionEventData;
import java.util.ArrayList;
import java.util.Collections;
import java.util.List;
Expand Down Expand Up @@ -465,17 +465,9 @@ public ReadWriteSpan recordException(Throwable exception, Attributes additionalA
additionalAttributes = Attributes.empty();
}

AttributesMap attributes =
AttributesMap.create(
spanLimits.getMaxNumberOfAttributes(), spanLimits.getMaxAttributeValueLength());

AttributeUtil.addExceptionAttributes(exception, attributes::put);

additionalAttributes.forEach(attributes::put);

addTimedEvent(
ExceptionEventData.create(
clock.now(), exception, attributes, attributes.getTotalAddedValues()));
LazyExceptionEventData.create(clock.now(), exception, additionalAttributes, spanLimits));

return this;
}

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,116 @@
/*
* Copyright The OpenTelemetry Authors
* SPDX-License-Identifier: Apache-2.0
*/

package io.opentelemetry.sdk.trace.internal.data;

import com.google.auto.value.AutoValue;
import com.google.auto.value.extension.memoized.Memoized;
import io.opentelemetry.api.common.AttributeKey;
import io.opentelemetry.api.common.Attributes;
import io.opentelemetry.sdk.internal.AttributeUtil;
import io.opentelemetry.sdk.internal.AttributesMap;
import io.opentelemetry.sdk.trace.SpanLimits;
import io.opentelemetry.sdk.trace.data.ExceptionEventData;
import java.io.PrintWriter;
import java.io.StringWriter;
import javax.annotation.Nullable;
import javax.annotation.concurrent.Immutable;

/**
* An {@link ExceptionEventData} implementation with {@link #getAttributes()} lazily evaluated,
* allowing the (relatively) expensive exception attribute rendering to take place off the hot path.
*
* <p>This class is internal and is hence not for public use. Its APIs are unstable and can change
* at any time.
*/
@AutoValue
@Immutable
public abstract class LazyExceptionEventData implements ExceptionEventData {

private static final AttributeKey<String> EXCEPTION_TYPE =
AttributeKey.stringKey("exception.type");
private static final AttributeKey<String> EXCEPTION_MESSAGE =
AttributeKey.stringKey("exception.message");
private static final AttributeKey<String> EXCEPTION_STACKTRACE =
AttributeKey.stringKey("exception.stacktrace");
private static final String EXCEPTION_EVENT_NAME = "exception";

@Override
public final String getName() {
return EXCEPTION_EVENT_NAME;
}

// Autowire generates AutoValue_LazyExceptionEventData and $AutoValue_LazyExceptionEventData to
// account to memoize getAttributes. Unfortunately, $AutoValue_LazyExceptionEventData's
// exceptionMessage constructor param is properly annotated with @Nullable, but
// AutoValue_LazyExceptionEventData's is not. So we suppress the NullAway false positive.
@SuppressWarnings("NullAway")
public static ExceptionEventData create(
long epochNanos,
Throwable exception,
Attributes additionalAttributes,
SpanLimits spanLimits) {
// Compute exception message at initialization time to be conservative about possibility of
// Exception#geMessage() not being thread safe
return new AutoValue_LazyExceptionEventData(
epochNanos, exception, spanLimits, exception.getMessage(), additionalAttributes);
}

abstract SpanLimits getSpanLimits();

@Nullable
abstract String getExceptionMessage();

public abstract Attributes getAdditionalAttributes();
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cc @HaloFour I believe this should be a (temporary) workaround to the changes in #6795. To restore the behavior you were relying on, you should be able to:

  • Check if eventData instanceOf LazyExceptionEventData
  • If so, access the attributes via LazyExceptionEventData#getAdditionalAttributes() without rendering the exception.*

I'd like to followup with this PRs change with:

  • Add SDK customization hook for defining your own exception rendering logic
  • Introduce API for setting exception on logs LogRecordBuilder#setException
  • Improve the default exception rendering logic to be attribute limits aware, so that if attributes can't be longer than 1000 characters we don't waste resources rendering the stacktrace past this limit


@Override
@Memoized
public Attributes getAttributes() {
Throwable exception = getException();
SpanLimits spanLimits = getSpanLimits();
Attributes additionalAttributes = getAdditionalAttributes();

AttributesMap attributes =
AttributesMap.create(
spanLimits.getMaxNumberOfAttributes(), spanLimits.getMaxAttributeValueLength());

AttributeUtil.addExceptionAttributes(attributes::put, exception);

additionalAttributes.forEach(attributes::put);

String exceptionName = exception.getClass().getCanonicalName();
String exceptionMessage = getExceptionMessage();
StringWriter stringWriter = new StringWriter();
try (PrintWriter printWriter = new PrintWriter(stringWriter)) {
exception.printStackTrace(printWriter);
}
String stackTrace = stringWriter.toString();

if (exceptionName != null) {
attributes.put(EXCEPTION_TYPE, exceptionName);
}
if (exceptionMessage != null) {
attributes.put(EXCEPTION_MESSAGE, exceptionMessage);
}
if (stackTrace != null) {
attributes.put(EXCEPTION_STACKTRACE, stackTrace);
}

additionalAttributes.forEach(attributes::put);

return attributes.immutableCopy();
}

@Override
public int getTotalAttributeCount() {
// getAttributes() lazily adds 3 attributes to getAdditionalAttributes():
// - exception.type
// - exception.message
// - exception.stacktrace
return getAdditionalAttributes().size() + 3;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for users of this method does it matter that there could be overlap and so this calculation could overcount (e.g. a user overriding exception.message)?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh great point. Need to think this through a little more

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we move this to a Draft until you're ready to get this figured out?

}

LazyExceptionEventData() {}
}
Loading