open-telemetry / opentelemetry-collector

OpenTelemetry Collector
https://opentelemetry.io
Apache License 2.0
4.32k stars 1.43k forks source link

Interesting gotcha in `pcommon.Map` that likely could use documenting #10366

Open skandragon opened 3 months ago

skandragon commented 3 months ago

Problem Statement

This is framed as a feature request, but it is really more of a documentation request I think.

I recommend adding something to the effect of "the return value from Get() is only valid until the Map is modified."

Here's a simple test to show the behavior that bit me hard last night:

func TestMap(t *testing.T) {
    m := pcommon.NewMap()
    m.PutStr("key1", "value1")
    m.PutStr("key2", "value2")

    v, ok := m.Get("key1")
    assert.True(t, ok)
    assert.Equal(t, "value1", v.AsString())
    m.Remove("key1")
    assert.Equal(t, "value1", v.AsString()) // <- fails to match
}

This test will fail because the second equality check will find "value2" stored in v.

In my case, I was doing something like "find the key, get the value, remove the entry, and return v.AsString(). This was returning a random-seeming value from the Map and it took some time to find out why. When I wrote a simple test that inserted the value I wanted to find, it worked. Same with the key not being present.

Proposed Solution

Add a warning to Get() and Remove() about this side effect.

Alternatives

Fixing the code to make this not happen, but that seems like its not necessary if a warning is in place.

mx-psi commented 3 months ago

Relates to #5588 and #5476. Improving docs was suggested here https://github.com/open-telemetry/opentelemetry-collector/issues/5588#issuecomment-1165608022