[pdata] Revisit need of Val suffix of Value methods

[dmitryax]

Currently all the setter and getter methods of pcommon.Value end with Val suffix, e.g.: Value.BoolVal, Value.SetBoolVal, Value.MapVal etc. The only reason for having the suffix is that otherwise we end up with a Value.String method unintentionally implementing fmt.Stringer interface.

Potentially we can remove the suffix from all the methods except for Value.StringVal where we can add an explicit note about the naming. I'm not sure if it's a good enough reason to introduce this exception.

We can also keep the existing naming. I just wanted to bring this for discussion before releasing pdata 1.0. I don't have a strong preference.

@open-telemetry/collector-approvers please share your thoughts

[bogdandrutu]

Another option is to ignore the fmt.Stringer implementation problem and just use String.

[bogdandrutu]

Another option is to call GetString?

[dmitryax]

Get prefix goes against go naming recommendations, but I still like it more than other options.

If we ignore fmt.Stringer, Value of any types other than string will be printed as "" and we won't be able to change that, so for me it's the least desirable option

[dmitryax]

I also like Get because it's getting more explicit that you can get anything from Value and in most cases it's a zero/invalid result.

[bogdandrutu]

Yes, if we go with GetX, we should not panic if not correct type, and return zero initialized value.

[dmitryax]

Yes, if we go with GetX, we should not panic if not correct type, and return zero initialized value.

This is what we are doing now, it's all zero values, which also means invalid values for complex types like Map{} and Slice{}.

[dmitryax]

There are 3 different parts of the related pdata API that we need to consider additionally in this issue.

1. pcommon.Value (https:/open-telemetry/opentelemetry-collector/blob/main/pdata/pcommon/common.go#L70-L321) which this issue is submitted for initially. It's a wrapper over a proto oneof message called Value. All getters/setters methods have Val suffix just to avoid implementing fmt.Stringer interface, but getter for type doesn't have any prefix.

2. pmetric.NumberDataPoint (https:/open-telemetry/opentelemetry-collector/blob/main/pdata/pmetric/generated_metrics.go#L1254-L1288) and pmetric.Exemplar (https:/open-telemetry/opentelemetry-collector/blob/main/pdata/pmetric/generated_metrics.go#L2619-L2653) that wrap a proto message having a oneof message called the Value as a field, but doesn't wrap the a oneof message itself as done in 1. So we don't deal with the oneof message directly and probably should have Value part in all the related methods. Currently we have suffix Val for getter/settes methods and Value prefix for type accessor.

3. pmetric.Metric (https:/open-telemetry/opentelemetry-collector/blob/main/pdata/pmetric/generated_metrics.go#L633-L774). Similar to 2, it wraps a proto message having a oneof message called Data as a field. Currently we don't have any prefixes for getter/setter methods but do have Data prefix in the type getter.

Given the 1 and 2/3 are pretty different, I don't think necessary to have method names consistent across them, but we probably need to follow the same pattern for 2 and 3.

Approach A

One solution that brings consistency between 2 and 3 setters / getters / type getters is to have the following interface:

1. Use Get/Set prefix instead of Val suffix, keep type accessor as is:

func (v Value) Type() ValueType
func (v Value) GetString() string
func (v Value) SetString(val)
func (v Value) GetBool() bool
func (v Value) SetBool(val)
func (v Value) GetMap() Map
func (v Value) SetEmptyMap() Map
func (v Value) SetEmpty() Value
...

2. Use Value suffix instead of Val for getter/setter to be more explicit and for consistency with type accessor:

func (ms NumberDataPoint) ValueType() NumberDataPointValueType
func (ms NumberDataPoint) DoubleValue() float64
func (ms NumberDataPoint) SetDoubleValue(v float64)
func (ms NumberDataPoint) IntValue() int64
func (ms NumberDataPoint) SetIntValue(v int64)

3. Add Data suffix to be consistent with 2 and the type accessor:

func (ms Metric) DataType() MetricDataType
func (ms Metric) GaugeData() Gauge
func (ms Metric) SetEmptyGaugeData() Gauge
func (ms Metric) SumData() Sum
func (ms Metric) SetEmptySumData() Sum

An open question for this approach whether we want to add Get prefix to 2 and 3 for consistency with 1.

The problem is with this option is that setters like SetEmptyGaugeData maybe doesn't sound nice and we would probably need to consider if we want to keep Gauge and Sum struct names or rename them to GaugeData/SumData.

Approach B

We can ignore that 2 and 3 goes through a oneof message proxy and apply the same naming as we have in 1:

2 will be:

func (ms NumberDataPoint) Type() NumberDataPointValueType
func (ms NumberDataPoint) GetDouble() float64
func (ms NumberDataPoint) SetDouble(v float64)
func (ms NumberDataPoint) GetInt() int64
func (ms NumberDataPoint) SetInt(v int64)

3 will be:

func (ms Metric) Type() MetricDataType
func (ms Metric) GetGauge() Gauge
func (ms Metric) SetEmptyGauge() Gauge
func (ms Metric) GetSum() Sum
func (ms Metric) SetEmptySum() Sum

The problem with this option is that we potentially can run into a conflict with other fields that possibly can be added to 2 and 3 structs in future. Also func (ms Metric) Type() would makes us reconsider type name MetricDataType, same for 2.

Approach C

Apply approach A with just one exception for getter/setter name that brings inconsistency but provides more user friendly naming.

1 and 2 will be changed as defined in the approach A, but 3 will be kept at it currently is:

3 will be:

func (ms Metric) DataType() MetricDataType
func (ms Metric) GetGauge() Gauge
func (ms Metric) SetEmptyGauge() Gauge
func (ms Metric) Sum() Sum
func (ms Metric) SetEmptySum() Sum

Same as for approach A, it's an open question if we want to add Get prefix to 2 and 3 for consistency with 1.

Approach D

Another otpion is to bring additional structs representing the oneof protobuf message similar to pcommon.Value: NumberValue for 2 and MetricData for 3. It'll allow us to bring consistency across all three pdata parts, but introduces another probably redundant entity making the user interface less convenient.

@open-telemetry/collector-approvers please share your thoughts which option sounds better to you

Bogdan's Edites:

• Removed reference to key form Approach A, probably confused with the Map API.

[TylerHelmuth]

@dmitryax in Approach C you have func (ms Metric) GetGauge() Gauge but also func (ms Metric) Sum() Sum. For gauge, did you mean func (ms Metric) Gauge() Gauge?

[bogdandrutu]

@dmitryax thanks for the writeup this helps a lot.

My thinking is a bit different about the Metric. I think the data part in the Metric proto is an unfortunate requirement from the protobuf library to have a name for the oneof field, and I think that actually the oneof refers to the metric itself. Will try to push a PR to rename data to metric to make it more clear.

Because of that I believe that for metric, I prefer:

func (ms Metric) Type() MetricType
func (ms Metric) Gauge() Gauge
func (ms Metric) SetEmptyGauge() Gauge
func (ms Metric) Sum() Sum
func (ms Metric) SetEmptySum() Sum

MetricDataType -> MetricType
Metric.DataType() -> Metric.Type()

[TylerHelmuth]

For Approach B part 2, I am missing the risk of

func (ms NumberDataPoint) GetDouble() float64
func (ms NumberDataPoint) SetDouble(v float64)
func (ms NumberDataPoint) GetInt() int64
func (ms NumberDataPoint) SetInt(v int64)

It feels to me like func (ms NumberDataPoint) DoubleVal() float64 is already obfuscating the fact that behind the scenes we're interacting with a struct instead of a float64. func (ms NumberDataPoint) GetDouble() float64 looks like a rename to me, not a change in risk, unless I'm missing something.

[bogdandrutu]

Having the Value in the name suggest that this is a Double entry inside a oneof named Value. But that does not help with conflicts, I would double check but I believe you cannot have another member called Double outside the oneof in the same message, because moving out and inside a oneof is a no op on the wire.

[dmitryax]

Generally, I'm leaning towards Approach B. But still not sure if we need to bring Get to use cases 2 and 3, maybe we can leave Get only for 1.

If we go with Approach B, I'd like to make sure that it's ok to think about NumberDataPoint/Exemplar as a value itself because it's not only about possible conflicts, it may be that [Get|Set][Double|Int] name just doesn't provide enough context to the user in NumberDataPoint/Exemplar struct.

[bogdandrutu]

Bogdan's summary

Rules

1. All the following rules refer to ${OneofName} as the "oneof" group name. It is used to avoid conflicts and to better understand that the type/getter/setter refers to a value within that group. When the oneof group values refer to the message itself, the ${OneofName} should be skipped to avoid duplicate words (e.g. type ValueValueType int32).
2. A type enum has to be created (skipping the ${OneofName} is based on rule 1) with the name ${MessageName}${OneofName}Type which is returned via a func ${MessageName}.${OneofName}Type().
3. Getters are named and defined as (skipping the ${OneofName} is based on rule 1) ${MessageName}.${OneofMemberName}${OneofName}() ${OneofMemberValue}. Getters MUST not panic, and in case of a miss type, a zero initialized object is returned. Accessing a zero initialized object may panic though.
4. Setters are named and defined as (skipping the ${OneofName} is based on rule 1):
   • For immutable types ${MessageName}.Set${OneofMemberName}${OneofName}(${OneofMemberValue})
   • For mutable types define a setter that initializes with an empty value and returns the reference to that value ${MessageName}.SetEmpty${OneofMemberName}${OneofName}() ${OneofMemberValue}

Results

pcommon.Value

• Ignore the oneof "data" name, since this falls under the rule 1 exception.
• Exception from the rule 3: use "Get" prefix to avoid conflict with fmt.Stringer.

func (v Value) Type() ValueType
func (v Value) GetString() string
func (v Value) SetString(val)
func (v Value) GetBool() bool
func (v Value) SetBool(val)
func (v Value) GetMap() Map
func (v Value) SetEmptyMap() Map
...

pmetric.NumberDataPoint

• Use Value suffix (change compare to current Val) since that is the rule 1.

func (ms NumberDataPoint) ValueType() NumberDataPointValueType
func (ms NumberDataPoint) DoubleValue() float64
func (ms NumberDataPoint) SetDoubleValue(v float64)
func (ms NumberDataPoint) IntValue() int64
func (ms NumberDataPoint) SetIntValue(v int64)

pmetric.Metric

• Ignore the oneof "data" name, since this falls under the rule 1 exception:

func (ms Metric) Type() MetricType
func (ms Metric) Gauge() Gauge
func (ms Metric) SetEmptyGauge() Gauge
func (ms Metric) Sum() Sum
func (ms Metric) SetEmptySum() Sum

Action Items

1. Approve the proposed rules and maybe document them (we can followup and don't have to block progress on this because of missing documentation, since it is documented here).
2. Decide on the Get prefix exception OR we ignore the unfortunate conflict with fmt.Stringer?
3. Start implementing changes.

[dmitryax]

@bogdandrutu thanks for providing the summary. I like the proposal. I would vote for adding Get prefix as an exception to the Value struct to avoid implementing fmt.Stringer

[bogdandrutu]

Personally, I am not convinced about the Get, but we have lots of other things to do for pmetric.Metric and pmetric.NumberDataPoint so we can start with these, and keep thinking for 1-2 days on the Get or other alternative for that.

[dmitryax]

@bogdandrutu thanks for taking care of the issues discovered above.

The only thing left is to decide if we go with the Get prefix as an exception for Value to avoid implementing fmt.Stringer.

My vote is that we shouldn't implement it unintentionally. But at the same time, I don't like that we add Get prefix to all the Value getters. It doesn't highlight the intention to avoid having String method, but maybe brings even more confusion.

Maybe we can choose another name for string getter/setter? I don't think it's required to call it String. We don't have any naming consistency with the return type, e.g. we have Bytes for ByteSlice, Int for int64, Double for float64. Maybe we can use something like Str for string?

[bogdandrutu]

I like Str option.

[TylerHelmuth]

I like Str. would the final API look like:

func (v Value) Type() ValueType
func (v Value) GetStr() string
func (v Value) SetStr(val)
func (v Value) GetBool() bool
func (v Value) SetBool(val)
func (v Value) GetMap() Map
func (v Value) SetEmptyMap() Map
func (v Value) SetEmpty() Value

func (ms NumberDataPoint) ValueType() NumberDataPointValueType
func (ms NumberDataPoint) DoubleValue() float64
func (ms NumberDataPoint) SetDoubleValue(v float64)
func (ms NumberDataPoint) IntValue() int64
func (ms NumberDataPoint) SetIntValue(v int64)

func (ms Metric) Type() MetricType
func (ms Metric) Gauge() Gauge
func (ms Metric) SetEmptyGauge() Gauge
func (ms Metric) Sum() Sum
func (ms Metric) SetEmptySum() Sum

[dmitryax]

@TylerHelmuth , no we won't have Get prefix for Value getters in that case. It'll be:

func (v Value) Type() ValueType
func (v Value) Str() string
func (v Value) SetStr(val)
func (v Value) Bool() bool
func (v Value) SetBool(val)
func (v Value) Map() Map
func (v Value) SetEmptyMap() Map
func (v Value) SetEmpty() Value

[TylerHelmuth]

@dmitryax sorry, meant to remove all the Gets. That's what I get for multitasking.

[dmitryax]

Ok looks like we have an agreement here. Updated #6099 accordingly