Skip to content
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

Ignore null or blank string-valued span attributes. #771

Merged
Show file tree
Hide file tree
Changes from all 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
4 changes: 4 additions & 0 deletions api/src/main/java/io/opentelemetry/internal/StringUtils.java
Original file line number Diff line number Diff line change
Expand Up @@ -41,5 +41,9 @@ private static boolean isPrintableChar(char ch) {
return ch >= ' ' && ch <= '~';
}

public static boolean isNullOrBlank(String value) {
return value == null || value.length() == 0;
}

private StringUtils() {}
}
6 changes: 3 additions & 3 deletions api/src/main/java/io/opentelemetry/trace/AttributeValue.java
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@
package io.opentelemetry.trace;

import com.google.auto.value.AutoValue;
import io.opentelemetry.internal.Utils;
import javax.annotation.Nullable;
import javax.annotation.concurrent.Immutable;

/**
Expand Down Expand Up @@ -152,8 +152,7 @@ abstract static class AttributeValueString extends AttributeValue {
AttributeValueString() {}

static AttributeValue create(String stringValue) {
return new AutoValue_AttributeValue_AttributeValueString(
Utils.checkNotNull(stringValue, "stringValue"));
return new AutoValue_AttributeValue_AttributeValueString(stringValue);
}

@Override
Expand All @@ -162,6 +161,7 @@ public final Type getType() {
}

@Override
@Nullable
public abstract String getStringValue();
}

Expand Down
1 change: 0 additions & 1 deletion api/src/main/java/io/opentelemetry/trace/DefaultSpan.java
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,6 @@ static DefaultSpan createRandom() {
@Override
public void setAttribute(String key, String value) {
Utils.checkNotNull(key, "key");
Utils.checkNotNull(value, "value");
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -134,7 +134,6 @@ public NoopSpanBuilder addLink(Link link) {
@Override
public NoopSpanBuilder setAttribute(String key, String value) {
Utils.checkNotNull(key, "key");
Utils.checkNotNull(value, "value");
return this;
}

Expand Down
14 changes: 11 additions & 3 deletions api/src/main/java/io/opentelemetry/trace/Span.java
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
package io.opentelemetry.trace;

import java.util.Map;
import javax.annotation.Nullable;
import javax.annotation.concurrent.ThreadSafe;

/**
Expand All @@ -30,6 +31,7 @@
*/
@ThreadSafe
public interface Span {

/**
* Type of span. Can be used to specify additional relationships between spans in addition to a
* parent/child relationship.
Expand Down Expand Up @@ -81,11 +83,14 @@ enum Kind {
* Sets an attribute to the {@code Span}. If the {@code Span} previously contained a mapping for
* the key, the old value is replaced by the specified value.
*
* <p>If a null or empty String {@code value} is passed in, the attribute will be silently
* dropped. Note: this behavior could change in the future.
*
* @param key the key for this attribute.
* @param value the value for this attribute.
* @since 0.1.0
*/
void setAttribute(String key, String value);
void setAttribute(String key, @Nullable String value);

/**
* Sets an attribute to the {@code Span}. If the {@code Span} previously contained a mapping for
Expand Down Expand Up @@ -354,6 +359,7 @@ enum Kind {
* @since 0.1.0
*/
interface Builder {

/**
* Sets the parent {@code Span} to use. If not set, the value of {@code Tracer.getCurrentSpan()}
* at {@link #startSpan()} time will be used as parent.
Expand Down Expand Up @@ -452,14 +458,16 @@ interface Builder {
* Sets an attribute to the newly created {@code Span}. If {@code Span.Builder} previously
* contained a mapping for the key, the old value is replaced by the specified value.
*
* <p>If a null or empty String {@code value} is passed in, the attribute will be silently
* dropped. Note: this behavior could change in the future.
*
* @param key the key for this attribute.
* @param value the value for this attribute.
* @return this.
* @throws NullPointerException if {@code key} is {@code null}.
* @throws NullPointerException if {@code value} is {@code null}.
* @since 0.3.0
*/
Builder setAttribute(String key, String value);
Builder setAttribute(String key, @Nullable String value);

/**
* Sets an attribute to the newly created {@code Span}. If {@code Span.Builder} previously
Expand Down
14 changes: 10 additions & 4 deletions api/src/test/java/io/opentelemetry/internal/StringUtilsTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,7 @@

package io.opentelemetry.internal;

import static org.junit.Assert.assertFalse;
import static org.junit.Assert.assertTrue;
import static com.google.common.truth.Truth.assertThat;

import org.junit.Test;
import org.junit.runner.RunWith;
Expand All @@ -29,7 +28,14 @@ public final class StringUtilsTest {

@Test
public void isPrintableString() {
assertTrue(StringUtils.isPrintableString("abcd"));
assertFalse(StringUtils.isPrintableString("\2ab\3cd"));
assertThat(StringUtils.isPrintableString("abcd")).isTrue();
assertThat(StringUtils.isPrintableString("\2ab\3cd")).isFalse();
}

@Test
public void isNullOrEmpty() throws Exception {
assertThat(StringUtils.isNullOrBlank("")).isTrue();
assertThat(StringUtils.isNullOrBlank(null)).isTrue();
assertThat(StringUtils.isNullOrBlank("hello")).isFalse();
}
}
2 changes: 2 additions & 0 deletions api/src/test/java/io/opentelemetry/trace/DefaultSpanTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,8 @@ public void doNotCrash() {
"MyStringAttributeKey", AttributeValue.stringAttributeValue("MyStringAttributeValue"));
span.setAttribute("MyBooleanAttributeKey", AttributeValue.booleanAttributeValue(true));
span.setAttribute("MyLongAttributeKey", AttributeValue.longAttributeValue(123));
span.setAttribute("NullString", (String) null);
span.setAttribute("EmptyString", "");
span.addEvent("event");
span.addEvent("event", 0);
span.addEvent(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,11 +18,13 @@

import com.google.common.base.Preconditions;
import com.google.common.collect.EvictingQueue;
import io.opentelemetry.internal.StringUtils;
import io.opentelemetry.sdk.common.Clock;
import io.opentelemetry.sdk.common.InstrumentationLibraryInfo;
import io.opentelemetry.sdk.resources.Resource;
import io.opentelemetry.sdk.trace.config.TraceConfig;
import io.opentelemetry.trace.AttributeValue;
import io.opentelemetry.trace.AttributeValue.Type;
import io.opentelemetry.trace.EndSpanOptions;
import io.opentelemetry.trace.Event;
import io.opentelemetry.trace.Link;
Expand Down Expand Up @@ -325,6 +327,9 @@ public void setAttribute(String key, boolean value) {
public void setAttribute(String key, AttributeValue value) {
Preconditions.checkNotNull(key, "key");
Preconditions.checkNotNull(value, "value");
if (value.getType() == Type.STRING && StringUtils.isNullOrBlank(value.getStringValue())) {
return;
}
synchronized (lock) {
if (hasEnded) {
logger.log(Level.FINE, "Calling setAttribute() on an ended Span.");
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@

package io.opentelemetry.sdk.trace;

import io.opentelemetry.internal.StringUtils;
import io.opentelemetry.internal.Utils;
import io.opentelemetry.sdk.common.Clock;
import io.opentelemetry.sdk.common.InstrumentationLibraryInfo;
Expand All @@ -24,6 +25,7 @@
import io.opentelemetry.sdk.trace.Sampler.Decision;
import io.opentelemetry.sdk.trace.config.TraceConfig;
import io.opentelemetry.trace.AttributeValue;
import io.opentelemetry.trace.AttributeValue.Type;
import io.opentelemetry.trace.DefaultSpan;
import io.opentelemetry.trace.Link;
import io.opentelemetry.trace.Span;
Expand Down Expand Up @@ -159,6 +161,9 @@ public Span.Builder setAttribute(String key, boolean value) {
public Span.Builder setAttribute(String key, AttributeValue value) {
Utils.checkNotNull(key, "key");
Utils.checkNotNull(value, "value");
if (value.getType() == Type.STRING && StringUtils.isNullOrBlank(value.getStringValue())) {
return this;
}
attributes.putAttribute(key, value);
return this;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -286,6 +286,10 @@ public void setAttribute() {
RecordEventsReadableSpan span = createTestRootSpan();
try {
span.setAttribute("StringKey", "StringVal");
span.setAttribute("NullStringKey", (String) null);
span.setAttribute("EmptyStringKey", "");
span.setAttribute("NullStringAttributeValue", AttributeValue.stringAttributeValue(null));
span.setAttribute("EmptyStringAttributeValue", AttributeValue.stringAttributeValue(""));
span.setAttribute("LongKey", 1000L);
span.setAttribute("DoubleKey", 10.0);
span.setAttribute("BooleanKey", false);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -169,6 +169,17 @@ public void setAttribute() {
}
}

@Test
public void setAttribute_nullStringValue() throws Exception {
Span.Builder spanBuilder = tracerSdk.spanBuilder(SPAN_NAME);
spanBuilder.setAttribute("emptyString", "");
spanBuilder.setAttribute("nullString", (String) null);
spanBuilder.setAttribute("nullStringAttributeValue", AttributeValue.stringAttributeValue(null));
spanBuilder.setAttribute("emptyStringAttributeValue", AttributeValue.stringAttributeValue(""));
RecordEventsReadableSpan span = (RecordEventsReadableSpan) spanBuilder.startSpan();
assertThat(span.toSpanData().getAttributes()).isEmpty();
}

@Test
public void droppingAttributes() {
final int maxNumberOfAttrs = 8;
Expand Down