Convert Attributes and AttributeValue into interfaces

[jkwatson]

The creational methods are now in inner classes, which is definitely an API breaking change.

Note: had to pull out the other inner classes up a level, otherwise they would inherit the interfaces public access modifier.

The rationale for this is to make life easier for the instrumentation agent code, where it is more desirable to deal with interfaces than abstract classes.

Relevant Issues:

• Proposal: Remove AttributeValue class and implement attribute value type-safety with a type-indicating Key. #1580
• Too many wrapper objects? #1314
• Improve performance of trace/metrics export pipeline by reducing number of allocs #1191
• Proposal: Remove AttributeValue from the public interface. #1081

Other PRs that strive to achieve the same goal in different ways:

• Prototype code on what might look like to remove the AttributeValue from the API #1500
• Prototype of removing bare span attribute keys #1097

[codecov]

Codecov Report

Merging #1589 into master will decrease coverage by 0.12%.
The diff coverage is 86.61%.

@@             Coverage Diff              @@
##             master    #1589      +/-   ##
============================================
- Coverage     91.67%   91.55%   -0.13%     
- Complexity      979     1006      +27     
============================================
  Files           116      126      +10     
  Lines          3533     3530       -3     
  Branches        307      298       -9     
============================================
- Hits           3239     3232       -7     
- Misses          203      205       +2     
- Partials         91       93       +2     

Impacted Files	Coverage Δ	Complexity Δ
...va/io/opentelemetry/common/BaseAttributeValue.java	11.11% <11.11%> (ø)	2.00 <2.00> (?)
...ntelemetry/sdk/trace/RecordEventsReadableSpan.java	81.74% <90.00%> (ø)	76.00 <4.00> (ø)
...io/opentelemetry/common/ArrayBackedAttributes.java	100.00% <100.00%> (ø)	3.00 <3.00> (?)
...n/java/io/opentelemetry/common/AttributeValue.java	100.00% <100.00%> (+17.77%)	0.00 <0.00> (-10.00)	⬆️
...io/opentelemetry/common/AttributeValueBoolean.java	100.00% <100.00%> (ø)	3.00 <3.00> (?)
...entelemetry/common/AttributeValueBooleanArray.java	100.00% <100.00%> (ø)	7.00 <7.00> (?)
.../io/opentelemetry/common/AttributeValueDouble.java	100.00% <100.00%> (ø)	3.00 <3.00> (?)
...pentelemetry/common/AttributeValueDoubleArray.java	100.00% <100.00%> (ø)	7.00 <7.00> (?)
...va/io/opentelemetry/common/AttributeValueLong.java	100.00% <100.00%> (ø)	3.00 <3.00> (?)
.../opentelemetry/common/AttributeValueLongArray.java	100.00% <100.00%> (ø)	7.00 <7.00> (?)
... and 22 more

---

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 ff88c88...ad7d535. Read the comment docs.

[Oberon00]

Can you link the corresponding issue or otherwise state a rationale?

[jkwatson]

Can you link the corresponding issue or otherwise state a rationale?

Sorry about that. I probably should have marked this as a draft. This is another attempt, in a long series, at providing a way for the java agent to not have to do a bunch of bending-over-backwards to do extra wrapping of concrete classes that exist in the API. I'll update the description with pointers to the many discussions.

[trask]

Ugh. I tried implementing both the unshaded and shaded AttributeValue interfaces at the same time, but getType() method can't return two different enums. If Type was interface we could use covariant return to return something that implements both interfaces. But that would be a poor API choice.

[bogdandrutu]

@trask I think that is a downgrade in usability because cannot use switch statement.

[jkwatson]

Ugh. I tried implementing both the unshaded and shaded AttributeValue interfaces at the same time, but getType() method can't return two different enums. If Type was interface we could use covariant return to return something that implements both interfaces. But that would be a poor API choice.

oh, bummer. Pretty stuck if we need to keep the Type around. If we got rid of AttributeValue altogether, and switched out for the typed keys, it might work, though. I don't remember if we still need the Type enum in that case. I'll take a look tomorrow.

[bogdandrutu]

@jkwatson want to make sure we don't do all the decisions based on the ability to shade or not. It is very important the agent use-case but API should be developed for usability as well. That being said I am not oppose to search for new ways that are user friendly as well as agent friendly

[jkwatson]

@jkwatson want to make sure we don't do all the decisions based on the ability to shade or not. It is very important the agent use-case but API should be developed for usability as well. That being said I am not oppose to search for new ways that are user friendly as well as agent friendly

Absolutely. I always favor the user in these decisions (although our primary user is the instrumentation folks at the moment!). Trying to find the balance between developer ergonomics and performance in the agent is the name of the game.

[jkwatson]

Closing this as a failed experiment, since it makes a less friendly API (with the inner Factory classes) and doesn't actually make it easier for shading in the agent.