Do we need MoveAndAppendTo or can we replaces with simpler MoveTo for consistency?

[bogdandrutu]

No description provided.

[dmitryax]

MoveTo always replaces the destination, but MoveAndAppendTo appends new elements. I think we should have distinctive names for those purposes, maybe not to verbose?

[bogdandrutu]

But do we need the one with the Append functionality at this point?

[dmitryax]

You mean AppendEmpty? It cannot be replaces with a one-liner. User would have to iterate over the source slice with At(i).CopyTo(dest.AppendEmpty())

[bogdandrutu]

Look at these PRs:

• [chore] remove unnecessary allocations in routingprocessor opentelemetry-collector-contrib#14546
• [chore] remove unnecessary allocations in groupbyattrsprocessor opentelemetry-collector-contrib#14547
• [chore] remove unnecessary allocations in awsfirehosereceiver opentelemetry-collector-contrib#14548
• [chore] remove unnecessary allocations in fluentforwardreceiver opentelemetry-collector-contrib#14549
• [chore] remove usage of MoveAndAppend in test where easy to replace opentelemetry-collector-contrib#14550
• [chore] remove unnecessary allocations in translator/jaeger opentelemetry-collector-contrib#14551
• [chore] remove unnecessary allocations in metricstransformprocessor opentelemetry-collector-contrib#14552

What I am saying is that I believe we need a "MoveTo" version, and the MoveAndAppendTo showed be replaced if possible with multiple AppendEmpty. There are couple more usages of MoveAndAppendTo, will look into that soon.

[dmitryax]

Sound good. I believe, if we confirm that MoveAndAppendTo is redundant in contrib, we can remove it

[bogdandrutu]

Few more PRs:

• [chore] Remove usage of MoveAndAppend in dockerstatsreceiver opentelemetry-collector-contrib#14621
• [chore] Remove usage of MoveAndAppend in tailsamplingprocessor opentelemetry-collector-contrib#14622

[bogdandrutu]

Few more usages removed:

• [chore] metricstransformprocessor: avoid unnecessary new metrics slice when possible opentelemetry-collector-contrib#14628
• [chore] loadbalancingexporter fix small nits, remove usage of MoveAndAppend opentelemetry-collector-contrib#14629

[bogdandrutu]

My investigation led to the conclusion that there are few cases where the "MoveAndAppend" is useful, see the usages:

• https://github.com/open-telemetry/opentelemetry-collector-contrib/blob/45142e9320d940deba9e5889a9daa437200437a2/processor/metricstransformprocessor/operation_aggregate_labels.go#L254

• opentelemetry-collector/processor/batchprocessor/batch_processor.go

  Line 242 in f0d69d6

  td.ResourceSpans().MoveAndAppendTo(bt.traceData.ResourceSpans())

Because of that, I believe we may want to keep it. The only things left are to investigate:

1. Do we also need a MoveTo func as well? I believe this can be done also by adding a Clear func, and combine that with the current MoveAndAppendTo. We probably can postpone this.
2. Is the MoveAndAppendTo a good enough name? I believe it is decent, unless anyone has a better suggestion I would not change it.

@open-telemetry/collector-approvers

[dmitryax]

I agree that name MoveAndAppendTo is good enough.

Currently we have only CopyTo and MoveAndAppendTo for slices. Maybe it make sense to have MoveTo, CopyTo, MoveAndAppendTo and CopyAndAppendTo for consistency? It'll be move clear that the first two override the destination. Or they can be removed in favor of Clear.

Also pcommon.Map currently has CopyTo only. I've seen a few places where merge capability would be pretty useful in contrib. So maybe we can add MoveTo, MoveAndPutTo, CopyAndPutTo there?

[bogdandrutu]

Is anything right now that blocks us to evolve in the future? Adding new funcs can always happen later, the problem is with removing, so do we want to remove any of them?

[dmitryax]

Right. I don't see any method that should be removed at this point.

[bogdandrutu]

Then we can move this after the release?

[dmitryax]

Then we can move this after the release?

Sure, no rush needed with that. No breaking changes expected

[bogdandrutu]

I think this issue is now complete. If needed in the future to extend the functionality we can reopen.