[pdata] Map `Remove` and `RemoveIf` implementations have unforeseen side-effects

[mdonkers]

trafficstars

Component(s)

pdata

What happened?

Describe the bug The way the Remove and RemoveIf functions are implemented for pcommon.Map, they lead to unexpected side-effects that could have impact on the integrity of the data if not aware. A pcommon.Value that first is retrieved from a map, then gets removed from said map, will have its value changed. So then re-using the value can lead to bad consequences.

The following minimal test shows the issue:

func Test_minimal_reproducer(t *testing.T) {
	logs := plog.NewLogs()
	resourceLogs1 := logs.ResourceLogs().AppendEmpty()
	attrs1 := resourceLogs1.Resource().Attributes()
	attrs1.PutStr("attr.to.move", "mover")
	attrs1.PutStr("service.name", "dummy-service")
	attrs1.PutStr("service.namespace", "dash0")

	// Create a second Resource
	resourceLogs2 := logs.ResourceLogs().AppendEmpty()
	attrs2 := resourceLogs2.Resource().Attributes()
	attrs2.PutStr("service.name", "another-dummy-service")
	attrs2.PutStr("service.namespace", "dash0")

	// Now we pick up the attribute and remove from the first Resource, then add to the second Resource
	// Normally this would happen e.g. in some loop or other logic, but shortened just to make the issue clear
	if v, ok := logs.ResourceLogs().At(0).Resource().Attributes().Get("attr.to.move"); ok {
		// !! the Remove happens FIRST !!
		logs.ResourceLogs().At(0).Resource().Attributes().Remove("attr.to.move")

		logs.ResourceLogs().At(1).Resource().Attributes().PutStr("attr.to.move", v.AsString())
	}

	movedValue, ok := logs.ResourceLogs().At(1).Resource().Attributes().Get("attr.to.move")
	assert.True(t, ok)
	assert.Equal(t, "mover", movedValue.AsString()) // FAILS, value is "dash0" from the service.namespace
}

By the time the value for attr.to.move gets inserted into the second Resource attributes, due to the Remove called before, the value is no longer mover but dash0.

I understand why the Remove and RemoveIf are implemented that way, as not to have to move every key-value pair in the slice. But I don't think such side-effects are acceptable.

Steps to reproduce See the above test case for a reproducer

What did you expect to see? The internal value of the pcommon.Value object not getting updated whenever that entry is removed from a map.

What did you see instead? The internal value getting changed from mover to dash0 (which is the value belonging to the service.namespace attribute).

Collector version

v1.32.0

Environment information

not applicable

OpenTelemetry Collector configuration

not applicable

Log output

=== RUN   Test_minimal_reproducer
    attributes_lower_transformer_test.go:356: 
        	Error Trace:	/Development/projects/Dash0/dash0/components/collector/processor/dash0schemaprocessor/internal/transformer/attributelower/attributes_lower_transformer_test.go:356
        	Error:      	Not equal: 
        	            	expected: "mover"
        	            	actual  : "dash0"
        	            	
        	            	Diff:
        	            	--- Expected
        	            	+++ Actual
        	            	@@ -1 +1 @@
        	            	-mover
        	            	+dash0
        	Test:       	Test_minimal_reproducer
--- FAIL: Test_minimal_reproducer (0.00s)

Additional context

One option would be copying the value:

vCopy := pcommon.NewValueEmpty()
v.CopyTo(vCopy)

But also for this, users need to be aware and its easily forgotten / overlooked.

[github-actions[bot]]

Pinging code owners:

• pdata: @BogdanDrutu @dmitryax

See Adding Labels via Comments if you do not have permissions to add labels yourself.