Proposal: Remove AttributeValue from the public interface.

[Oberon00]

The class io.opentelemetry.common.AttributeValue seems a more cumbersome alternative to use over the setAttribute overloads for the particular attribute types. It also is rather complicated (e.g., introduces possibilities for null even for primitives). Thus, IMHO it would be best to make this class a private implementation detail in the SDK, instead of a public interface in the API.

[carlosalberto]

I'd be fine with hiding AttributeValue - one thing to note is that adding a method to add multiple attributes would be a no-go (which was something we talked about sometime in the past):

// setAttributes(AttributeValue... atrs);
span.setAttributes(strAttr, longAttr, doubAttr, boolAttr);

[trask]

I'm a big +1 for this proposal from auto-instrumentation perspective, because then we wouldn't need to bridge this class, which is not very efficient:

https://github.com/open-telemetry/opentelemetry-auto-instr-java/blob/v0.2.2/instrumentation/opentelemetry-api-0.3/src/main/java/io/opentelemetry/auto/instrumentation/opentelemetryapi/trace/Bridging.java#L103-L135

[jkwatson]

I'd be fine with hiding AttributeValue - one thing to note is that adding a method to add multiple attributes would be a no-go (which was something we talked about sometime in the past):

// setAttributes(AttributeValue... atrs);
span.setAttributes(strAttr, longAttr, doubAttr, boolAttr);

Unfortunately, the AtributeValue is just the value, without the key, so this API wouldn't work, even with what we have today. We'd need to introduce an Attribute class that contained both the key and the value.

[carlosalberto]

Unfortunately, the AtributeValue is just the value

Right, I just forgot we don't have it like that (have been reading so much proto stuff lately here). So definitely I'm up for removing it.

[bogdandrutu]

I am ok-ish to remove this, my reasons to not remove it is the AttributeValue is used in a lot of other places like events, links, resource, spans. So you will have to have a lot of setters for all the possible values.

Before making a final decision would like to build a small prototype of how adding events/links/resources will look like.

[jkwatson]

@Oberon00 any suggestions on what to do with the Span Links and Events APIs with this proposal? Would you replace with an Attribute that has both the key & (typed) value in it?

[Oberon00]

Would you replace with an Attribute that has both the key & (typed) value in it?

No, if we find we have to do that, I'd rather stay with the current design.

I would have suggested adding all overloads. Maybe we can find some design that allows event.attributes().set("foo", 123), i.e. have some AttributeMap class/interface. Extracting attribute code at that level, we could write the setter overloads for all the different types only once and still be compile-time null-safe.

[jkwatson]

I would have suggested adding all overloads. Maybe we can find some design that allows event.attributes().set("foo", 123), i.e. have some AttributeMap class/interface. Extracting attribute code at that level, we could write the setter overloads for all the different types only once and still be compile-time null-safe.

I'm working on prototyping how this would look for Events and Links. So far, it feels pretty decent. I'll put in a draft PR when it's a little more ready.

[bogdandrutu]

Keep in mind that AttributeValue is used in:
Resource
Events
Links
Attributes (for the Span)

We need to cover all these cases. Also it is very likely will need to add support for Array (already in the specs) and probably Map in the future.

[jkwatson]

Keep in mind that AttributeValue is used in:
Resource
Events
Links
Attributes (for the Span)

We need to cover all these cases. Also it is very likely will need to add support for Array (already in the specs) and probably Map in the future.

Oh, I know. :) I've been poring over the 100s of usages of AttributeValue over the past couple days. This will be a pretty big change, if we want to go forward with it.

[jkwatson]

After spending a couple of days exploring a couple of different options, I think that if we want to do this (and, I personally think it would be a good thing to clean up), then I think we should do some larger-scale API cleanup that will make remove AttributeValue much more tractable. I think this should include the API change to a type-safe key approach like was demonstrated in #1097, to support more fluent application of semantic attributes.

1. Change the Event interface to a concrete implementation with a Builder. We currently have 7 methods on the Span interface to create an Event, but no concrete implementation of the Event interface in the API. This proposal would greatly simplify the Span interface by only having a single method to add an Event to a Span.

2. Change the Link interface to a concrete implementation with a Builder. There are currently 3 methods on the Span.Builder to add a Link to the span. This could also be collapsed to a single method with a concrete Link implementation.

3. Add a concrete Attributes class which can wrap a Map, and provide the type-safety that is currently implemented in the AttributeValue class. This class could include the type-safe key approach prototyped in Prototype of removing bare span attribute keys #1097. [note: there is still thought needed in designing this, so that that SDK can extract the data from the implementation in a type-safe way, with the use of the AttributeValue type-safe wrappers only in the SDK. Perhaps there could be an SDK utility function to turn the Attributes instance into a Map<String, AttributeValue>?]

If we feel like this is a worthwhile pre-GA API rework, then it should be broken up into a bunch of smaller PRs, with the removal of AttributeValue from the API being the end of that set of work.

So, my proposal is that we re-work all of the Span attributes APIs to be consistent and simpler.

[carlosalberto]

Hey @jkwatson

On 1) and 2): I remember we used to have Link and Event as concrete classes in the API, but this was changed later (it was @bogdandrutu who did the change, so he might remember about this one). In any case we need to keep Event extensible to support MessageEvent?[1]

I personally like having overloads for simple usage, e.g. span.addEvent("hello") instead of forcing the user to create an Event and then add it to the Span - that being said, probably we have too many overloads, so we should think about removing some of them?

Also, observe that Event has no timestamp info, so we couldn't have. a single method for Span.addEvent() (unless we put timestamp in Event).

Around 3) I think our goal so far has been to have immutable classes (aside of Span), and this Attributes class would not be the case, so I'm not sure I like the idea.

Now, I have two questions:

1. How much all of these changes will actually help the end user? Because of the fact this would involve a lot of work and changes in the design, I'm a little wary of whether it's worth the time. It feels like this would help more people implementing the API, which is not that bad (as I don't imagine than a pair of implementations).

2. For the sake of the design change in itself: the argument on removing AttributeValue could probably be made in favor of removing Event and Link, so these classes remain internal. Is there any big difference between removing AttributeValue and the aforementioned classes, other than the simplicity of Attribute?

[1] https://github.com/open-telemetry/opentelemetry-java/pull/81/files

[jkwatson]

In any case we need to keep Event extensible to support MessageEvent?[1]

I think MessageEvent could easily be replaced by a static method for creating an instance of Event, rather than having a separate class for it.

[jkwatson]

With regards to the immutability, we could easily provide builders for these classes, including the Attributes implementation, so the actual classes were immutable.

For me, our current API is very strange. We have these 2 interfaces, but no way to create instances of them. There are different ways of creating Resources, Spans, Links, Events, etc, and we're lacking consistency, which makes the API a bit surprising to use.

I'm very aware that it's late in the game, so maybe we need to start thinking about a 2.0 API, which would make a strongly consistent and discoverable API for these attribute-driven features.

[jkwatson]

I've got some other ideas on how to approach this as a contrib module, so we can prototype API changes before making them part of the core. I'll be doing some exploratory work the next couple days to see if it would work.

[jkwatson]

For anyone interested in contributing/commenting/helping, I've opened up a draft PR where I'll be pushing my exploratory work in a contrib module: #1130

[bogdandrutu]

The main idea behind having these interfaces is to support lazy initialization for the fields (especially for the attributes part) in these objects. As an example someone may write their own implementation of the Event that calculates the attributes only if the Span is recording + does that async if a batch processor is used.

[bogdandrutu]

Happy to support that in a different way, but I think it is a useful think for example I know that in Google they had z-pages enable and all rpc spans were always recording all the events - some were sampled and exporter some just created and recording things and accessed only if someone goes to the z-page

[Oberon00]

So which things can you delay-calculate anyway? Wouldn't that mean keeping around references to objects that would otherwise be garbage-collected after the RPC request is handled to instead live until the span is exported? Checking for IsRecorded is possible without any lazy events, so this concerns only tail sampling, which we don't support. See my comment at open-telemetry/opentelemetry-specification#533 (comment):

I wonder if we should just remove lazy Events from the spec. Since the SDK only supports head-based sampling anyway, a check of the IsRecording property on the caller side should be even more effective in avoiding work.

[jkwatson]

For anyone interested in contributing/commenting/helping, I've opened up a draft PR where I'll be pushing my exploratory work in a contrib module: #1130

I've got 3 different options prototyped for adding Links over in that PR if people are interested in taking a look. I assume that Events options will end up looking very similar.

[Oberon00]

I think this is now resolved by #1631 (#1580).

[jkwatson]

closed via #1631