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

Add support to use SemanticAttributes with Span.Builder. #1076

Closed
Show file tree
Hide file tree
Changes from 1 commit
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
34 changes: 34 additions & 0 deletions api/src/main/java/io/opentelemetry/common/Acceptor.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
/*
* Copyright 2020, OpenTelemetry Authors
*
* Licensed 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 io.opentelemetry.common;

/**
* A function to be executed with a single argument, having a side effect.
*
* <p>Adapted from Java 8's {@code java.util.function.Consumer}.
Copy link
Contributor

Choose a reason for hiding this comment

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

They may be to call it Consumer as well?

Copy link
Member Author

Choose a reason for hiding this comment

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

Oracle and Copyright made me think that it might be better to have a different name.

*
* @param <T> Type of the input argument.
*/
public interface Acceptor<T> {

/**
* Performs this operation on the given argument.
*
* @param arg the input argument
*/
void exec(T arg);
Copy link
Contributor

Choose a reason for hiding this comment

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

On an interface called Acceptor, I'd expect this method to be called accept.

Copy link
Contributor

Choose a reason for hiding this comment

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

thinking out loud here, but what if we had this return T, rather than void, making it feel more like a way to extend the builder fluent nature?

}
7 changes: 7 additions & 0 deletions api/src/main/java/io/opentelemetry/trace/DefaultTracer.java
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@

package io.opentelemetry.trace;

import io.opentelemetry.common.Acceptor;
import io.opentelemetry.common.AttributeValue;
import io.opentelemetry.context.Scope;
import io.opentelemetry.internal.Utils;
Expand Down Expand Up @@ -157,6 +158,12 @@ public NoopSpanBuilder setStartTimestamp(long startTimestamp) {
return this;
}

@Override
public Span.Builder apply(Acceptor<Span.Builder> acceptor) {
acceptor.exec(this);
return this;
}

private NoopSpanBuilder(String name) {
Utils.checkNotNull(name, "name");
}
Expand Down
12 changes: 12 additions & 0 deletions api/src/main/java/io/opentelemetry/trace/Span.java
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@

package io.opentelemetry.trace;

import io.opentelemetry.common.Acceptor;
import io.opentelemetry.common.AttributeValue;
import java.util.Map;
import javax.annotation.Nullable;
Expand Down Expand Up @@ -544,6 +545,17 @@ interface Builder {
*/
Builder setStartTimestamp(long startTimestamp);

/**
* Applies the given consumer to this {@link Builder}.
Copy link
Contributor

Choose a reason for hiding this comment

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

should probably update the doc to call it an acceptor, rather than consumer.

*
* <p>This is indented to be used with {@link
Oberon00 marked this conversation as resolved.
Show resolved Hide resolved
* io.opentelemetry.trace.attributes.StringAttributeSetter#set(String)}, etc.
*
* @param acceptor The {@link Acceptor} to apply to this
* @return this.
*/
Builder apply(Acceptor<Builder> acceptor);

/**
* Starts a new {@link Span}.
*
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@

package io.opentelemetry.trace.attributes;

import io.opentelemetry.common.Acceptor;
import io.opentelemetry.trace.Span;
import javax.annotation.concurrent.Immutable;

Expand Down Expand Up @@ -60,4 +61,21 @@ public String key() {
public void set(Span span, boolean value) {
span.setAttribute(key(), value);
}

/**
* Returns a {@link Acceptor} for use set {@link Span.Builder#apply(Acceptor)} that sets the
* attribute to {@code value}.
Oberon00 marked this conversation as resolved.
Show resolved Hide resolved
*
* @param value the value for this attribute
* @return A {@link Acceptor} for use set {@link Span.Builder#apply(Acceptor)}.
*/
@SuppressWarnings("InconsistentOverloads")
public Acceptor<Span.Builder> set(final boolean value) {
return new Acceptor<Span.Builder>() {
@Override
public void exec(Span.Builder arg) {
Copy link
Contributor

Choose a reason for hiding this comment

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

rename the arg to builder?

arg.setAttribute(key(), value);
}
};
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@

package io.opentelemetry.trace.attributes;

import io.opentelemetry.common.Acceptor;
import io.opentelemetry.trace.Span;
import javax.annotation.concurrent.Immutable;

Expand Down Expand Up @@ -60,4 +61,21 @@ public String key() {
public void set(Span span, double value) {
span.setAttribute(key(), value);
}

/**
* Returns a {@link Acceptor} for use set {@link Span.Builder#apply(Acceptor)} that sets the
Oberon00 marked this conversation as resolved.
Show resolved Hide resolved
* attribute to {@code value}.
*
* @param value the value for this attribute
* @return A {@link Acceptor} for use set {@link Span.Builder#apply(Acceptor)}.
*/
@SuppressWarnings("InconsistentOverloads")
public Acceptor<Span.Builder> set(final double value) {
return new Acceptor<Span.Builder>() {
@Override
public void exec(Span.Builder arg) {
Copy link
Contributor

Choose a reason for hiding this comment

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

rename arg to builder

arg.setAttribute(key(), value);
}
};
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@

package io.opentelemetry.trace.attributes;

import io.opentelemetry.common.Acceptor;
import io.opentelemetry.trace.Span;
import javax.annotation.concurrent.Immutable;

Expand Down Expand Up @@ -60,4 +61,21 @@ public String key() {
public void set(Span span, long value) {
span.setAttribute(key(), value);
}

/**
* Returns a {@link Acceptor} for use set {@link Span.Builder#apply(Acceptor)} that sets the
Oberon00 marked this conversation as resolved.
Show resolved Hide resolved
* attribute to {@code value}.
*
* @param value the value for this attribute
* @return A {@link Acceptor} for use set {@link Span.Builder#apply(Acceptor)}.
*/
@SuppressWarnings("InconsistentOverloads")
public Acceptor<Span.Builder> set(final long value) {
return new Acceptor<Span.Builder>() {
@Override
public void exec(Span.Builder arg) {
Copy link
Contributor

Choose a reason for hiding this comment

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

you guessed it, rename arg to builder

arg.setAttribute(key(), value);
}
};
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@

package io.opentelemetry.trace.attributes;

import io.opentelemetry.common.Acceptor;
import io.opentelemetry.trace.Span;
import javax.annotation.Nullable;
import javax.annotation.concurrent.Immutable;
Expand Down Expand Up @@ -61,4 +62,21 @@ public String key() {
public void set(Span span, @Nullable String value) {
span.setAttribute(key(), value);
}

/**
* Returns a {@link Acceptor} for use set {@link Span.Builder#apply(Acceptor)} that sets the
Oberon00 marked this conversation as resolved.
Show resolved Hide resolved
* attribute to {@code value}.
*
* @param value the value for this attribute
* @return A {@link Acceptor} for use set {@link Span.Builder#apply(Acceptor)}.
*/
@SuppressWarnings("InconsistentOverloads")
public Acceptor<Span.Builder> set(@Nullable final String value) {
return new Acceptor<Span.Builder>() {
@Override
public void exec(Span.Builder arg) {
Copy link
Contributor

Choose a reason for hiding this comment

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

ditto, rename

arg.setAttribute(key(), value);
}
};
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
package io.opentelemetry.trace.attributes;

import static com.google.common.truth.Truth.assertThat;
import static io.opentelemetry.trace.attributes.SemanticAttributes.HTTP_METHOD;

import io.opentelemetry.OpenTelemetry;
import io.opentelemetry.trace.Span;
Expand All @@ -33,16 +34,17 @@
@RunWith(JUnit4.class)
public class SemanticAttributesTest {

private Span span;
private Span.Builder spanBuilder;

@Before
public void setUp() {
Tracer tracer = OpenTelemetry.getTracerProvider().get("io.telemetry.api");
span = tracer.spanBuilder("junit").startSpan();
spanBuilder = tracer.spanBuilder("junit");
}

@Test
public void shouldEnableSetAttributeOnSpan() throws IllegalAccessException {
Span span = spanBuilder.startSpan();
Set<String> keys = new HashSet<>();
Field[] fields = SemanticAttributes.class.getFields();
for (Field field : fields) {
Expand All @@ -65,6 +67,35 @@ public void shouldEnableSetAttributeOnSpan() throws IllegalAccessException {
assertThat(keys.size()).isEqualTo(fields.length);
}

@Test
public void shouldEnableSetAttributeOnSpanBuilder() throws IllegalAccessException {
Set<String> keys = new HashSet<>();
Field[] fields = SemanticAttributes.class.getFields();
for (Field field : fields) {
Object attribute = field.get(null);
if (attribute instanceof StringAttributeSetter) {
keys.add(((StringAttributeSetter) attribute).key());
spanBuilder.apply(((StringAttributeSetter) attribute).set("TestValue"));
spanBuilder.apply(((StringAttributeSetter) attribute).set(null));
} else if (attribute instanceof LongAttributeSetter) {
keys.add(((LongAttributeSetter) attribute).key());
spanBuilder.apply(((LongAttributeSetter) attribute).set(42));
} else if (attribute instanceof DoubleAttributeSetter) {
keys.add(((DoubleAttributeSetter) attribute).key());
spanBuilder.apply(((DoubleAttributeSetter) attribute).set(3.14));
} else if (attribute instanceof BooleanAttributeSetter) {
keys.add(((BooleanAttributeSetter) attribute).key());
spanBuilder.apply(((BooleanAttributeSetter) attribute).set(true));
}
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe add an else that calls fail, so that this test will highlight something not being covered?

Copy link
Member Author

Choose a reason for hiding this comment

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

IMHO the test for both this and the old SemConv API needs quite some improvements (such as testing that setAttribute was actually called, which is unfortunately not possible with only the API due to Mockito not supporting final classes). But I just copied & adapted the tests for the set method for now.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd be all in favor of making those classes non-final!

}
assertThat(keys.size()).isEqualTo(fields.length);
}

@Test
public void setBuilderDemo() {
spanBuilder.apply(HTTP_METHOD.set("GET"));
}

@Test
public void shouldCreateAllSetterTypes() {
assertThat(BooleanAttributeSetter.create("attr.one"))
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
package io.opentelemetry.sdk.trace;

import io.grpc.Context;
import io.opentelemetry.common.Acceptor;
import io.opentelemetry.common.AttributeValue;
import io.opentelemetry.internal.Utils;
import io.opentelemetry.sdk.common.Clock;
Expand Down Expand Up @@ -198,6 +199,12 @@ public Span.Builder setStartTimestamp(long startTimestamp) {
return this;
}

@Override
public Span.Builder apply(Acceptor<Span.Builder> acceptor) {
acceptor.exec(this);
return this;
}

@Override
public Span startSpan() {
SpanContext parentContext = parent(parentType, parent, remoteParent);
Expand Down