Add support to use SemanticAttributes with Span.Builder. by Oberon00 · Pull Request #1076 · open-telemetry/opentelemetry-java

Oberon00 · 2020-04-03T13:40:10Z

Suggestion on how to resolve #1075.

This design ensures that the API would not depend on a hypothetical contrib-semantic-attributes package. If we don't care about that, we could add overloads taking *AttributeSetter directly to the Span.Builder interface, without needing the Acceptor indirection.

This was designed with chainability in mind, i.e. you can do things like spanBuilder.setAttribute(foo, bar).apply(ATTR1.set(23)).apply(ATTR2.set(43)).startSpan().

carlosalberto · 2020-04-03T14:57:12Z

What's the disadvantage of adding an overload for each setter to take a SpanBuilder? Besides 'losing' chaining, that is.

// BooleanAttributeSetter
  public void set(Span.Builder builder, boolean value) {
    builder.setAttribute(key(), value);
  }

Oberon00 · 2020-04-03T15:19:43Z

It's only about losing chaining really. Or rather, having uglier chaining, since you can also do public Span.Builder set(Span.Builder builder, boolean value).

jkwatson · 2020-04-03T17:35:51Z

+   *
+   * @param arg the input argument
+   */
+  void exec(T arg);


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

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?

jkwatson · 2020-04-03T17:37:56Z

    Builder setStartTimestamp(long startTimestamp);

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


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

jkwatson · 2020-04-03T17:44:30Z

+  public Acceptor<Span.Builder> set(final boolean value) {
+    return new Acceptor<Span.Builder>() {
+      @Override
+      public void exec(Span.Builder arg) {


rename the arg to builder?

jkwatson · 2020-04-03T17:47:38Z

+  public Acceptor<Span.Builder> set(final double value) {
+    return new Acceptor<Span.Builder>() {
+      @Override
+      public void exec(Span.Builder arg) {


rename arg to builder

jkwatson · 2020-04-03T17:48:09Z

+  public Acceptor<Span.Builder> set(final long value) {
+    return new Acceptor<Span.Builder>() {
+      @Override
+      public void exec(Span.Builder arg) {


you guessed it, rename arg to builder

jkwatson · 2020-04-03T17:49:01Z

+  public Acceptor<Span.Builder> set(@Nullable final String value) {
+    return new Acceptor<Span.Builder>() {
+      @Override
+      public void exec(Span.Builder arg) {


ditto, rename

jkwatson · 2020-04-03T17:50:35Z

+      } else if (attribute instanceof BooleanAttributeSetter) {
+        keys.add(((BooleanAttributeSetter) attribute).key());
+        spanBuilder.apply(((BooleanAttributeSetter) attribute).set(true));
+      }


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

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.

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

Co-Authored-By: John Watson <jkwatson@gmail.com>

carlosalberto · 2020-04-03T18:01:43Z

One more thing to keep in mind is whether we can/should add methods to the public API, i.e. add a new method in Tracer (I remember we removed some Context related things that use to exist there, such as Tracer.withRunnable(Runnable), to decrease API surface, which ended up in tracing_utils).

jkwatson · 2020-04-03T18:06:19Z

I played around with this a bit locally, and I think the set methods on the Setter classes read better if they're renamed to value, like so:

    Builder builder = OpenTelemetry.getTracerProvider().get("").spanBuilder("foo");
    builder
        .apply(SemanticAttributes.DB_ISNTANCE.value("bigDb"))
        .apply(SemanticAttributes.DB_USER.value("username"))
        .apply(SemanticAttributes.DB_STATEMENT.value("select foo from bar"))
        .startSpan();

jkwatson · 2020-04-03T18:12:28Z

One more thing to keep in mind is whether we can/should add methods to the public API, i.e. add a new method in Tracer (I remember we removed some Context related things that use to exist there, such as Tracer.withRunnable(Runnable), to decrease API surface, which ended up in tracing_utils).

Yes, I'd like to play around with this a bit more and see if we can do something similar without increasing the API surface. I like the idea of this change a lot, but am also hesitant to add this to the API.

jkwatson · 2020-04-03T18:32:20Z

Or, without changing the API, we could do something like this (which I know isn't quite as fluent):

    OpenTelemetry.getTracerProvider().get("").spanBuilder("foo")
        .setAttribute(DB_STATEMENT.key(), DB_STATEMENT.value("select foo from bar"))
        .setAttribute(DB_URL.key(), DB_URL.value("jdbc:blah:blah:blah"))
        .setAttribute(DB_USER.key(), DB_USER.value("username"))
        .startSpan();

Oberon00 · 2020-04-04T12:53:33Z

Having to name the attribute twice is a no-go IMHO. You can forget to use the attribute for the value, it still works; use the wrong attribute (different from key, it still works. Also, this is so cumbersome that I fear people will rather use setAttribute after the span is created, just because the syntax looks so much nicer (and in doing so, deprive the sampler of attribute information).

Oberon00 · 2020-04-06T07:08:16Z

WDYT? Should I clean up this PR or do we want to look for & decide on another approach?

trask · 2020-04-06T15:29:14Z

What about:

  public Span.Builder setAttribute(LongSemanticAttribute attr, long value);
  public Span.Builder setAttribute(StringSemanticAttribute attr, String value);

jkwatson · 2020-04-06T15:39:23Z

What about:

  public Span.Builder setAttribute(LongSemanticAttribute attr, long value);
  public Span.Builder setAttribute(StringSemanticAttribute attr, String value);

On which class/interface? Are you proposing adding this to the Span.Builder itself?

trask · 2020-04-06T15:42:10Z

Yes (and something similar on Span also)

jkwatson · 2020-04-06T15:47:32Z

@Oberon00 If we want to do #1081, it seems like we should probably solve both of these issues in one implementation. It seems like adding an Attribute class to replace the AttributeValue would probably get both of these use-cases in a good place, but I'd like to hear what you think about that. Also, @trask , if we were to add an Attribute class which captures key & value, to replace the AttributeValue which is only the value, would that cause the same bridging issue in the agent?

trask · 2020-04-06T15:59:32Z

Yeah, we'd need to bridge that (get the key/value from the Attribute, and create a new Attribute in the embedded SDK).

Oberon00 · 2020-04-06T16:10:09Z

@jkwatson So basically we replace the generic Acceptor with an Attribute and instead of Span.Builder.apply(Acceptor<Span.Builder>) we have Span.Builder.setAttribute(Attribute)? Doesn't really sound like an improvement to me 😕

jkwatson · 2020-04-06T16:17:24Z

@jkwatson So basically we replace the generic Acceptor with an Attribute and instead of Span.Builder.apply(Acceptor<Span.Builder>) we have Span.Builder.setAttribute(Attribute)? Doesn't really sound like an improvement to me 😕

It enables this usage, which you gave a 👍 to above:

OpenTelemetry.getTracerProvider().get("").spanBuilder("foo")
        .setAttribute(SemanticAttributes.DB_STATEMENT.withValue("select foo from bar"))
        .setAttribute(SemanticAttributes.DB_URL.withValue("jdbc:blah:blah:blah"))
        .setAttribute(SemanticAttributes.DB_USER.withValue("username"))
        .startSpan();

Oberon00 · 2020-04-06T16:54:32Z

You are right, I think this design is viable, but after thinking it through, does it offer any benefit over the one in this PR?

Oberon00 · 2020-04-06T16:57:06Z

BTW, in Java 8 with Lambdas and Optional.map, we would have more design possibilities.

jkwatson · 2020-04-06T21:55:24Z

You are right, I think this design is viable, but after thinking it through, does it offer any benefit over the one in this PR?

I almost never cry "performance" when trying to make an API ergonomics decision, but I do wonder a bit if we impose an extra 2 method invocations per attribute if that might be a performance penalty with some JVMs.

That being said, I'm really ok with either option, especially if we're going to remove AttributeValue from the API.

@carlosalberto @bogdandrutu any opinions?

trask · 2020-04-06T22:28:20Z

I do wonder a bit if we impose an extra 2 method invocations per attribute if that might be a performance penalty with some JVMs

I don't think the extra 2 method invocations hurt compared to the extra memory allocation required for the temporary Acceptor/Attribute object. In theory the JVM could do escape analysis on this, but it's nice not to rely on that.

That's (partly) why I like passing in the name and value as separate args, instead of creating a temporary object to hold them both, e.g.

  public Span.Builder setAttribute(LongSemanticAttribute attr, long value);
  public Span.Builder setAttribute(StringSemanticAttribute attr, String value);

jkwatson · 2020-04-06T23:21:51Z

  public Span.Builder setAttribute(LongSemanticAttribute attr, long value);
  public Span.Builder setAttribute(StringSemanticAttribute attr, String value);

How is this really different than what we have today? That is, what does it buy us over the existing setAttribute methods that take a string key that can be supplied via the existing SemanticAttributes class? We can already do this:

OpenTelemetry.getTracerProvider().get("").spanBuilder("foo")
        .setAttribute(DB_STATEMENT.key(), "select foo from bar")
        .setAttribute(DB_URL.key(), "jdbc:blah:blah:blah")
        .setAttribute(DB_USER.key(), "username")
        .setAttribute(DB_PORT.key(), 3030)
        .setAttribute(DB_IS_AWESOME.key(), true)
        .startSpan();

trask · 2020-04-07T00:10:41Z

  public Span.Builder setAttribute(LongSemanticAttribute attr, long value);
  public Span.Builder setAttribute(StringSemanticAttribute attr, String value);

Gives you type-safety for SemanticAttributes.

If type-safety is not a requirement for SemanticAttributes, then SemanticAttributes could just be String constants for the attribute name, and we could use the existing setters.

Oberon00 · 2020-04-07T06:53:20Z

@trask I think your suggestion would be the most straightforward and performant design, but it also means that at least the AttributeSetter classes cannot be moved out of the API package. If we are OK with that, that's what I would go for. However, we should probably introduce an abstract base class implementing the Span.Builder interface that takes care of the boilerplate of calling setAttriubte(String, T) from setAttribute(TAttribute, T).

jkwatson · 2020-04-07T14:55:31Z

  public Span.Builder setAttribute(LongSemanticAttribute attr, long value);
  public Span.Builder setAttribute(StringSemanticAttribute attr, String value);

Gives you type-safety for SemanticAttributes.

Oh yes. Somehow I missed the one actual important part of what you suggested. I think I got distracted by the long names, and missed the point. What if we did this, but just called it LongAttribute, and drop the Semantic word, since it's not necessary, and the methods could be used for things other than just semantic convention enforcement.

Oberon00 · 2020-04-07T15:20:59Z

@jkwatson I think we got lost in pseudocode here, the actual name is LongAttributeSetter, etc.

jkwatson · 2020-04-07T15:22:06Z

@jkwatson I think we got lost in pseudocode here, the actual name is LongAttributeSetter, etc.

With the suggestion from @trask , then its no longer doing the setting, so removing that adjective probably would make sense.

Oberon00 · 2020-04-07T15:25:43Z

If we do #1081 first, then I'm OK with naming it just "Attribute" as there is no potential for confusion anymore, or at least only for people who work with SDK internals.

carlosalberto · 2020-04-07T18:24:27Z

I'm ok-ish with trying @trask approach, but honestly slightly afraid of adding more boilerplate for helping with some chaining (are users expected to really be setting tags quite often? I'd say 'no', as mostly only people doing instrumentation will, which is mostly 'us').

Similar to #1081 I'd be up for a PR that prototypes it out.

jkwatson · 2020-04-07T18:35:27Z

@carlosalberto I am also quite leery about increasing our API surface without some very deep thought and very clear non-trivial impact. It would be good if we had some sort of metric we could apply before adding API surface. Like, "this change will reduce the instrumentation code by 3 lines per span", which means 300 less lines of code in the instrumentation altogether.

jkwatson · 2020-04-09T16:36:24Z

I'm ok-ish with trying @trask approach, but honestly slightly afraid of adding more boilerplate for helping with some chaining (are users expected to really be setting tags quite often? I'd say 'no', as mostly only people doing instrumentation will, which is mostly 'us').

Similar to #1081 I'd be up for a PR that prototypes it out.

I'll see if I can put together a prototype draft PR for this. Unless someone else wants to get to it before me.

jkwatson · 2020-04-09T17:49:22Z

I'll see if I can put together a prototype draft PR for this. Unless someone else wants to get to it before me.

See #1096

iNikem · 2020-05-20T14:52:28Z

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


They may be to call it Consumer as well?

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

Oberon00 · 2020-07-01T17:47:48Z

This is superseded by now.

Oberon00 force-pushed the feature/semconvbuilder branch 2 times, most recently from dc23ac3 to bca7dd7 Compare April 3, 2020 14:00

Add support to use SemanticAttributes with Span.Builder.

5ff9226

Oberon00 force-pushed the feature/semconvbuilder branch from bca7dd7 to 5ff9226 Compare April 3, 2020 14:05

Oberon00 marked this pull request as ready for review April 3, 2020 14:12

Oberon00 requested review from arminru, bogdandrutu, carlosalberto, jkwatson, pavolloffay, songy23 and tylerbenson as code owners April 3, 2020 14:12