[pdata] Remove Val suffix from pcommon.Value getter/setter methods

[dmitryax]

Resolves #6081

[codecov]

Codecov Report

Base: 91.91% // Head: 91.77% // Decreases project coverage by -0.13% ⚠️

Coverage data is based on head (646d1d4) compared to base (a511b16).
Patch coverage: 75.26% of modified lines in pull request are covered.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6099      +/-   ##
==========================================
- Coverage   91.91%   91.77%   -0.14%     
==========================================
  Files         217      217              
  Lines       13317    13345      +28     
==========================================
+ Hits        12240    12248       +8     
- Misses        848      868      +20     
  Partials      229      229              

Impacted Files	Coverage Δ
pdata/pcommon/generated_primitive_slice.go	100.00% <ø> (ø)
pdata/pcommon/common.go	89.92% <73.25%> (-3.60%)	⬇️
...er/loggingexporter/internal/otlptext/databuffer.go	99.12% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

[bogdandrutu]

I like the new API... Want to hear more opinions. @open-telemetry/collector-approvers ?

[dmitryax]

@bogdandrutu If we merge this, we will still have (Set)[Double|Int]Val methods on pmetric.NumberDataPoint and pmetric.Examplar structs, but I believe it's good to keep the Val suffix there because they have other unrelated fields and they look through another Value entity, so we should keep something related to value in those method names.

[bogdandrutu]

Maybe that should be Value?

[bogdandrutu]

@dmitryax what about the metrics data type then? We definitely are not consistent here

[tigrannajaryan]

I am not sure this is an improvement. Get prefix is explicitly not recommended by Effective Go for getters. Granted, Val suffix is also not pretty.

Minor argument in favour of Val suffix: it is faster to use with autocomplete, less characters to type until you get the right hint.

However, I can live with either approach.

[TylerHelmuth]

I like this a lot. Get and Set feel like natural prefixes for this situation, even if it goes against Go naming standards

[dmitryax]

Maybe that should be Value?

Yes, (Set)[Double|Int]Value on pmetric.NumberDataPoint and pmetric.Examplar sounds better to me 👍

@dmitryax what about the metrics data type then? We definitely are not consistent here

We are not, yes. We don't have any prefix or suffix for the metric type getters, it's just Sum(), Gauge() etc. It goes through a oneof protobuf message called Data. I'm not sure what we can do there to bring consistency. Making it SumValue or SumData doesn't feel convenient.

[TylerHelmuth]

We definitely are not consistent here

This is pretty annoying, and if we can't use Get|Set everywhere then I'd probably vote for consistency over anything else. But if I was a new user to the pdata API, I think GetString and SetString are nicer than StringVal and SetStringVal.

But I also have a bias towards languages with Getters and Setters. An argument could be made that for Go Get isn't the expected name and would throw users off.

[TylerHelmuth]

it's just Sum(), Gauge()

This makes me really wish we could just do SetString and String. GetSum and GetGauge don't feel right. (Which is maybe a good argument against using Get for Value.)

[bogdandrutu]

@dmitryax please summarize the conversation in the issue:

• 4 places where we have this pattern right now (please enumerate them, and add links so people can find them easily), and all somehow inconsistent for naming.
• Let's play with the proposals: 1. use get/set (see how that impact other places); 2. In case of Metric/Exemplar should we have SumData to be consistent with DoubleValue?

So let's enumerate the rules that we want to apply (or exceptions if any) and apply them and see what we get. By apply just some code snippets it is fine.

[jpkrohling]

I preferred BoolVal to GetBool until I realized that Get would be a better pair to Set in SetBool.

[dmitryax]

Please do not merge until a decision is made on #6081 (comment)

[bogdandrutu]

To avoid merging until decision in the issue is made

[bogdandrutu]

Consider to have this comment on the "SetStr" as well.