From 5d7dcb5a71d86edb5369a6577a6dcc1d507930ca Mon Sep 17 00:00:00 2001 From: dmathieu <42@dmathieu.com> Date: Mon, 13 Oct 2025 15:22:13 +0200 Subject: [PATCH 1/4] introduce SetStack helper method --- pdata/pprofile/stacks.go | 47 +++++++++++++ pdata/pprofile/stacks_test.go | 122 ++++++++++++++++++++++++++++++++++ 2 files changed, 169 insertions(+) create mode 100644 pdata/pprofile/stacks.go create mode 100644 pdata/pprofile/stacks_test.go diff --git a/pdata/pprofile/stacks.go b/pdata/pprofile/stacks.go new file mode 100644 index 00000000000..ba2d51a7845 --- /dev/null +++ b/pdata/pprofile/stacks.go @@ -0,0 +1,47 @@ +// Copyright The OpenTelemetry Authors +// SPDX-License-Identifier: Apache-2.0 + +package pprofile // import "go.opentelemetry.io/collector/pdata/pprofile" + +import ( + "errors" + "fmt" + "math" +) + +var errTooManyStackTableEntries = errors.New("too many entries in StackTable") + +// SetStack updates a StackTable and a Sample's StackIndex to +// set its stack. +func SetStack(table StackSlice, record Sample, st Stack) error { + idx := int(record.StackIndex()) + if idx > 0 { + if idx >= table.Len() { + return fmt.Errorf("index value %d out of range for StackIndex", idx) + } + mapAt := table.At(idx) + if mapAt.Equal(st) { + // Stack already exists, nothing to do. + return nil + } + } + + for j, l := range table.All() { + if l.Equal(st) { + if j > math.MaxInt32 { + return errTooManyStackTableEntries + } + // Add the index of the existing stack to the indices. + record.SetStackIndex(int32(j)) //nolint:gosec // G115 overflow checked + return nil + } + } + + if table.Len() >= math.MaxInt32 { + return errTooManyStackTableEntries + } + + st.CopyTo(table.AppendEmpty()) + record.SetStackIndex(int32(table.Len() - 1)) //nolint:gosec // G115 overflow checked + return nil +} diff --git a/pdata/pprofile/stacks_test.go b/pdata/pprofile/stacks_test.go new file mode 100644 index 00000000000..32a8933c4ff --- /dev/null +++ b/pdata/pprofile/stacks_test.go @@ -0,0 +1,122 @@ +// Copyright The OpenTelemetry Authors +// SPDX-License-Identifier: Apache-2.0 + +package pprofile + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestSetStack(t *testing.T) { + table := NewStackSlice() + s := NewStack() + s.LocationIndices().Append(1) + s2 := NewStack() + s.LocationIndices().Append(2) + smpl := NewSample() + + // Put a first stack + require.NoError(t, SetStack(table, smpl, s)) + assert.Equal(t, 1, table.Len()) + assert.Equal(t, int32(0), smpl.StackIndex()) + + // Put the same stack + // This should be a no-op. + require.NoError(t, SetStack(table, smpl, s)) + assert.Equal(t, 1, table.Len()) + assert.Equal(t, int32(0), smpl.StackIndex()) + + // Set a new stack + // This sets the index and adds to the table. + require.NoError(t, SetStack(table, smpl, s2)) + assert.Equal(t, 2, table.Len()) + assert.Equal(t, int32(table.Len()-1), smpl.StackIndex()) //nolint:gosec // G115 + + // Set an existing stack + require.NoError(t, SetStack(table, smpl, s)) + assert.Equal(t, 2, table.Len()) + assert.Equal(t, int32(0), smpl.StackIndex()) + // Set another existing stack + require.NoError(t, SetStack(table, smpl, s2)) + assert.Equal(t, 2, table.Len()) + assert.Equal(t, int32(table.Len()-1), smpl.StackIndex()) //nolint:gosec // G115 +} + +func TestSetStackCurrentTooHigh(t *testing.T) { + table := NewStackSlice() + smpl := NewSample() + smpl.SetStackIndex(42) + + err := SetStack(table, smpl, NewStack()) + require.Error(t, err) + assert.Equal(t, 0, table.Len()) + assert.Equal(t, int32(42), smpl.StackIndex()) +} + +func BenchmarkSetStack(b *testing.B) { + for _, bb := range []struct { + name string + stack Stack + + runBefore func(*testing.B, StackSlice, Sample) + }{ + { + name: "with a new stack", + stack: NewStack(), + }, + { + name: "with an existing stack", + stack: func() Stack { + s := NewStack() + s.LocationIndices().Append(1) + return s + }(), + + runBefore: func(_ *testing.B, table StackSlice, _ Sample) { + s := table.AppendEmpty() + s.LocationIndices().Append(1) + }, + }, + { + name: "with a duplicate stack", + stack: NewStack(), + + runBefore: func(_ *testing.B, table StackSlice, obj Sample) { + require.NoError(b, SetStack(table, obj, NewStack())) + }, + }, + { + name: "with a hundred stacks to loop through", + stack: func() Stack { + s := NewStack() + s.LocationIndices().Append(1) + return s + }(), + + runBefore: func(_ *testing.B, table StackSlice, _ Sample) { + for range 100 { + table.AppendEmpty() + } + }, + }, + } { + b.Run(bb.name, func(b *testing.B) { + table := NewStackSlice() + obj := NewSample() + + if bb.runBefore != nil { + bb.runBefore(b, table, obj) + } + + b.ResetTimer() + b.ReportAllocs() + + for b.Loop() { + _ = SetStack(table, obj, bb.stack) + } + }) + } +} From 30bc9d4f3a8b604f3812bd7cae712657269d5ec6 Mon Sep 17 00:00:00 2001 From: dmathieu <42@dmathieu.com> Date: Mon, 13 Oct 2025 15:24:29 +0200 Subject: [PATCH 2/4] add changelog entry --- .chloggen/pprofile-set-stack.yaml | 25 +++++++++++++++++++++++++ 1 file changed, 25 insertions(+) create mode 100644 .chloggen/pprofile-set-stack.yaml diff --git a/.chloggen/pprofile-set-stack.yaml b/.chloggen/pprofile-set-stack.yaml new file mode 100644 index 00000000000..cf7acb8ec58 --- /dev/null +++ b/.chloggen/pprofile-set-stack.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/pprofile + +# A brief description of the change. Surround your text with quotes ("") if it needs to start with a backtick (`). +note: Introduce `SetStack` method + +# One or more tracking issues or pull requests related to the change +issues: [14007] + +# (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] From 3c987cd99864e8d08ee3004b6be508a70e8342cd Mon Sep 17 00:00:00 2001 From: dmathieu <42@dmathieu.com> Date: Tue, 14 Oct 2025 09:34:52 +0200 Subject: [PATCH 3/4] make setstack return the idx rather than modifying the sample --- pdata/pprofile/stacks.go | 30 +++++--------------- pdata/pprofile/stacks_test.go | 53 +++++++++++++++-------------------- 2 files changed, 30 insertions(+), 53 deletions(-) diff --git a/pdata/pprofile/stacks.go b/pdata/pprofile/stacks.go index ba2d51a7845..361d66e0e4f 100644 --- a/pdata/pprofile/stacks.go +++ b/pdata/pprofile/stacks.go @@ -5,43 +5,27 @@ package pprofile // import "go.opentelemetry.io/collector/pdata/pprofile" import ( "errors" - "fmt" "math" ) var errTooManyStackTableEntries = errors.New("too many entries in StackTable") -// SetStack updates a StackTable and a Sample's StackIndex to -// set its stack. -func SetStack(table StackSlice, record Sample, st Stack) error { - idx := int(record.StackIndex()) - if idx > 0 { - if idx >= table.Len() { - return fmt.Errorf("index value %d out of range for StackIndex", idx) - } - mapAt := table.At(idx) - if mapAt.Equal(st) { - // Stack already exists, nothing to do. - return nil - } - } - +// SetStack updates a StackTable, adding or providing a stack and returns its +// index. +func SetStack(table StackSlice, st Stack) (int32, error) { for j, l := range table.All() { if l.Equal(st) { if j > math.MaxInt32 { - return errTooManyStackTableEntries + return 0, errTooManyStackTableEntries } - // Add the index of the existing stack to the indices. - record.SetStackIndex(int32(j)) //nolint:gosec // G115 overflow checked - return nil + return int32(j), nil //nolint:gosec // G115 overflow checked } } if table.Len() >= math.MaxInt32 { - return errTooManyStackTableEntries + return 0, errTooManyStackTableEntries } st.CopyTo(table.AppendEmpty()) - record.SetStackIndex(int32(table.Len() - 1)) //nolint:gosec // G115 overflow checked - return nil + return int32(table.Len() - 1), nil //nolint:gosec // G115 overflow checked } diff --git a/pdata/pprofile/stacks_test.go b/pdata/pprofile/stacks_test.go index 32a8933c4ff..abe65335b9a 100644 --- a/pdata/pprofile/stacks_test.go +++ b/pdata/pprofile/stacks_test.go @@ -16,44 +16,37 @@ func TestSetStack(t *testing.T) { s.LocationIndices().Append(1) s2 := NewStack() s.LocationIndices().Append(2) - smpl := NewSample() // Put a first stack - require.NoError(t, SetStack(table, smpl, s)) + idx, err := SetStack(table, s) + require.NoError(t, err) assert.Equal(t, 1, table.Len()) - assert.Equal(t, int32(0), smpl.StackIndex()) + assert.Equal(t, int32(0), idx) // Put the same stack // This should be a no-op. - require.NoError(t, SetStack(table, smpl, s)) + idx, err = SetStack(table, s) + require.NoError(t, err) assert.Equal(t, 1, table.Len()) - assert.Equal(t, int32(0), smpl.StackIndex()) + assert.Equal(t, int32(0), idx) // Set a new stack // This sets the index and adds to the table. - require.NoError(t, SetStack(table, smpl, s2)) + idx, err = SetStack(table, s2) + require.NoError(t, err) assert.Equal(t, 2, table.Len()) - assert.Equal(t, int32(table.Len()-1), smpl.StackIndex()) //nolint:gosec // G115 + assert.Equal(t, int32(table.Len()-1), idx) //nolint:gosec // G115 // Set an existing stack - require.NoError(t, SetStack(table, smpl, s)) + idx, err = SetStack(table, s) + require.NoError(t, err) assert.Equal(t, 2, table.Len()) - assert.Equal(t, int32(0), smpl.StackIndex()) + assert.Equal(t, int32(0), idx) // Set another existing stack - require.NoError(t, SetStack(table, smpl, s2)) + idx, err = SetStack(table, s2) + require.NoError(t, err) assert.Equal(t, 2, table.Len()) - assert.Equal(t, int32(table.Len()-1), smpl.StackIndex()) //nolint:gosec // G115 -} - -func TestSetStackCurrentTooHigh(t *testing.T) { - table := NewStackSlice() - smpl := NewSample() - smpl.SetStackIndex(42) - - err := SetStack(table, smpl, NewStack()) - require.Error(t, err) - assert.Equal(t, 0, table.Len()) - assert.Equal(t, int32(42), smpl.StackIndex()) + assert.Equal(t, int32(table.Len()-1), idx) //nolint:gosec // G115 } func BenchmarkSetStack(b *testing.B) { @@ -61,7 +54,7 @@ func BenchmarkSetStack(b *testing.B) { name string stack Stack - runBefore func(*testing.B, StackSlice, Sample) + runBefore func(*testing.B, StackSlice) }{ { name: "with a new stack", @@ -75,7 +68,7 @@ func BenchmarkSetStack(b *testing.B) { return s }(), - runBefore: func(_ *testing.B, table StackSlice, _ Sample) { + runBefore: func(_ *testing.B, table StackSlice) { s := table.AppendEmpty() s.LocationIndices().Append(1) }, @@ -84,8 +77,9 @@ func BenchmarkSetStack(b *testing.B) { name: "with a duplicate stack", stack: NewStack(), - runBefore: func(_ *testing.B, table StackSlice, obj Sample) { - require.NoError(b, SetStack(table, obj, NewStack())) + runBefore: func(_ *testing.B, table StackSlice) { + _, err := SetStack(table, NewStack()) + require.NoError(b, err) }, }, { @@ -96,7 +90,7 @@ func BenchmarkSetStack(b *testing.B) { return s }(), - runBefore: func(_ *testing.B, table StackSlice, _ Sample) { + runBefore: func(_ *testing.B, table StackSlice) { for range 100 { table.AppendEmpty() } @@ -105,17 +99,16 @@ func BenchmarkSetStack(b *testing.B) { } { b.Run(bb.name, func(b *testing.B) { table := NewStackSlice() - obj := NewSample() if bb.runBefore != nil { - bb.runBefore(b, table, obj) + bb.runBefore(b, table) } b.ResetTimer() b.ReportAllocs() for b.Loop() { - _ = SetStack(table, obj, bb.stack) + _, _ = SetStack(table, bb.stack) } }) } From e06726b1ee56c1039962059cca9d040e1d05a3dc Mon Sep 17 00:00:00 2001 From: Damien Mathieu <42@dmathieu.com> Date: Tue, 21 Oct 2025 19:23:29 +0200 Subject: [PATCH 4/4] Update pdata/pprofile/stacks.go Co-authored-by: Alex Boten <223565+codeboten@users.noreply.github.com> --- pdata/pprofile/stacks.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pdata/pprofile/stacks.go b/pdata/pprofile/stacks.go index 361d66e0e4f..762429d7994 100644 --- a/pdata/pprofile/stacks.go +++ b/pdata/pprofile/stacks.go @@ -10,7 +10,7 @@ import ( var errTooManyStackTableEntries = errors.New("too many entries in StackTable") -// SetStack updates a StackTable, adding or providing a stack and returns its +// SetStack updates a StackSlice, adding or providing a stack and returns its // index. func SetStack(table StackSlice, st Stack) (int32, error) { for j, l := range table.All() {