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

[pdata] `Map.Remove` mutates value returned by `Map.Get`

Open dmitryax opened this issue 3 years ago • 4 comments

Map.Delete doesn't work as a clean map interface and mutates values returned by Map.Get. The following snippet

	m := pcommon.NewMap()
	m.InsertString("k1", "v1")
	m.InsertString("k2", "v2")
	v1, _ := m.Get("k1")
	fmt.Println(v1.StringVal())
	m.Remove("k1")
	fmt.Println(v1.StringVal())

prints

v1
v2

instead of expected

v1
v1

dmitryax avatar Jun 05 '22 00:06 dmitryax

@bogdandrutu I believe this is a side-affect of this change https://github.com/open-telemetry/opentelemetry-collector/pull/2017 . Not sure if it warrants bringing back the following Map representation

type Map struct {
	orig *[]*otlpcommon.KeyValue
}

I would say we should. WDYT?

dmitryax avatar Jun 05 '22 00:06 dmitryax

This is true for other slices, and I think we specifically told (was there at one point at least if I remember) in the description that instances returned by At and Get will be invalid if Remove or even add is called.

bogdandrutu avatar Jun 06 '22 15:06 bogdandrutu

@dmitryax without that optimization we spend lots and lots of CPU allocating 1000s of small objects :))

bogdandrutu avatar Jun 06 '22 15:06 bogdandrutu

BTW, this and other optimization won't be necessary and won't cause problems like this if we move to https://github.com/splunk/exp-lazyproto :-)

tigrannajaryan avatar Aug 15 '22 22:08 tigrannajaryan

Removing from 1.0 milestone since fixing it is not a breaking change for the pdata API

dmitryax avatar Nov 09 '22 01:11 dmitryax