opentelemetry-collector
opentelemetry-collector copied to clipboard
[pdata] Revisit primitive slice length mutation API
Following the discussion in https://github.com/GoogleCloudPlatform/opentelemetry-operations-go/issues/479 is seems like EnsureCapacity + Append maybe not that intuitive to use when we have SetAt interface. Other slices don't have SetAt, so it maybe unnecessary to provide the same API for length mutation as they have, and we can introduce different API to update slice length. SetLen still seems a pretty good option to me, in that case I would remove Append and EnsureCapacity.
cc @bogdandrutu @dashpole
Here is the commit with SetLen implementation for the reference https://github.com/open-telemetry/opentelemetry-collector/commit/965858eb3972b9588e82766a428fd61b3a2e0656
I think there is a difference between EnsureCapacity + Append vs SetLen + SetAt if we need to not add an item, right? It seems better to encourage EnsureCapacity + Append so there aren't empty elements in the list?
e.g.
EnsureCapacityOrSetLen(len(foo))
for i := 0; i < len(foo); i++ {
If foo[i].IsBad() {
continue
}
AppendOrSetAt(i)
}
@dashpole we also want to have a way to modify an existing value. For example if you have to merge a new point into an existing one.
@dashpole this is a good example for append when we fill the slice from another slice. But if we want to filter the pdata slice in place we cannot do it without SetLen or another allocation. I'm not sure if that's a very popular use case. Maybe we can have both SetLen and EnsureCapacity/Append
@dmitryax I think we should do this (from the linked issue):
Document "slice equivalent" of pdata functions (e.g. append() for Append(), make([]foo, 0, 5) for EnsureCapacity, foo[i] = bar for SetAt)
Sounds good to me. Added such documentation in https://github.com/open-telemetry/opentelemetry-collector/pull/6150