Update Attributes API to comply the spec #1437

thisthat · 2020-07-20T14:37:14Z

Update attributes API to the latest spec: open-telemetry/opentelemetry-specification#777

sdk/src/main/java/io/opentelemetry/sdk/resources/Resource.java

codecov · 2020-07-20T14:49:42Z

Codecov Report

Merging #1437 into master will decrease coverage by 0.02%.
The diff coverage is 96.00%.

@@             Coverage Diff              @@
##             master    #1437      +/-   ##
============================================
- Coverage     91.39%   91.37%   -0.03%     
  Complexity      954      954              
============================================
  Files           116      116              
  Lines          3416     3431      +15     
  Branches        281      290       +9     
============================================
+ Hits           3122     3135      +13     
- Misses          205      206       +1     
- Partials         89       90       +1

Impacted Files	Coverage Δ	Complexity Δ
.../main/java/io/opentelemetry/common/Attributes.java	`96.07% <93.10%> (-3.93%)`	`10.00 <0.00> (ø)`
...n/java/io/opentelemetry/common/AttributeValue.java	`82.22% <100.00%> (+2.47%)`	`10.00 <1.00> (+1.00)`
...ntelemetry/sdk/trace/RecordEventsReadableSpan.java	`81.81% <100.00%> (-0.08%)`	`77.00 <0.00> (-1.00)`

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 dfc5cbe...0387979. Read the comment docs.

sdk/src/main/java/io/opentelemetry/sdk/resources/Resource.java

anuraaga

Shouldn't we just not set null in Attributes instead of filtering on consumption?

"Attribute values of null are considered to be not set and get discarded as if that SetAttribute call had never been made"

jkwatson · 2020-07-20T23:10:18Z

Shouldn't we just not set null in Attributes instead of filtering on consumption?

"Attribute values of null are considered to be not set and get discarded as if that SetAttribute call had never been made"

Since anyone can create a Resource, even outside of this codebase, we wouldn't be able to control that, so having the filter in place seems like a good idea. But, yes, "don't do that" is always a good rule when it comes to nulls. ;)

anuraaga · 2020-07-21T03:22:41Z

@jkwatson Sorry wasn't clear, I meant filtering out null values when creating attributes, e.g. the builder's setters

https://github.com/open-telemetry/opentelemetry-java/blob/master/api/src/main/java/io/opentelemetry/common/Attributes.java#L148

thisthat · 2020-07-21T06:00:39Z

@anuraaga null values are meaningful because they are used to delete previously set attributes (for Span).

anuraaga · 2020-07-21T08:40:48Z

Hmm if we use null as an implementation detail of emptiness it's ok. In that case shouldn't forEach internally filter null? Having all users of forEach filter like this seems weird

anuraaga · 2020-07-21T09:05:42Z

On second thought, do we have an API for deleting attributes?

thisthat · 2020-07-21T11:00:19Z

Yes we do have it https://github.com/open-telemetry/opentelemetry-java/blob/master/sdk/src/main/java/io/opentelemetry/sdk/trace/RecordEventsReadableSpan.java#L314
However, looking again at the spec, Events and Links attributes should also offer the possibility to delete existing attributes.

jkwatson · 2020-07-21T17:33:33Z

Yes we do have it https://github.com/open-telemetry/opentelemetry-java/blob/master/sdk/src/main/java/io/opentelemetry/sdk/trace/RecordEventsReadableSpan.java#L314
However, looking again at the spec, Events and Links attributes should also offer the possibility to delete existing attributes.

I don't think events and links need to support this. they are created immutable (or lazily so), so you won't be removing attributes from existing links or events.

jkwatson · 2020-07-21T17:35:47Z

Note: the way that attributes are deleted from spans is pretty hacky at the moment (it involves using an extension of HashMap). I'd love to figure out a better way to do it.

bogdandrutu · 2020-07-22T02:23:58Z

sdk/src/main/java/io/opentelemetry/sdk/resources/Resource.java

+      if (value.getType() == AttributeValue.Type.STRING && value.getStringValue() == null) {
+        return;
+      }


Why other values like null array is not removed?

I think along these lines, if we're using null to model deletion, we should just store null directly, not an attribute value with a value of null.

Because null values inside arrays are meaningful. Since the Attributes API does not create null attributes, it cannot exist a null value but only a value that contains an AttributeValue with null. Does it make sense?

No, it does not make sense. What if the "array" is null, not the value inside the array?

The "array" cannot be null. Resource uses the Attributes class for storing attributes which creates always an object for arrays: https://github.com/open-telemetry/opentelemetry-java/blob/master/api/src/main/java/io/opentelemetry/common/Attributes.java#L204

Very strange behavior for arrays then:

opentelemetry-java/api/src/main/java/io/opentelemetry/common/AttributeValue.java

Line 331 in 78ed649

if (stringValues == null) {

We replace with empty array/list.... Not too much consistency.

General Java guidelines are to never return null for a collection, but use an empty one (per Josh Bloch in Effective Java). So, I think this behavior is a good one.

We discussed about this. It is not we return null, it is we get null and add an entry with empty collection, why then not add an entry with empty string?

I have uniformed the handling of null in 18e3719. The following cases are dropped (or removed if the key was already present):

null strings, e.g. (String) null

new arrays with 0 length, e.g. new String[0]

null arrays, e.g. (String[]) null

Instead, the following are stored as attributes:

Empty strings

Arrays with null values as members, e.g. new String[] {null}

anuraaga · 2020-07-22T02:46:14Z

So assuming we do need to keep null values to model deletion (seems a bit weird if we support deletion we could have an explicit deletion API? Java Maps have the exact problem @jkwatson mentions of supporting null values in a very undefined way), still I wonder why we don't filter in the producer rather than all the consumers, here

https://github.com/open-telemetry/opentelemetry-java/blob/master/api/src/main/java/io/opentelemetry/common/ImmutableKeyValuePairs.java#L58

thisthat · 2020-07-23T06:28:04Z

@anuraaga filtering in the producer makes sense only if we do have an explicit API to delete Span attributes and we would need to change the spec.

anuraaga · 2020-07-23T06:31:07Z

@anuraaga filtering in the producer makes sense only if we do have an explicit API to delete Span attributes

Really? We'd still be setting the attribute with null, meaning it's deleted. And if we filter in the producer, it won't be returned to the consumer, fulfilling the requirement of acting as if SetAttribute call had never been made. What am I missing? :)

thisthat · 2020-07-23T06:54:29Z

I was missing something 😅 ImmutableKeyValuePairs is not used to set Span attributes. We could indeed filter in the producer!

sdk/src/main/java/io/opentelemetry/sdk/resources/Resource.java

api/src/main/java/io/opentelemetry/common/AttributeValue.java

jkwatson · 2020-08-05T17:30:41Z

I haven't been following the full conversation, but if this about discarding empty arrays, I'm against it. We also don't discard empty strings.

See spec https://github.com/open-telemetry/opentelemetry-specification/blob/v0.6.0/specification/trace/api.md#set-attributes:

Attribute values expressing a numerical value of zero or an empty string are considered meaningful and MUST be stored and passed on to span processors / exporters.

The same should hold for empty arrays IMHO.

This is only for the Resource, not for Span attributes. @thisthat can you update the PR title to reflect that it's just for Resource attributes?

anuraaga · 2020-08-06T06:17:42Z

Thanks for bearing with my confusion!

thisthat · 2020-08-17T07:25:22Z

@open-telemetry/java-approvers I have update the PR to implement the latest specification introduced by open-telemetry/opentelemetry-specification#777
Resource (Attributes API) and Span attributes have the same behavior:

accept empty arrays
accept null values within arrays
drop null or empty keys
drop null values or delete the attribute if was previously set

anuraaga

Thanks a lot for the update! I think it's ok now, but for the future, when making such a large change to a PR it may be better to close the old one and start a new one to give everyone a clean slate.

api/src/main/java/io/opentelemetry/common/Attributes.java

anuraaga

Thanks!

api/src/main/java/io/opentelemetry/common/Attributes.java

api/src/main/java/io/opentelemetry/common/AttributeValue.java

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

jkwatson

Thanks for sticking with this one!

thisthat requested review from arminru, bogdandrutu, carlosalberto, jkwatson, pavolloffay and tylerbenson as code owners July 20, 2020 14:37

iNikem reviewed Jul 20, 2020

View reviewed changes

sdk/src/main/java/io/opentelemetry/sdk/resources/Resource.java Outdated Show resolved Hide resolved

jkwatson reviewed Jul 20, 2020

View reviewed changes

sdk/src/main/java/io/opentelemetry/sdk/resources/Resource.java Outdated Show resolved Hide resolved

anuraaga reviewed Jul 20, 2020

View reviewed changes

thisthat changed the title ~~Forbit null values~~ Forbid null values Jul 21, 2020

jkwatson approved these changes Jul 21, 2020

View reviewed changes

bogdandrutu reviewed Jul 22, 2020

View reviewed changes

jkwatson approved these changes Jul 30, 2020

View reviewed changes

thisthat force-pushed the null-resource-value branch from 387ce57 to 18e3719 Compare August 4, 2020 08:43

jkwatson reviewed Aug 4, 2020

View reviewed changes

sdk/src/main/java/io/opentelemetry/sdk/resources/Resource.java Outdated Show resolved Hide resolved

anuraaga reviewed Aug 5, 2020

View reviewed changes

sdk/src/main/java/io/opentelemetry/sdk/resources/Resource.java Outdated Show resolved Hide resolved

jkwatson reviewed Aug 5, 2020

View reviewed changes

api/src/main/java/io/opentelemetry/common/AttributeValue.java Outdated Show resolved Hide resolved

thisthat changed the title ~~Forbid null values~~ Forbid null values and empty arrays in Resource Attributes Aug 6, 2020

anuraaga approved these changes Aug 6, 2020

View reviewed changes

thisthat changed the title ~~Forbid null values and empty arrays in Resource Attributes~~ Update Attributes API to comply the spec Aug 17, 2020

thisthat added 10 commits August 17, 2020 09:16

Forbit null values

d349e45

Simplify code and better naming

0209edd

Fix error prone error

a648e44

Uniform null handling

a97baaf

Improve tests for SpanAttributes

83fe7c4

Improve tests for Resource Attributes

34cdcca

Change interface of AttributeValue

c3785f6

isEmpty overrided only when necessary

22e1948

Update Attributes API

b90449d

Remove unused code

2bdb1cf

thisthat force-pushed the null-resource-value branch from 98372f3 to 2bdb1cf Compare August 17, 2020 07:17

anuraaga suggested changes Aug 17, 2020

View reviewed changes

api/src/main/java/io/opentelemetry/common/Attributes.java Outdated Show resolved Hide resolved

api/src/main/java/io/opentelemetry/common/Attributes.java Outdated Show resolved Hide resolved

Rename method

ed949be

anuraaga approved these changes Aug 17, 2020

View reviewed changes

jkwatson reviewed Aug 17, 2020

View reviewed changes

api/src/main/java/io/opentelemetry/common/Attributes.java Outdated Show resolved Hide resolved

jkwatson reviewed Aug 17, 2020

View reviewed changes

api/src/main/java/io/opentelemetry/common/Attributes.java Outdated Show resolved Hide resolved

jkwatson reviewed Aug 17, 2020

View reviewed changes

api/src/main/java/io/opentelemetry/common/AttributeValue.java Outdated Show resolved Hide resolved

jkwatson self-requested a review August 17, 2020 15:36

jkwatson reviewed Aug 17, 2020

View reviewed changes

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

thisthat added 2 commits August 18, 2020 08:31

Address PR feedback

075ce06

fix javadoc

0387979

jkwatson approved these changes Aug 18, 2020

View reviewed changes

jkwatson merged commit 792f0f4 into open-telemetry:master Aug 18, 2020

thisthat deleted the null-resource-value branch September 22, 2020 05:59

Update Attributes API to comply the spec #1437

Update Attributes API to comply the spec #1437

Uh oh!

Conversation

thisthat commented Jul 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

codecov bot commented Jul 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

anuraaga left a comment

Choose a reason for hiding this comment

Uh oh!

jkwatson commented Jul 20, 2020

Uh oh!

anuraaga commented Jul 21, 2020

Uh oh!

thisthat commented Jul 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

anuraaga commented Jul 21, 2020

Uh oh!

anuraaga commented Jul 21, 2020

Uh oh!

thisthat commented Jul 21, 2020

Uh oh!

jkwatson commented Jul 21, 2020

Uh oh!

jkwatson commented Jul 21, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

anuraaga commented Jul 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

thisthat commented Jul 23, 2020

Uh oh!

anuraaga commented Jul 23, 2020

Uh oh!

thisthat commented Jul 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jkwatson commented Aug 5, 2020

Uh oh!

anuraaga commented Aug 6, 2020

Uh oh!

thisthat commented Aug 17, 2020

Uh oh!

anuraaga left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

anuraaga left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

thisthat commented Jul 20, 2020 •

edited

Loading

codecov bot commented Jul 20, 2020 •

edited

Loading

thisthat commented Jul 21, 2020 •

edited

Loading

anuraaga commented Jul 22, 2020 •

edited

Loading

thisthat commented Jul 23, 2020 •

edited

Loading