Proposal: Remove AttributeValue class and implement attribute value type-safety with a type-indicating Key.

[jkwatson]

Is your feature request related to a problem? Please describe.

1. Users have expressed some confusion about the way the Semantic conventions for Attribute keys/values are implemented. The usage of the AttributeSetter classes is awkward, and when you aren't using a pre-defined instance of the SemanticAttributes class, it's awkward for API users to have to define their own AttributeSetters to make their API usage consistent.
2. The use of AttributeValue requires, in almost every case, an additional instance allocation to wrap the value that is being set in the Attributes. Most commonly, in instrumentation, the attribute Keys are the thing that is known ahead of time and can be statically allocated, requiring no additional runtime allocation overhead, after the instrumentation (or the API itself, in the case of pre-defined semantic attributers) is initialized. Our current implementation requires the wrapping of every value, at runtime, in the hot path of the application of the instrumentation.

Describe the solution you'd like
Rather than provide the type-safe wrapper the value side of the attribute, I propose to make the type safety be enforced by providing type-information on the key side. The API would look something like (for the Span interface):

 void setAttribute(StringValuedKey key, String value);
 void setAttribute(LongValuedKey key, long value);
 void setAttribute(DoubleValuedKey key, double value);
 void setAttribute(BooleanValuedKey key, boolean value);
...

As mentioned above, this would have the significant allocation advantage of not needing to do any extra allocations at runtime for the values of the attributes, at least for the String valued attributes (which make up the majority of our semantic attributes). The implementation would probably need to wrap the primitives in boxed versions in the storage, so there would be a net zero gain in runtime allocations for the 3 primitive-typed values [again, assuming the keys are statically pre-defined, either in the instrumentation, or in the API's semantic attribute definitions].

A version of this solution was prototyped in this PR: #1097

Describe alternatives you've considered
Several other alternatives have been prototyped and proposed:

• Removing AttributeValue altogether, and not replacing it with anything: Prototype code on what might look like to remove the AttributeValue from the API #1500
• Much discussion has taken place on this issue: Proposal: Remove AttributeValue from the public interface. #1081
• And some on this one as well: Too many wrapper objects? #1314

Additional context
This change would be a significant API-breaking change, and would be hard to retrofit post GA. It could be done as an API addition, but would then have the significant downside of having duplicated Attribute management APIs and semantic conventions. Hence, I have marked this issue as required-for-ga, because I think it could only feasibly be done before then.

[anuraaga]

I like this idea since the API looks nicer in general. But just for clarity, can you describe why the allocations at runtime are different? Methods like setAttribute(String key, long value), which are called by the setters right now, have the same performance characteristics right?

[jkwatson]

For longs, doubles and booleans, yes, the number of allocations is identical, assuming that you've statically pre-allocated your keys. For Strings, though, we have one less allocation, since we don't need to wrap the String in an additional AttributeValue wrapper.

This analysis assumes the same type of List<Object> storage that is being used today in the APIs Attributes implementation.

The big win is the fact that you don't have to wrap String values another time, and you can statically pre-allocate your keys.

[anuraaga]

Yeah but #1500 does that without replacing anything - which reading further in the issue you mention explicitly I realize :) Pretty sure we're on the same page but just wanted to make sure they're technically somewhat independent improvements.

[jkwatson]

Yeah but #1500 does that without replacing anything - which reading further in the issue you mention explicitly I realize :) Pretty sure we're on the same page but just wanted to make sure they're technically somewhat independent improvements.

We're on the same page. I think this makes for a slightly nicer API, so I thought I'd at least write up the proposal. :)

[kenfinnigan]

+1 from me, the current API is a bit clunky from a developer experience perspective

[jkwatson]

If people are in favor of this issue, please put a 👍 on the PR itself, so we can track interest.

[tylerbenson]

I like the idea of having a typed key and not having to construct wrappers. Something to consider is adding a construct of a mapper or serializer as a way to deconstruct complex objects outside of the critical path. For example applying a url attribute is often the result of concatenating/parsing which can be relatively expensive, so better to postpone that work. It's also possible that a single attribute object might want to result in multiple underlying attributes set on the span.

[jkwatson]

Note: a very small demo of this here: #1598

[jkwatson]

I like the idea of having a typed key and not having to construct wrappers. Something to consider is adding a construct of a mapper or serializer as a way to deconstruct complex objects outside of the critical path. For example applying a url attribute is often the result of concatenating/parsing which can be relatively expensive, so better to postpone that work. It's also possible that a single attribute object might want to result in multiple underlying attributes set on the span.

check out the most recent commit which shows how this could work.

[jkwatson]

This proposal seems to have general approval from @anuraaga , @trask , @tylerbenson (and me), as well as @kenfinnigan . I will go ahead and do the work to make this into a real PR that changes our Attributes to use this pattern.

@bogdandrutu @carlosalberto If you disagree with this approach, please speak up ASAP, as it will be a significant change to the APIs (and a significant amount of work to make it happen).