From f5c3129c113665f58c367d9772d59ed2c4177aa9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tim=20R=C3=BChsen?= Date: Thu, 3 Apr 2025 12:05:42 +0200 Subject: [PATCH 01/11] Introduce pprofile.PutAttribute helper --- .chloggen/profiles-PutAttributes.yaml | 25 ++++++ pdata/pprofile/attributes.go | 58 ++++++++++++++ pdata/pprofile/attributes_test.go | 111 ++++++++++++++++++++++++++ 3 files changed, 194 insertions(+) create mode 100644 .chloggen/profiles-PutAttributes.yaml diff --git a/.chloggen/profiles-PutAttributes.yaml b/.chloggen/profiles-PutAttributes.yaml new file mode 100644 index 000000000000..4532251f7745 --- /dev/null +++ b/.chloggen/profiles-PutAttributes.yaml @@ -0,0 +1,25 @@ +# Use this changelog template to create an entry for release notes. + +# One of 'breaking', 'deprecation', 'new_component', 'enhancement', 'bug_fix' +change_type: enhancement + +# The name of the component, or a single word describing the area of concern, (e.g. otlpreceiver) +component: pdata/profile + +# A brief description of the change. Surround your text with quotes ("") if it needs to start with a backtick (`). +note: Introduce PutAttribute helper method to modify the content of attributable records. + +# One or more tracking issues or pull requests related to the change +issues: [12798] + +# (Optional) One or more lines of additional information to render under the primary note. +# These lines will be padded with 2 spaces and then inserted directly into the document. +# Use pipe (|) for multiline entries. +subtext: + +# Optional: The change log or logs in which this entry should be included. +# e.g. '[user]' or '[user, api]' +# Include 'user' if the change is relevant to end users. +# Include 'api' if there is a change to a library API. +# Default: '[user]' +change_logs: [api] diff --git a/pdata/pprofile/attributes.go b/pdata/pprofile/attributes.go index 34bb88cc2efe..21f20ef1d2ea 100644 --- a/pdata/pprofile/attributes.go +++ b/pdata/pprofile/attributes.go @@ -66,3 +66,61 @@ func AddAttribute(table AttributeTableSlice, record attributable, key string, va return nil } + +// PutAttribute updates an AttributeTable and a record's AttributeIndices to +// add a new attribute. +// The assumption is that attributes are a map as for other signals (metrics, logs, etc.), thus +// the same key must not appear twice in a list of attributes / attribute indices. +// The record can be any struct that implements an `AttributeIndices` method. +func PutAttribute(table AttributeTableSlice, record attributable, key string, value pcommon.Value) error { + for i := range record.AttributeIndices().Len() { + idx := record.AttributeIndices().At(i) + attr := table.At(int(idx)) + if attr.Key() == key { + if attr.Value().Equal(value) { + // Attribute already exists, nothing to do. + return nil + } + + // Attribute exists, but the value is different, so update it. + + // If the attribute table already contains the key/value pair, just update the index. + for j := range table.Len() { + a := table.At(j) + if a.Key() == key && a.Value().Equal(value) { + if j >= math.MaxInt32 { + return fmt.Errorf("attribute %s=%#v has too high an index %d to be added to AttributeIndices", key, value, j) + } + record.AttributeIndices().SetAt(i, int32(j)) //nolint:gosec // overflow checked + return nil + } + } + + // Add the key/value pair as a new attribute to the table... + entry := table.AppendEmpty() + entry.SetKey(key) + value.CopyTo(entry.Value()) + + if table.Len() >= math.MaxInt32 { + return errors.New("AttributeTable can't take more attributes") + } + + // ...and update the index. + record.AttributeIndices().SetAt(i, int32(table.Len()-1)) //nolint:gosec // overflow checked + return nil + } + } + + // Add the key/value pair as a new attribute to the table... + entry := table.AppendEmpty() + entry.SetKey(key) + value.CopyTo(entry.Value()) + + if table.Len() >= math.MaxInt32 { + return errors.New("AttributeTable can't take more attributes") + } + + // ...and add to the index. + record.AttributeIndices().Append(int32(table.Len() - 1)) //nolint:gosec // overflow checked + return nil +} diff --git a/pdata/pprofile/attributes_test.go b/pdata/pprofile/attributes_test.go index 1524bac6ab03..95abf2004d56 100644 --- a/pdata/pprofile/attributes_test.go +++ b/pdata/pprofile/attributes_test.go @@ -74,6 +74,48 @@ func TestAddAttribute(t *testing.T) { assert.Equal(t, []int32{0}, mapp.AttributeIndices().AsRaw()) } +func TestPutAttribute(t *testing.T) { + table := NewAttributeTableSlice() + indices := NewProfile() + + // Put a first attribute. + require.NoError(t, PutAttribute(table, indices, "hello", pcommon.NewValueStr("world"))) + assert.Equal(t, 1, table.Len()) + assert.Equal(t, []int32{0}, indices.AttributeIndices().AsRaw()) + + // Put an attribute, same key, same value. + // This should be a no-op. + require.NoError(t, PutAttribute(table, indices, "hello", pcommon.NewValueStr("world"))) + assert.Equal(t, 1, table.Len()) + assert.Equal(t, []int32{0}, indices.AttributeIndices().AsRaw()) + + // Put an attribute, same key, different value. + // This updates the index and adds to the table. + fmt.Printf("test\n") + require.NoError(t, PutAttribute(table, indices, "hello", pcommon.NewValueStr("world2"))) + assert.Equal(t, 2, table.Len()) + assert.Equal(t, []int32{1}, indices.AttributeIndices().AsRaw()) + + // Put an attribute that already exists in the table. + // This updates the index and does not add to the table. + require.NoError(t, PutAttribute(table, indices, "hello", pcommon.NewValueStr("world"))) + assert.Equal(t, 2, table.Len()) + assert.Equal(t, []int32{0}, indices.AttributeIndices().AsRaw()) + + // Put a new attribute. + // This adds an index and adds to the table. + require.NoError(t, PutAttribute(table, indices, "good", pcommon.NewValueStr("day"))) + assert.Equal(t, 3, table.Len()) + assert.Equal(t, []int32{0, 2}, indices.AttributeIndices().AsRaw()) + + // Add multiple distinct attributes. + for i := range 100 { + require.NoError(t, PutAttribute(table, indices, fmt.Sprintf("key_%d", i), pcommon.NewValueStr("day"))) + assert.Equal(t, i+4, table.Len()) + assert.Equal(t, i+3, indices.AttributeIndices().Len()) + } +} + func BenchmarkFromAttributeIndices(b *testing.B) { table := NewAttributeTableSlice() @@ -162,3 +204,72 @@ func BenchmarkAddAttribute(b *testing.B) { }) } } + +func BenchmarkPutAttribute(b *testing.B) { + for _, bb := range []struct { + name string + key string + value pcommon.Value + + runBefore func(*testing.B, AttributeTableSlice, attributable) + }{ + { + name: "with a new string attribute", + key: "attribute", + value: pcommon.NewValueStr("test"), + }, + { + name: "with an existing attribute", + key: "attribute", + value: pcommon.NewValueStr("test"), + + runBefore: func(_ *testing.B, table AttributeTableSlice, _ attributable) { + entry := table.AppendEmpty() + entry.SetKey("attribute") + entry.Value().SetStr("test") + }, + }, + { + name: "with a duplicate attribute", + key: "attribute", + value: pcommon.NewValueStr("test"), + + runBefore: func(_ *testing.B, table AttributeTableSlice, obj attributable) { + require.NoError(b, PutAttribute(table, obj, "attribute", pcommon.NewValueStr("test"))) + }, + }, + { + name: "with a hundred attributes to loop through", + key: "attribute", + value: pcommon.NewValueStr("test"), + + runBefore: func(_ *testing.B, table AttributeTableSlice, _ attributable) { + for i := range 100 { + entry := table.AppendEmpty() + entry.SetKey(fmt.Sprintf("attr_%d", i)) + entry.Value().SetStr("test") + } + + entry := table.AppendEmpty() + entry.SetKey("attribute") + entry.Value().SetStr("test") + }, + }, + } { + b.Run(bb.name, func(b *testing.B) { + table := NewAttributeTableSlice() + obj := NewLocation() + + if bb.runBefore != nil { + bb.runBefore(b, table, obj) + } + + b.ResetTimer() + b.ReportAllocs() + + for n := 0; n < b.N; n++ { + _ = PutAttribute(table, obj, bb.key, bb.value) + } + }) + } +} From cbac5e837e45276a194330c45940b5e739696fbd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tim=20R=C3=BChsen?= Date: Thu, 3 Apr 2025 18:04:09 +0200 Subject: [PATCH 02/11] Simplify checks for int32 overflow --- pdata/pprofile/attributes.go | 21 ++++++--------------- 1 file changed, 6 insertions(+), 15 deletions(-) diff --git a/pdata/pprofile/attributes.go b/pdata/pprofile/attributes.go index 21f20ef1d2ea..64b2a735618a 100644 --- a/pdata/pprofile/attributes.go +++ b/pdata/pprofile/attributes.go @@ -73,6 +73,10 @@ func AddAttribute(table AttributeTableSlice, record attributable, key string, va // the same key must not appear twice in a list of attributes / attribute indices. // The record can be any struct that implements an `AttributeIndices` method. func PutAttribute(table AttributeTableSlice, record attributable, key string, value pcommon.Value) error { + if record.AttributeIndices().Len() >= math.MaxInt32-1 { + return errors.New("AttributeTable can't take more attributes") + } + for i := range record.AttributeIndices().Len() { idx := record.AttributeIndices().At(i) attr := table.At(int(idx)) @@ -82,15 +86,10 @@ func PutAttribute(table AttributeTableSlice, record attributable, key string, va return nil } - // Attribute exists, but the value is different, so update it. - // If the attribute table already contains the key/value pair, just update the index. for j := range table.Len() { a := table.At(j) if a.Key() == key && a.Value().Equal(value) { - if j >= math.MaxInt32 { - return fmt.Errorf("attribute %s=%#v has too high an index %d to be added to AttributeIndices", key, value, j) - } record.AttributeIndices().SetAt(i, int32(j)) //nolint:gosec // overflow checked return nil } @@ -101,11 +100,7 @@ func PutAttribute(table AttributeTableSlice, record attributable, key string, va entry.SetKey(key) value.CopyTo(entry.Value()) - if table.Len() >= math.MaxInt32 { - return errors.New("AttributeTable can't take more attributes") - } - - // ...and update the index. + // ...and update the existing index. record.AttributeIndices().SetAt(i, int32(table.Len()-1)) //nolint:gosec // overflow checked return nil } @@ -116,11 +111,7 @@ func PutAttribute(table AttributeTableSlice, record attributable, key string, va entry.SetKey(key) value.CopyTo(entry.Value()) - if table.Len() >= math.MaxInt32 { - return errors.New("AttributeTable can't take more attributes") - } - - // ...and add to the index. + // ...and add a new index to the indices. record.AttributeIndices().Append(int32(table.Len() - 1)) //nolint:gosec // overflow checked return nil } From 94550e88bf14731448f846c8e17a332c57213461 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tim=20R=C3=BChsen?= Date: Thu, 3 Apr 2025 18:18:18 +0200 Subject: [PATCH 03/11] Remove AddAttribute() --- .chloggen/profiles-PutAttributes.yaml | 2 +- pdata/pprofile/attributes.go | 37 ---------- pdata/pprofile/attributes_test.go | 99 --------------------------- 3 files changed, 1 insertion(+), 137 deletions(-) diff --git a/.chloggen/profiles-PutAttributes.yaml b/.chloggen/profiles-PutAttributes.yaml index 4532251f7745..820b110acda0 100644 --- a/.chloggen/profiles-PutAttributes.yaml +++ b/.chloggen/profiles-PutAttributes.yaml @@ -7,7 +7,7 @@ change_type: enhancement component: pdata/profile # A brief description of the change. Surround your text with quotes ("") if it needs to start with a backtick (`). -note: Introduce PutAttribute helper method to modify the content of attributable records. +note: Replace AddAttribute with the PutAttribute helper method to modify the content of attributable records. # One or more tracking issues or pull requests related to the change issues: [12798] diff --git a/pdata/pprofile/attributes.go b/pdata/pprofile/attributes.go index 64b2a735618a..e6e26fcd64c0 100644 --- a/pdata/pprofile/attributes.go +++ b/pdata/pprofile/attributes.go @@ -5,7 +5,6 @@ package pprofile // import "go.opentelemetry.io/collector/pdata/pprofile" import ( "errors" - "fmt" "math" "go.opentelemetry.io/collector/pdata/pcommon" @@ -31,42 +30,6 @@ func FromAttributeIndices(table AttributeTableSlice, record attributable) pcommo return m } -// AddAttribute updates an AttributeTable and a record's AttributeIndices to -// add a new attribute. -// The record can by any struct that implements an `AttributeIndices` method. -func AddAttribute(table AttributeTableSlice, record attributable, key string, value pcommon.Value) error { - for i := range table.Len() { - a := table.At(i) - - if a.Key() == key && a.Value().Equal(value) { - if i >= math.MaxInt32 { - return fmt.Errorf("Attribute %s=%#v has too high an index to be added to AttributeIndices", key, value) - } - - for j := range record.AttributeIndices().Len() { - v := record.AttributeIndices().At(j) - if v == int32(i) { //nolint:gosec // overflow checked - return nil - } - } - - record.AttributeIndices().Append(int32(i)) //nolint:gosec // overflow checked - return nil - } - } - - if table.Len() >= math.MaxInt32 { - return errors.New("AttributeTable can't take more attributes") - } - table.EnsureCapacity(table.Len() + 1) - entry := table.AppendEmpty() - entry.SetKey(key) - value.CopyTo(entry.Value()) - record.AttributeIndices().Append(int32(table.Len()) - 1) //nolint:gosec // overflow checked - - return nil -} - // PutAttribute updates an AttributeTable and a record's AttributeIndices to // add a new attribute. // The assumption is that attributes are a map as for other signals (metrics, logs, etc.), thus diff --git a/pdata/pprofile/attributes_test.go b/pdata/pprofile/attributes_test.go index 95abf2004d56..e5ae2813c040 100644 --- a/pdata/pprofile/attributes_test.go +++ b/pdata/pprofile/attributes_test.go @@ -44,36 +44,6 @@ func TestFromAttributeIndices(t *testing.T) { assert.Equal(t, attrs.AsRaw(), m) } -func TestAddAttribute(t *testing.T) { - table := NewAttributeTableSlice() - att := table.AppendEmpty() - att.SetKey("hello") - att.Value().SetStr("world") - - // Add a brand new attribute - loc := NewLocation() - err := AddAttribute(table, loc, "bonjour", pcommon.NewValueStr("monde")) - require.NoError(t, err) - - assert.Equal(t, 2, table.Len()) - assert.Equal(t, []int32{1}, loc.AttributeIndices().AsRaw()) - - // Add an already existing attribute - mapp := NewMapping() - err = AddAttribute(table, mapp, "hello", pcommon.NewValueStr("world")) - require.NoError(t, err) - - assert.Equal(t, 2, table.Len()) - assert.Equal(t, []int32{0}, mapp.AttributeIndices().AsRaw()) - - // Add a duplicate attribute - err = AddAttribute(table, mapp, "hello", pcommon.NewValueStr("world")) - require.NoError(t, err) - - assert.Equal(t, 2, table.Len()) - assert.Equal(t, []int32{0}, mapp.AttributeIndices().AsRaw()) -} - func TestPutAttribute(t *testing.T) { table := NewAttributeTableSlice() indices := NewProfile() @@ -136,75 +106,6 @@ func BenchmarkFromAttributeIndices(b *testing.B) { } } -func BenchmarkAddAttribute(b *testing.B) { - for _, bb := range []struct { - name string - key string - value pcommon.Value - - runBefore func(*testing.B, AttributeTableSlice, attributable) - }{ - { - name: "with a new string attribute", - key: "attribute", - value: pcommon.NewValueStr("test"), - }, - { - name: "with an existing attribute", - key: "attribute", - value: pcommon.NewValueStr("test"), - - runBefore: func(_ *testing.B, table AttributeTableSlice, _ attributable) { - entry := table.AppendEmpty() - entry.SetKey("attribute") - entry.Value().SetStr("test") - }, - }, - { - name: "with a duplicate attribute", - key: "attribute", - value: pcommon.NewValueStr("test"), - - runBefore: func(_ *testing.B, table AttributeTableSlice, obj attributable) { - require.NoError(b, AddAttribute(table, obj, "attribute", pcommon.NewValueStr("test"))) - }, - }, - { - name: "with a hundred attributes to loop through", - key: "attribute", - value: pcommon.NewValueStr("test"), - - runBefore: func(_ *testing.B, table AttributeTableSlice, _ attributable) { - for i := range 100 { - entry := table.AppendEmpty() - entry.SetKey(fmt.Sprintf("attr_%d", i)) - entry.Value().SetStr("test") - } - - entry := table.AppendEmpty() - entry.SetKey("attribute") - entry.Value().SetStr("test") - }, - }, - } { - b.Run(bb.name, func(b *testing.B) { - table := NewAttributeTableSlice() - obj := NewLocation() - - if bb.runBefore != nil { - bb.runBefore(b, table, obj) - } - - b.ResetTimer() - b.ReportAllocs() - - for n := 0; n < b.N; n++ { - _ = AddAttribute(table, obj, bb.key, bb.value) - } - }) - } -} - func BenchmarkPutAttribute(b *testing.B) { for _, bb := range []struct { name string From a32a23d4184f04c55f878c845137c58980ba824a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tim=20R=C3=BChsen?= Date: Mon, 7 Apr 2025 10:49:07 +0200 Subject: [PATCH 04/11] Test every exisiting record type --- pdata/pprofile/attributes_test.go | 44 +++++++++++++++++++++---------- 1 file changed, 30 insertions(+), 14 deletions(-) diff --git a/pdata/pprofile/attributes_test.go b/pdata/pprofile/attributes_test.go index e5ae2813c040..192bde3e96ab 100644 --- a/pdata/pprofile/attributes_test.go +++ b/pdata/pprofile/attributes_test.go @@ -44,45 +44,61 @@ func TestFromAttributeIndices(t *testing.T) { assert.Equal(t, attrs.AsRaw(), m) } -func TestPutAttribute(t *testing.T) { +func testPutAttribute(t *testing.T, record attributable) { table := NewAttributeTableSlice() - indices := NewProfile() // Put a first attribute. - require.NoError(t, PutAttribute(table, indices, "hello", pcommon.NewValueStr("world"))) + require.NoError(t, PutAttribute(table, record, "hello", pcommon.NewValueStr("world"))) assert.Equal(t, 1, table.Len()) - assert.Equal(t, []int32{0}, indices.AttributeIndices().AsRaw()) + assert.Equal(t, []int32{0}, record.AttributeIndices().AsRaw()) // Put an attribute, same key, same value. // This should be a no-op. - require.NoError(t, PutAttribute(table, indices, "hello", pcommon.NewValueStr("world"))) + require.NoError(t, PutAttribute(table, record, "hello", pcommon.NewValueStr("world"))) assert.Equal(t, 1, table.Len()) - assert.Equal(t, []int32{0}, indices.AttributeIndices().AsRaw()) + assert.Equal(t, []int32{0}, record.AttributeIndices().AsRaw()) // Put an attribute, same key, different value. // This updates the index and adds to the table. fmt.Printf("test\n") - require.NoError(t, PutAttribute(table, indices, "hello", pcommon.NewValueStr("world2"))) + require.NoError(t, PutAttribute(table, record, "hello", pcommon.NewValueStr("world2"))) assert.Equal(t, 2, table.Len()) - assert.Equal(t, []int32{1}, indices.AttributeIndices().AsRaw()) + assert.Equal(t, []int32{1}, record.AttributeIndices().AsRaw()) // Put an attribute that already exists in the table. // This updates the index and does not add to the table. - require.NoError(t, PutAttribute(table, indices, "hello", pcommon.NewValueStr("world"))) + require.NoError(t, PutAttribute(table, record, "hello", pcommon.NewValueStr("world"))) assert.Equal(t, 2, table.Len()) - assert.Equal(t, []int32{0}, indices.AttributeIndices().AsRaw()) + assert.Equal(t, []int32{0}, record.AttributeIndices().AsRaw()) // Put a new attribute. // This adds an index and adds to the table. - require.NoError(t, PutAttribute(table, indices, "good", pcommon.NewValueStr("day"))) + require.NoError(t, PutAttribute(table, record, "good", pcommon.NewValueStr("day"))) assert.Equal(t, 3, table.Len()) - assert.Equal(t, []int32{0, 2}, indices.AttributeIndices().AsRaw()) + assert.Equal(t, []int32{0, 2}, record.AttributeIndices().AsRaw()) // Add multiple distinct attributes. for i := range 100 { - require.NoError(t, PutAttribute(table, indices, fmt.Sprintf("key_%d", i), pcommon.NewValueStr("day"))) + require.NoError(t, PutAttribute(table, record, fmt.Sprintf("key_%d", i), pcommon.NewValueStr("day"))) assert.Equal(t, i+4, table.Len()) - assert.Equal(t, i+3, indices.AttributeIndices().Len()) + assert.Equal(t, i+3, record.AttributeIndices().Len()) + } +} + +func TestPutAttribute(t *testing.T) { + // Test every existing record type. + for _, tt := range []struct { + name string + attr attributable + }{ + {"Profile", NewProfile()}, + {"Sample", NewSample()}, + {"Mapping", NewMapping()}, + {"Location", NewLocation()}, + } { + t.Run(tt.name, func(t *testing.T) { + testPutAttribute(t, tt.attr) + }) } } From c064094a721434315524bdbebea9b0888806384d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tim=20R=C3=BChsen?= Date: Mon, 14 Apr 2025 12:05:21 +0200 Subject: [PATCH 05/11] Explicitly check for array overflows --- pdata/pprofile/attributes.go | 25 +++++++++++++++++++++---- 1 file changed, 21 insertions(+), 4 deletions(-) diff --git a/pdata/pprofile/attributes.go b/pdata/pprofile/attributes.go index e6e26fcd64c0..0665ddc93bac 100644 --- a/pdata/pprofile/attributes.go +++ b/pdata/pprofile/attributes.go @@ -5,6 +5,7 @@ package pprofile // import "go.opentelemetry.io/collector/pdata/pprofile" import ( "errors" + "fmt" "math" "go.opentelemetry.io/collector/pdata/pcommon" @@ -30,18 +31,19 @@ func FromAttributeIndices(table AttributeTableSlice, record attributable) pcommo return m } +var errTooManyTableEntries = errors.New("too many entries in AttributeTable") + // PutAttribute updates an AttributeTable and a record's AttributeIndices to // add a new attribute. // The assumption is that attributes are a map as for other signals (metrics, logs, etc.), thus // the same key must not appear twice in a list of attributes / attribute indices. // The record can be any struct that implements an `AttributeIndices` method. func PutAttribute(table AttributeTableSlice, record attributable, key string, value pcommon.Value) error { - if record.AttributeIndices().Len() >= math.MaxInt32-1 { - return errors.New("AttributeTable can't take more attributes") - } - for i := range record.AttributeIndices().Len() { idx := record.AttributeIndices().At(i) + if idx < 0 || idx >= int32(table.Len()) { + return fmt.Errorf("index value %d out of range in AttributeIndices[%d]", idx, i) + } attr := table.At(int(idx)) if attr.Key() == key { if attr.Value().Equal(value) { @@ -53,11 +55,18 @@ func PutAttribute(table AttributeTableSlice, record attributable, key string, va for j := range table.Len() { a := table.At(j) if a.Key() == key && a.Value().Equal(value) { + if j > math.MaxInt32 { + return errTooManyTableEntries + } record.AttributeIndices().SetAt(i, int32(j)) //nolint:gosec // overflow checked return nil } } + if table.Len() >= math.MaxInt32 { + return errTooManyTableEntries + } + // Add the key/value pair as a new attribute to the table... entry := table.AppendEmpty() entry.SetKey(key) @@ -69,6 +78,14 @@ func PutAttribute(table AttributeTableSlice, record attributable, key string, va } } + if table.Len() >= math.MaxInt32 { + return errTooManyTableEntries + } + + if record.AttributeIndices().Len() >= math.MaxInt32 { + return errors.New("too many entries in AttributeIndices") + } + // Add the key/value pair as a new attribute to the table... entry := table.AppendEmpty() entry.SetKey(key) From 93be1c2d1c6bf211efe481d4b4f14034fdc25439 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tim=20R=C3=BChsen?= Date: Mon, 14 Apr 2025 12:21:39 +0200 Subject: [PATCH 06/11] Add test case to check negative index detection --- pdata/pprofile/attributes_test.go | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/pdata/pprofile/attributes_test.go b/pdata/pprofile/attributes_test.go index 192bde3e96ab..0fe0be6490c6 100644 --- a/pdata/pprofile/attributes_test.go +++ b/pdata/pprofile/attributes_test.go @@ -83,6 +83,15 @@ func testPutAttribute(t *testing.T, record attributable) { assert.Equal(t, i+4, table.Len()) assert.Equal(t, i+3, record.AttributeIndices().Len()) } + + // Add a negative index to the record. + record.AttributeIndices().Append(-1) + // Try putting a new attribute, make sure it fails, and that table/indices didn't change. + tableLen := table.Len() + indicesLen := record.AttributeIndices().Len() + require.Error(t, PutAttribute(table, record, "newKey", pcommon.NewValueStr("value"))) + require.Equal(t, tableLen, table.Len()) + require.Equal(t, indicesLen, record.AttributeIndices().Len()) } func TestPutAttribute(t *testing.T) { From 2ff423fc1999844ba7d6cee8fa4dcfee36899424 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tim=20R=C3=BChsen?= Date: Mon, 14 Apr 2025 12:26:28 +0200 Subject: [PATCH 07/11] Add test case to check out-of-range index detection --- pdata/pprofile/attributes_test.go | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/pdata/pprofile/attributes_test.go b/pdata/pprofile/attributes_test.go index 0fe0be6490c6..a49faa4cc992 100644 --- a/pdata/pprofile/attributes_test.go +++ b/pdata/pprofile/attributes_test.go @@ -86,9 +86,16 @@ func testPutAttribute(t *testing.T, record attributable) { // Add a negative index to the record. record.AttributeIndices().Append(-1) - // Try putting a new attribute, make sure it fails, and that table/indices didn't change. tableLen := table.Len() indicesLen := record.AttributeIndices().Len() + // Try putting a new attribute, make sure it fails, and that table/indices didn't change. + require.Error(t, PutAttribute(table, record, "newKey", pcommon.NewValueStr("value"))) + require.Equal(t, tableLen, table.Len()) + require.Equal(t, indicesLen, record.AttributeIndices().Len()) + + // Set the last index to the table length, which is out of range. + record.AttributeIndices().SetAt(indicesLen-1, int32(tableLen)) + // Try putting a new attribute, make sure it fails, and that table/indices didn't change. require.Error(t, PutAttribute(table, record, "newKey", pcommon.NewValueStr("value"))) require.Equal(t, tableLen, table.Len()) require.Equal(t, indicesLen, record.AttributeIndices().Len()) From 4359f6519419e6428337683d717ac80e9d625ed6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tim=20R=C3=BChsen?= Date: Mon, 14 Apr 2025 15:48:52 +0200 Subject: [PATCH 08/11] Appease gosec --- pdata/pprofile/attributes.go | 6 +++--- pdata/pprofile/attributes_test.go | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/pdata/pprofile/attributes.go b/pdata/pprofile/attributes.go index 0665ddc93bac..b4dd54e24bc3 100644 --- a/pdata/pprofile/attributes.go +++ b/pdata/pprofile/attributes.go @@ -40,11 +40,11 @@ var errTooManyTableEntries = errors.New("too many entries in AttributeTable") // The record can be any struct that implements an `AttributeIndices` method. func PutAttribute(table AttributeTableSlice, record attributable, key string, value pcommon.Value) error { for i := range record.AttributeIndices().Len() { - idx := record.AttributeIndices().At(i) - if idx < 0 || idx >= int32(table.Len()) { + idx := int(record.AttributeIndices().At(i)) + if idx < 0 || idx >= table.Len() { return fmt.Errorf("index value %d out of range in AttributeIndices[%d]", idx, i) } - attr := table.At(int(idx)) + attr := table.At(idx) if attr.Key() == key { if attr.Value().Equal(value) { // Attribute already exists, nothing to do. diff --git a/pdata/pprofile/attributes_test.go b/pdata/pprofile/attributes_test.go index a49faa4cc992..490377b582f5 100644 --- a/pdata/pprofile/attributes_test.go +++ b/pdata/pprofile/attributes_test.go @@ -94,7 +94,7 @@ func testPutAttribute(t *testing.T, record attributable) { require.Equal(t, indicesLen, record.AttributeIndices().Len()) // Set the last index to the table length, which is out of range. - record.AttributeIndices().SetAt(indicesLen-1, int32(tableLen)) + record.AttributeIndices().SetAt(indicesLen-1, int32(tableLen)) //nolint:gosec // Try putting a new attribute, make sure it fails, and that table/indices didn't change. require.Error(t, PutAttribute(table, record, "newKey", pcommon.NewValueStr("value"))) require.Equal(t, tableLen, table.Len()) From 2ae3be672f984ee28bca3dc64f065eae1fbdb42b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tim=20R=C3=BChsen?= Date: Thu, 24 Apr 2025 19:08:03 +0200 Subject: [PATCH 09/11] Avoid duplicates in the attributes table --- pdata/pprofile/attributes.go | 20 ++++++++++++++++---- pdata/pprofile/attributes_test.go | 7 +++++++ 2 files changed, 23 insertions(+), 4 deletions(-) diff --git a/pdata/pprofile/attributes.go b/pdata/pprofile/attributes.go index b4dd54e24bc3..eea1283c7f1f 100644 --- a/pdata/pprofile/attributes.go +++ b/pdata/pprofile/attributes.go @@ -78,14 +78,26 @@ func PutAttribute(table AttributeTableSlice, record attributable, key string, va } } - if table.Len() >= math.MaxInt32 { - return errTooManyTableEntries - } - if record.AttributeIndices().Len() >= math.MaxInt32 { return errors.New("too many entries in AttributeIndices") } + for j := range table.Len() { + a := table.At(j) + if a.Key() == key && a.Value().Equal(value) { + if j > math.MaxInt32 { + return errTooManyTableEntries + } + // Add the index of the existing attribute to the indices. + record.AttributeIndices().Append(int32(j)) //nolint:gosec // overflow checked + return nil + } + } + + if table.Len() >= math.MaxInt32 { + return errTooManyTableEntries + } + // Add the key/value pair as a new attribute to the table... entry := table.AppendEmpty() entry.SetKey(key) diff --git a/pdata/pprofile/attributes_test.go b/pdata/pprofile/attributes_test.go index 490377b582f5..c3ac629267be 100644 --- a/pdata/pprofile/attributes_test.go +++ b/pdata/pprofile/attributes_test.go @@ -58,6 +58,13 @@ func testPutAttribute(t *testing.T, record attributable) { assert.Equal(t, 1, table.Len()) assert.Equal(t, []int32{0}, record.AttributeIndices().AsRaw()) + // Special case: removing and adding again should not change the table as + // this can lead to multiple identical attributes in the table. + record.AttributeIndices().FromRaw([]int32{}) + require.NoError(t, PutAttribute(table, record, "hello", pcommon.NewValueStr("world"))) + assert.Equal(t, 1, table.Len()) + assert.Equal(t, []int32{0}, record.AttributeIndices().AsRaw()) + // Put an attribute, same key, different value. // This updates the index and adds to the table. fmt.Printf("test\n") From af7929eb51d57fb4159ac05dbe641f009cb64765 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tim=20R=C3=BChsen?= Date: Wed, 30 Apr 2025 08:29:33 +0200 Subject: [PATCH 10/11] Deprecate AddAttribute --- .chloggen/profiles-PutAttributes.yaml | 2 +- pdata/pprofile/attributes.go | 41 +++++++++++++++++++++++++-- pdata/pprofile/attributes_test.go | 30 ++++++++++++++++++++ 3 files changed, 70 insertions(+), 3 deletions(-) diff --git a/.chloggen/profiles-PutAttributes.yaml b/.chloggen/profiles-PutAttributes.yaml index 820b110acda0..ccdedbbffd93 100644 --- a/.chloggen/profiles-PutAttributes.yaml +++ b/.chloggen/profiles-PutAttributes.yaml @@ -1,7 +1,7 @@ # Use this changelog template to create an entry for release notes. # One of 'breaking', 'deprecation', 'new_component', 'enhancement', 'bug_fix' -change_type: enhancement +change_type: deprecation # The name of the component, or a single word describing the area of concern, (e.g. otlpreceiver) component: pdata/profile diff --git a/pdata/pprofile/attributes.go b/pdata/pprofile/attributes.go index eea1283c7f1f..1569b178aca7 100644 --- a/pdata/pprofile/attributes.go +++ b/pdata/pprofile/attributes.go @@ -17,7 +17,7 @@ type attributable interface { // FromAttributeIndices builds a [pcommon.Map] containing the attributes of a // record. -// The record can by any struct that implements an `AttributeIndices` method. +// The record can be any struct that implements an `AttributeIndices` method. // Updates made to the return map will not be applied back to the record. func FromAttributeIndices(table AttributeTableSlice, record attributable) pcommon.Map { m := pcommon.NewMap() @@ -31,10 +31,47 @@ func FromAttributeIndices(table AttributeTableSlice, record attributable) pcommo return m } +// AddAttribute updates an AttributeTable and a record's AttributeIndices to +// add a new attribute. +// The record can be any struct that implements an `AttributeIndices` method. +// Deprecated: [v0.126.0] Use PutAttribute instead. +func AddAttribute(table AttributeTableSlice, record attributable, key string, value pcommon.Value) error { + for i := range table.Len() { + a := table.At(i) + + if a.Key() == key && a.Value().Equal(value) { + if i >= math.MaxInt32 { + return fmt.Errorf("Attribute %s=%#v has too high an index to be added to AttributeIndices", key, value) + } + + for j := range record.AttributeIndices().Len() { + v := record.AttributeIndices().At(j) + if v == int32(i) { //nolint:gosec // overflow checked + return nil + } + } + + record.AttributeIndices().Append(int32(i)) //nolint:gosec // overflow checked + return nil + } + } + + if table.Len() >= math.MaxInt32 { + return errors.New("AttributeTable can't take more attributes") + } + table.EnsureCapacity(table.Len() + 1) + entry := table.AppendEmpty() + entry.SetKey(key) + value.CopyTo(entry.Value()) + record.AttributeIndices().Append(int32(table.Len()) - 1) //nolint:gosec // overflow checked + + return nil +} + var errTooManyTableEntries = errors.New("too many entries in AttributeTable") // PutAttribute updates an AttributeTable and a record's AttributeIndices to -// add a new attribute. +// add or update an attribute. // The assumption is that attributes are a map as for other signals (metrics, logs, etc.), thus // the same key must not appear twice in a list of attributes / attribute indices. // The record can be any struct that implements an `AttributeIndices` method. diff --git a/pdata/pprofile/attributes_test.go b/pdata/pprofile/attributes_test.go index c3ac629267be..fed4ff8d4d37 100644 --- a/pdata/pprofile/attributes_test.go +++ b/pdata/pprofile/attributes_test.go @@ -13,6 +13,36 @@ import ( "go.opentelemetry.io/collector/pdata/pcommon" ) +func TestAddAttribute(t *testing.T) { + table := NewAttributeTableSlice() + att := table.AppendEmpty() + att.SetKey("hello") + att.Value().SetStr("world") + + // Add a brand new attribute + loc := NewLocation() + err := AddAttribute(table, loc, "bonjour", pcommon.NewValueStr("monde")) + require.NoError(t, err) + + assert.Equal(t, 2, table.Len()) + assert.Equal(t, []int32{1}, loc.AttributeIndices().AsRaw()) + + // Add an already existing attribute + mapp := NewMapping() + err = AddAttribute(table, mapp, "hello", pcommon.NewValueStr("world")) + require.NoError(t, err) + + assert.Equal(t, 2, table.Len()) + assert.Equal(t, []int32{0}, mapp.AttributeIndices().AsRaw()) + + // Add a duplicate attribute + err = AddAttribute(table, mapp, "hello", pcommon.NewValueStr("world")) + require.NoError(t, err) + + assert.Equal(t, 2, table.Len()) + assert.Equal(t, []int32{0}, mapp.AttributeIndices().AsRaw()) +} + func TestFromAttributeIndices(t *testing.T) { table := NewAttributeTableSlice() att := table.AppendEmpty() From 32306811319cd1f80a3eaa294571d3010b1e8511 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tim=20R=C3=BChsen?= Date: Wed, 30 Apr 2025 08:55:03 +0200 Subject: [PATCH 11/11] Drop AddAttribute function comments --- pdata/pprofile/attributes.go | 3 --- 1 file changed, 3 deletions(-) diff --git a/pdata/pprofile/attributes.go b/pdata/pprofile/attributes.go index 1569b178aca7..232d7e4b68e2 100644 --- a/pdata/pprofile/attributes.go +++ b/pdata/pprofile/attributes.go @@ -31,9 +31,6 @@ func FromAttributeIndices(table AttributeTableSlice, record attributable) pcommo return m } -// AddAttribute updates an AttributeTable and a record's AttributeIndices to -// add a new attribute. -// The record can be any struct that implements an `AttributeIndices` method. // Deprecated: [v0.126.0] Use PutAttribute instead. func AddAttribute(table AttributeTableSlice, record attributable, key string, value pcommon.Value) error { for i := range table.Len() {