Refactor AnyValue enum?

[utpilla]

There are a few concerns with the current implementation of AnyValue enum:

1. The spec allows for null or an empty value as a valid value for AnyValue.
2. The current implementation of appender tracing simply records the debug string for complex AnyValue variants such as Hashmap<Key, AnyValue>, Vec<Anyvalue>, and Vec<u8>. We could get hold of the concrete type and do more targeted logging for these types.
3. Use a struct instead of enum as the public interface for representing AnyValue? Currently, AnyValue enum is marked public which means adding any new variant to it in the future would be a breaking change. We could probably use a struct that wraps over AnyValue enum and mark the enum private/internal. That would allow us to easily make changes to the enum, thereby providing us the flexibility to make changes to the implementation easily.

[lalitb]

Thanks @utpilla. Very good points.

Regarding 1 and 2, it's worth testing if the tracing library allows emitting attributes with None (encapsulated within Options) value or values with iterators to HashMap or Vector. Looking into the tracing doc, it seems possible (basically it supports anything which implements Debug trait)

Regarding 3, the struct approach seems good, though it does introduce another level of indirection. We can also evaluate other approaches, such as adding a catch-all Unknown variant. This would ensure new variants can be added without breaking existing code.

[cijothomas]

On 7/9 SIG, we also discussed the need to make AnyValue and Value Enum variants consistent. The former provides conversion for lower size integers, but the latter does not. We should be consistent here.
TBD what is the list of supported types.

[demurgos]

@cijothomas Why is there both Value and AnyValue? As far as I can tell the spec only defines AnyValue as the generic dynamic value type, but the Rust impl has both types in its public API with seemingly no real reason why sometimes it's Value and sometimes it's AnyValue.

I would be willing to send a PR to refactor value handling to use a single consistent type. I also agree with the points in the first message: Null should be fully support and the implementation should better manage its guarantees. I would strongly recommend to not expose the internal representation so it is possible to evolve it.

[lalitb]

Why is there both Value and AnyValue?

This is discussed here - #1253 (comment)

[lalitb]

I also agree with the points in the first message: Null should be fully support and the implementation should better manage its guarantees.

Rust doesn't have a built-in null type. While we can introduce an AnyValue::Null variant, it first needs to be validated how this variant will map from the Value type coming from tracing.

[demurgos]

Thank you very much. I checked in the spec before posting but missed this part. It's a bit confusing to have similar but different value types. I was also confused because the proto definition does not make any distinction there. Here are some other spec discussions that I found about it:

• Support maps and heterogeneous arrays as attribute values opentelemetry-specification#2888
• [Common] Spec inconsistency with proto definition of Attributes opentelemetry-specification#2581

Given the spec links, I see that AnyValue can't be null, only Value.

It would still be good to have some better way to avoid duplication between these types, but it may be more complex than expected.