Avoid using Attribute in common pdata components related to AnyValue proto field

[dmitryax]

Problems

All of the pdata wrappers related to AnyValue fields include Attribute part in its name because the field used to be applicable to the attributes fields only, like here. But it's not the case anymore since pdata.LogRecord.Body uses AnyValue wrapped by pdata.AttributeValue. This is the main goal that intended to be addressed in #4818.

But the are couple of other naming issues related to this problem that need to be addressed:

1. AttributeMap naming

pdata.AttributeMap struct is a wrapper around repeated KeyValue proto field. The name seems reasonable when it's used for attributes like here. But it also can be used as an instance of AnyValue. For example the following line currently returns a value of AttributeMap type while there is nothing related to attribute in the call:

pdata.NewLogs().ResourceLogs().AppendEmpty().LogRecords().AppendEmpty().Body().MapVal()

2. AttributeValueSlice naming

There are couple of problem with this name:

1. It can be easily confused with all the other AttributeValue... types like AttributeValueString, AttributeValueInt, AttributeValueMap, AttributeValueArray, etc. They both have the same naming schema but different meaning:

• AttributeValue<type> is a wrapper of AnyValue with a particular <type> .
• AttributeValueSlice is a slice of AnyValue wrappers.

It's especially confusing to have both AttributeValueArray and AttributeValueSlice.

2. AttributeValueSlice (wrapper over an array of AnyValue fields) is similar to AttributesMap (wrapper over a key-value collection of AnyValue). Therefore they should have similar naming. This is why the both of the problems described in this issue should be considered together.

Possible solutions

We cannot follow the approach proposed in #4818 and rename AttributeValueSlice to AnyValueSlice. It would require renaming AttributeMap to AnyValueMap which will conflict with existing AnyValueMap (wrapper of AnyValue with KeyValueList type).

Therefore we need to define some consistent naming for AttributeMap and AttributeValueSlice that is different from what is proposed in #4818. There are two possible options:

1. Use AnyValue prefix for pdata.AttributeValueSlice and pdata.AttributeMap

Use AnyValue prefix for pdata.AttributeValueSlice and pdata.AttributeMap instead of using it for AttributeValue<type>. It means that another naming schema has to be found for AttributeValue<type> that is currently proposed to be renamed as AnyValue<type> in #4818 to avoid collisions.

• pdata.AttributeValueSlice becomes pdata.AnyValueSlice: slice of AnyValue wrappers.
• pdata.AttributeMap becomes pdata.AnyValueMap: map of AnyValue wrappers.

2. Drop Attribute part in pdata.AttributeValueSlice and pdata.AttributeMap

pdata.AttributeValueSlice and pdata.AttributeMap can be renamed to pdata.Slice and pdata.Map. This approach seems to be working well. pdata.Slice and pdata.Map should be descriptive enough.

The only concern with this approach is that pdata.NewAttributeMapFromMap function becomes pdata.NewMapFromMap after straight-forward renaming. We probably need another name for this function. Maybe pdata.NewMapFromRawMap.

3. Use Slice and KeyValueSlice names instead of AttributeValueSlice and AttributeMap

pdata.KeyValueSlice name may seem not ideal, but it represents its internals and works well with all other names.
pdata.NewAttributeMapFromMap function becomes pdata.NewKeyValueSliceFromMap then.

---

Any other ideas are very welcome.

[bogdandrutu]

Another option that I can think of:

• pdata.AnyValue -> pdata.Value
• pdata.Map
• pdata.Slice

Different idea:
When it comes to the "New" funcs, I think generics would make this API much nicer NewValue(int64|float64|...). An option for us (in order to not require golang 18) may be to move the New funcs in a helper package that we never mark 1.0 until generics can be used.

[dmitryax]

@bogdandrutu we will have the following types then, right?

const (
	ValueTypeEmpty ValueType = iota
	ValueTypeString
	ValueTypeInt
	ValueTypeDouble
	ValueTypeBool
	ValueTypeMap
	ValueTypeArray
	ValueTypeBytes
)

It looks good to me, I don't think we can anticipate any conflicts in future.

Not sure the generic function NewValue(int64|float64|...) will work with this. Even if it works, it probably doesn't help to reduce the code and IMO can make it more confusing to use.

[bogdandrutu]

+1 on the type part.

For the generics, I would like to look how will that will be, not 100% sure about all the generics tricks. We can discuss that separately.

[dmitryax]

Submitted PR for this issue #5001 and updated #4978

[dmitryax]

Once #5001 is merge we will get the following types:

AnyValue wrappers:

const (
	ValueTypeEmpty ValueType = iota
	ValueTypeString
	ValueTypeInt
	ValueTypeDouble
	ValueTypeBool
	ValueTypeMap
	ValueTypeArray
	ValueTypeBytes
)

AnyValue collection wrappers :

pdata.Slice
pdata.Map

So we will have pdata.NewValueMap().MapVal() returning a pdata.Map value.
But pdata.NewValueArray().SliceVal() returning a pdata.Slice value.

Not sure if we need to rename pdata.ValueArray or pdata.Slice to align them to the same name.

@bogdandrutu let me know WDYT

[bogdandrutu]

But pdata.NewValueArray().SliceVal() returning a pdata.Slice value.

Should we NewValueArray to NewValueSlice?

[dmitryax]

Should we NewValueArray to NewValueSlice?

Yes, I think renaming pdata.ValueArray to pdata.ValueSlice is better than renaming pdata.Slice to pdata.Array.

The only concern is that pdata.ValueType.String() will change its behavior. It'll be returning "SLICE" instead of "ARRAY" value. Technically it's a breaking change, but I believe we can make another exception here.

cc @bogdandrutu @Aneurysm9

[Aneurysm9]

Do we have a constant defined for that string that is typically used? If so and we update it simultaneously that might not be so bad, but if not we should look at what other options we have. Is it the case that the pdata.ValueType value is typically compared directly and its stringification used only for log/debug output?

[dmitryax]

Is it the case that the pdata.ValueType value is typically compared directly and its stringification used only for log/debug output?

Yes it is. Result of pdata.ValueType.String() call is used in some output like here. String "ARRAY" isn't used as identifier anywhere. pdata.ValueTypeArray is used identifier instead. It's an iota constant which is should be fine to replace with pdata.ValueTypeSlice keeping pdata.ValueTypeArray at the end for the deprecation period I think.

[Aneurysm9]

It's an iota constant which is should be fine to replace with pdata.ValueTypeSlice keeping pdata.ValueTypeArray at the end for the deprecation period I think.

Maybe change the value in the iota block to ValueTypeSlice and later on have:

const ValueTypeArray = ValueTypeSlice

They're semantically identical and shouldn't have different values, I don't think.

[dmitryax]

Good idea. Thanks!

[dmitryax]

The initial problem stated in this ticket is resolved now. I created another follow-up issue for the ValueTypeArray renaming: #5065