Add Span#setAttributes method to add Attributes to a Span #2799

HaloFour · 2021-02-11T02:08:49Z

Adds helper methods Span#setAttributes and SpanBuilder#setAttributes which accept an Attributes instance and copy the attributes into the span.

api/all/src/main/java/io/opentelemetry/api/trace/SpanBuilder.java

jkwatson · 2021-02-11T02:36:19Z

Cool. This is a great addition to the APIs. I think we need some unit tests for these methods as well. Thanks!

codecov · 2021-02-11T03:35:13Z

Codecov Report

Merging #2799 (2bc85a7) into main (b3860d4) will not change coverage.
The diff coverage is n/a.

❗ Current head 2bc85a7 differs from pull request most recent head 92df217. Consider uploading reports for the commit 92df217 to get more accurate results

@@     Coverage Diff      @@
##   main   #2799   +/-   ##
============================
============================

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b3860d4...92df217. Read the comment docs.

iNikem · 2021-02-11T07:44:21Z

api/all/src/main/java/io/opentelemetry/api/trace/Span.java

+   * @param attributes the attributes
+   * @return this.
+   */
+  default Span setAttributes(Attributes attributes) {


This seems more like addAll to me than set. The latter implies that new attributes completely replace the old ones.

I'd almost argue that set* should be called put* given the Map-like semantics. But I'll let you guys argue/settle on the names. 😁

I've worked out my Gradle/IDE issues and hopefully will get some unit tests up later this morning. Apologies for the rapid fire commits last night if they resulted in notifications.

setAll is another option for this.

Map-like would be then putAll :)

Yep. I'm game with whatever name you guys prefer, let me know and I'll update the PR.

well, IMO, all the rest of the methods, for good or for ill, are set*, so we should definitely start with set. :)

jkwatson · 2021-02-11T17:05:32Z

api/all/src/main/java/io/opentelemetry/api/trace/SpanBuilder.java

+    SpanBuilder span = this;
+    for (Map.Entry<AttributeKey<?>, Object> entry : attributes.asMap().entrySet()) {
+      @SuppressWarnings("unchecked")
+      AttributeKey<Object> attributeKey = (AttributeKey<Object>) entry.getKey();
+      span = span.setAttribute(attributeKey, entry.getValue());
+    }
+    return span;


Can't you replace this section with this:

attributes.forEach((key, o) -> this.setAttribute((AttributeKey<Object>) key, o)); return this;

With this, we'd need to add an @SuppressWarnings("unchecked") to the interface method, which would be a bit odd, I guess. At the very least, we could put this more efficient implementation down in the SDK's implemenation.

That annotation doesn't make it into bytecode or javadoc, so maybe it would be ok?

Ah, a little better:

attributes.forEach((key, v) -> { @SuppressWarnings("unchecked") AttributeKey<Object> k = (AttributeKey<Object>) key; setAttribute(k, v); }); return this;

Sure. I was thinking I'd want to be agnostic about whether setAttribute returned the same instance or a different immutable instance but if it's always guaranteed to be the former then it's a moot point. I could always scope the unchecked operation to an assignment to a local within the lambda which I would think Hotspot should be able to elide.

hm. interesting. I suppose there could be an impl that was immutable and this wouldn't work with that. Perhaps just push the better one down into the SDK where we know we're mutable?

The javadoc has @return this, which I think specifies the contract for this interface that it's always mutable so I think we don't need to worry too much.

Good point, I'm being too defensive here. I'll also update the name of the method to setAllAttributes to I think more clearly describe what it does.

sdk/trace/src/main/java/io/opentelemetry/sdk/trace/SdkSpanBuilder.java

bogdandrutu · 2021-02-12T04:13:07Z

api/all/src/main/java/io/opentelemetry/api/trace/Span.java

+      return this;
+    }
+    attributes.forEach(
+        (attributeKey, value) -> this.setAttribute((AttributeKey<Object>) attributeKey, value));


s/Object/??

attributeKey is already an AttributeKey<?>, it needs to be unchecked cast to AttributeKey<Object> in order to call setAttribute with value since it is an Object.

anuraaga · 2021-02-12T06:09:36Z

api/all/src/main/java/io/opentelemetry/api/trace/Span.java

@@ -162,6 +162,23 @@ default Span setAttribute(AttributeKey<Long> key, int value) {
    return setAttribute(key, (long) value);
  }

+  /**
+   * Sets attributes to the {@code Span}. If the {@code Span} previously contained a mapping for any


Suggested change

* Sets attributes to the {@code Span}. If the {@code Span} previously contained a mapping for any

* Sets attributes to the {@link Span}. If the {@link Span} previously contained a mapping for any

anuraaga · 2021-02-12T06:10:20Z

api/all/src/main/java/io/opentelemetry/api/trace/Span.java

+      return this;
+    }
+    attributes.forEach(
+        (attributeKey, value) -> this.setAttribute((AttributeKey<Object>) attributeKey, value));


Thanks @HaloFour, this follows the pattern we have in a couple other places like putAll where we hide the unchecked cast from users with this sort of shortcut.

anuraaga · 2021-02-12T06:11:11Z

api/all/src/main/java/io/opentelemetry/api/trace/SpanBuilder.java

@@ -233,6 +233,23 @@
   */
  <T> SpanBuilder setAttribute(AttributeKey<T> key, @Nonnull T value);

+  /**
+   * Sets attributes to the {@code Span}. If the {@code Span} previously contained a mapping for any


Suggested change

* Sets attributes to the {@code Span}. If the {@code Span} previously contained a mapping for any

* Sets attributes to the {@link SpanBuilder}. If the {@link SpanBuilder} previously contained a mapping for any

anuraaga · 2021-02-12T06:12:03Z

sdk/trace/src/test/java/io/opentelemetry/sdk/trace/RecordEventsReadableSpanTest.java

+    }
+
+    SpanData spanData = span.toSpanData();
+    assertThat(spanData.getAttributes().size()).isEqualTo(16);


You may consider a map assertion on spanData.getAttributes.asMap() though this is fine too

jkwatson · 2021-02-12T20:04:13Z

Pending a decision on the name of the method, this looks great. Thanks!

anuraaga · 2021-02-17T07:37:34Z

We are going to give the decision on setAttribute vs putAttribute another day to settle but have tentatively decided to go with the former, so we will likely continue with the current name setAllAttributes. Thanks for the patience @HaloFour

anuraaga

@HaloFour Sorry for the delay on the response on this - the method name looks great, do you mind rebasing and we'll get this in?

Oberon00 · 2021-02-26T08:11:08Z

Is this something that could be brought up in the spec?

anuraaga · 2021-02-26T08:19:47Z

@Oberon00 I see, in the spec indeed there is a sentence about "single attribute"

https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/trace/api.md#set-attributes

Let me file a spec issue - hope it doesn't take too long :)

anuraaga · 2021-02-26T08:24:14Z

open-telemetry/opentelemetry-specification#1473 is the spec issue

HaloFour · 2021-02-26T12:54:20Z

Cool beans. Give me a heads up as to whether we're unblocked by the spec issue and I'll rebase.

jkwatson · 2021-03-16T17:42:36Z

open-telemetry/opentelemetry-specification#1473 has been merged, so this has been unblocked, @HaloFour .

HaloFour · 2021-03-16T17:52:31Z

Excellent, I'll fix the merge conflict and rebase. Thanks!

jkwatson

Thanks!

jkwatson · 2021-04-23T21:38:57Z

Sorry, I had forgotten about this PR until I ran into this myself. approved.
@bogdandrutu Any objections to this API addition?

jkwatson · 2021-04-27T21:41:20Z

Merging! Thanks @HaloFour, and sorry for the long delay!

HaloFour requested review from anuraaga, arminru, bogdandrutu, carlosalberto, jkwatson, Oberon00, pavolloffay, thisthat and tylerbenson as code owners February 11, 2021 02:08

jkwatson reviewed Feb 11, 2021

View reviewed changes

api/all/src/main/java/io/opentelemetry/api/trace/SpanBuilder.java Outdated Show resolved Hide resolved

jkwatson added the API API related issues label Feb 11, 2021

iNikem reviewed Feb 11, 2021

View reviewed changes

jkwatson reviewed Feb 11, 2021

View reviewed changes

sdk/trace/src/main/java/io/opentelemetry/sdk/trace/SdkSpanBuilder.java Outdated Show resolved Hide resolved

bogdandrutu reviewed Feb 12, 2021

View reviewed changes

anuraaga reviewed Feb 12, 2021

View reviewed changes

anuraaga approved these changes Feb 26, 2021

View reviewed changes

anuraaga mentioned this pull request Feb 26, 2021

Add method to allow setting multiple attributes at once to … open-telemetry/opentelemetry-specification#1473

Merged

jkwatson added the blocked:spec blocked on open or unresolved spec label Mar 5, 2021

jkwatson removed the blocked:spec blocked on open or unresolved spec label Mar 16, 2021

carlosalberto approved these changes Mar 16, 2021

View reviewed changes

HaloFour force-pushed the set-attributes branch from 96c4257 to 2bc85a7 Compare March 17, 2021 00:41

Add Span#setAllAttributes method to add Attributes to a Span

92df217

HaloFour force-pushed the set-attributes branch from 2bc85a7 to 92df217 Compare April 19, 2021 12:18

jkwatson approved these changes Apr 23, 2021

View reviewed changes

jkwatson mentioned this pull request Apr 26, 2021

Add Span.setAttributes method #3178

Closed

jkwatson merged commit 7d31cdd into open-telemetry:main Apr 27, 2021

jkwatson mentioned this pull request Apr 27, 2021

Add a task to compare public APIs for every stable module. #3183

Merged

This was referenced Dec 19, 2021

Temurin JDK #4011

Merged

use Eclipse Temurin JDK docker image #4012

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Span#setAttributes method to add Attributes to a Span #2799

Add Span#setAttributes method to add Attributes to a Span #2799

HaloFour commented Feb 11, 2021

jkwatson commented Feb 11, 2021

codecov bot commented Feb 11, 2021 •

edited

Loading

iNikem Feb 11, 2021

HaloFour Feb 11, 2021

jkwatson Feb 11, 2021

iNikem Feb 11, 2021

HaloFour Feb 11, 2021

jkwatson Feb 12, 2021

jkwatson Feb 11, 2021

jkwatson Feb 11, 2021

jkwatson Feb 11, 2021

HaloFour Feb 11, 2021

jkwatson Feb 11, 2021

anuraaga Feb 12, 2021

HaloFour Feb 12, 2021

bogdandrutu Feb 12, 2021

HaloFour Feb 12, 2021

anuraaga Feb 12, 2021

anuraaga Feb 12, 2021

anuraaga Feb 12, 2021

anuraaga Feb 12, 2021

jkwatson commented Feb 12, 2021

anuraaga commented Feb 17, 2021

anuraaga left a comment

Oberon00 commented Feb 26, 2021

anuraaga commented Feb 26, 2021

anuraaga commented Feb 26, 2021

HaloFour commented Feb 26, 2021

jkwatson commented Mar 16, 2021

HaloFour commented Mar 16, 2021

jkwatson left a comment

jkwatson commented Apr 23, 2021

jkwatson commented Apr 27, 2021

	* Sets attributes to the {@code Span}. If the {@code Span} previously contained a mapping for any
	* Sets attributes to the {@link Span}. If the {@link Span} previously contained a mapping for any

	* Sets attributes to the {@code Span}. If the {@code Span} previously contained a mapping for any
	* Sets attributes to the {@link SpanBuilder}. If the {@link SpanBuilder} previously contained a mapping for any

Add Span#setAttributes method to add Attributes to a Span #2799

Add Span#setAttributes method to add Attributes to a Span #2799

Conversation

HaloFour commented Feb 11, 2021

jkwatson commented Feb 11, 2021

codecov bot commented Feb 11, 2021 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkwatson commented Feb 12, 2021

anuraaga commented Feb 17, 2021

anuraaga left a comment

Choose a reason for hiding this comment

Oberon00 commented Feb 26, 2021

anuraaga commented Feb 26, 2021

anuraaga commented Feb 26, 2021

HaloFour commented Feb 26, 2021

jkwatson commented Mar 16, 2021

HaloFour commented Mar 16, 2021

jkwatson left a comment

Choose a reason for hiding this comment

jkwatson commented Apr 23, 2021

jkwatson commented Apr 27, 2021

codecov bot commented Feb 11, 2021 •

edited

Loading