opentelemetry-collector icon indicating copy to clipboard operation
opentelemetry-collector copied to clipboard

[pdata] Revisit primitive slice length mutation API

Open dmitryax opened this issue 3 years ago • 5 comments
trafficstars

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

dmitryax avatar Sep 16 '22 16:09 dmitryax

Here is the commit with SetLen implementation for the reference https://github.com/open-telemetry/opentelemetry-collector/commit/965858eb3972b9588e82766a428fd61b3a2e0656

dmitryax avatar Sep 16 '22 16:09 dmitryax

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 avatar Sep 16 '22 16:09 dashpole

@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.

bogdandrutu avatar Sep 16 '22 17:09 bogdandrutu

@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 avatar Sep 16 '22 17:09 dmitryax

@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)

bogdandrutu avatar Sep 16 '22 17:09 bogdandrutu

Sounds good to me. Added such documentation in https://github.com/open-telemetry/opentelemetry-collector/pull/6150

dmitryax avatar Sep 26 '22 18:09 dmitryax