Skip to content

Commit

Permalink
fix: pprof grouping for samples with span_id (#3450)
Browse files Browse the repository at this point in the history
The following code:

```go
pprof.Do(context.Background(), pprof.Labels("function", "slow", "qwe", "asd", "asdasd", "zxczxc"), func(c context.Context) {
   work(40000)
   pprof.Do(c, pprof.Labels("span_id", "239"), func(c context.Context) {
     work(40000)
   })
})
```

may produce sampels with the folowing labels

```
(1,2) (3,4) (5,6)
(1,2) (3,4) (5,6) (7, 8)
```

After applying `LabelsWithout` they will look like:

```
(5,6) (3,4) (1,2) 
(1,2) (5,6) (3,4) | (7, 8)  // | denotes slice length and to the right is the filtered label
```


Next `CompareSampleLabels` will fail to recognize that these two label sets are equals
because first one is in reverse sorted order and the other is not.

In this PR we change `FilterLabelsInPlace` function to maintain sorted order of non filtered labels,
maintain reverse sorted order of filtered labels and put the filtered labels on the right side in place
so that `slices.Reverse` is not needed.
  • Loading branch information
korniltsev authored Jul 31, 2024
1 parent 02f2ded commit 96cad4a
Show file tree
Hide file tree
Showing 4 changed files with 72 additions and 10 deletions.
3 changes: 2 additions & 1 deletion pkg/pprof/pprof.go
Original file line number Diff line number Diff line change
Expand Up @@ -662,6 +662,7 @@ func GroupSamplesWithoutLabelsByKey(p *profilev1.Profile, keys []int64) []Sample
// We hide labels matching the keys to the end
// of the slice, after len() boundary.
s.Label = LabelsWithout(s.Label, keys)
sort.Sort(LabelsByKeyValue(s.Label)) // TODO: Find a way to avoid this.
}
// Sorting and grouping accounts only for labels kept.
sort.Sort(SamplesByLabels(p.Sample))
Expand All @@ -677,7 +678,7 @@ func GroupSamplesWithoutLabelsByKey(p *profilev1.Profile, keys []int64) []Sample
func restoreRemovedLabels(labels []*profilev1.Label) []*profilev1.Label {
labels = labels[len(labels):cap(labels)]
for i, l := range labels {
if l == nil {
if l == nil { // labels had extra capacity in sample labels
labels = labels[:i]
break
}
Expand Down
78 changes: 69 additions & 9 deletions pkg/pprof/pprof_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -931,16 +931,16 @@ func Test_GroupSamplesWithout(t *testing.T) {
},
},
{
Labels: []*profilev1.Label{{Key: 1, Str: 3}},
Labels: []*profilev1.Label{{Key: 1, Str: 1}, {Key: 2, Str: 2}},
Samples: []*profilev1.Sample{
{Label: []*profilev1.Label{}},
{Label: []*profilev1.Label{}},
},
},
{
Labels: []*profilev1.Label{{Key: 2, Str: 2}, {Key: 1, Str: 1}},
Labels: []*profilev1.Label{{Key: 1, Str: 3}},
Samples: []*profilev1.Sample{
{Label: []*profilev1.Label{}},
{Label: []*profilev1.Label{}},
},
},
},
Expand Down Expand Up @@ -971,14 +971,14 @@ func Test_GroupSamplesWithout(t *testing.T) {
},
},
{
Labels: []*profilev1.Label{{Key: 3, Str: 3}, {Key: 1, Str: 1}},
Labels: []*profilev1.Label{{Key: 1, Str: 1}, {Key: 3, Str: 3}},
Samples: []*profilev1.Sample{
{Label: []*profilev1.Label{{Key: 2, Str: 100}}},
{Label: []*profilev1.Label{{Key: 2, Str: 101}}},
},
},
{
Labels: []*profilev1.Label{{Key: 3, Str: 4}, {Key: 1, Str: 1}},
Labels: []*profilev1.Label{{Key: 1, Str: 1}, {Key: 3, Str: 4}},
Samples: []*profilev1.Sample{
{Label: []*profilev1.Label{{Key: 2, Str: 102}}},
},
Expand Down Expand Up @@ -1009,7 +1009,7 @@ func Test_GroupSamplesWithout(t *testing.T) {
},
},
{
Labels: []*profilev1.Label{{Key: 3, Str: 3}, {Key: 2, Str: 2}, {Key: 1, Str: 1}},
Labels: []*profilev1.Label{{Key: 1, Str: 1}, {Key: 2, Str: 2}, {Key: 3, Str: 3}},
Samples: []*profilev1.Sample{
{Label: []*profilev1.Label{}},
},
Expand Down Expand Up @@ -1048,12 +1048,57 @@ func Test_GroupSamplesWithout(t *testing.T) {
},
},
},
{
description: "without single existent, single group",
input: &profilev1.Profile{
Sample: []*profilev1.Sample{
{Label: []*profilev1.Label{{Key: 1, Str: 1}, {Key: 2, Str: 100}, {Key: 3, Str: 3}}},
{Label: []*profilev1.Label{{Key: 1, Str: 1}, {Key: 3, Str: 3}}},
},
},
without: []int64{2},
expected: []SampleGroup{
{
Labels: []*profilev1.Label{{Key: 1, Str: 1}, {Key: 3, Str: 3}},
Samples: []*profilev1.Sample{
{Label: []*profilev1.Label{{Key: 2, Str: 100}}},
{Label: []*profilev1.Label{}},
},
},
},
},
{
description: "Testcase for extra labels capacity (restoreRemovedLabels nil check)",
input: &profilev1.Profile{
Sample: []*profilev1.Sample{
{Label: []*profilev1.Label{{Key: 1, Str: 1}, {Key: 2, Str: 100}, {Key: 3, Str: 3}, nil, nil}[:3]},
{Label: []*profilev1.Label{{Key: 1, Str: 1}, {Key: 3, Str: 3}}},
}[:2],
},
without: []int64{2},
expected: []SampleGroup{
{
Labels: []*profilev1.Label{{Key: 1, Str: 1}, {Key: 3, Str: 3}},
Samples: []*profilev1.Sample{
{Label: []*profilev1.Label{{Key: 2, Str: 100}}},
{Label: []*profilev1.Label{}},
},
},
},
},
}

for _, tc := range testCases {
tc := tc
t.Run(tc.description, func(t *testing.T) {
require.Equal(t, tc.expected, GroupSamplesWithoutLabelsByKey(tc.input, tc.without))
for _, g := range tc.expected {
for _, sample := range g.Samples {
for _, label := range sample.Label {
assert.NotNil(t, label)
}
}
}
})
}
}
Expand Down Expand Up @@ -1325,10 +1370,10 @@ func Test_GroupSamplesWithout_Go_CPU_profile(t *testing.T) {
assert.Equal(t, groups[0].Labels, []*profilev1.Label{{Key: 18, Str: 19}})
assert.Equal(t, len(groups[0].Samples), 5)

assert.Equal(t, groups[1].Labels, []*profilev1.Label{{Key: 22, Str: 23}, {Key: 18, Str: 19}})
assert.Equal(t, groups[1].Labels, []*profilev1.Label{{Key: 18, Str: 19}, {Key: 22, Str: 23}})
assert.Equal(t, len(groups[1].Samples), 325)

assert.Equal(t, groups[2].Labels, []*profilev1.Label{{Key: 22, Str: 27}, {Key: 18, Str: 19}})
assert.Equal(t, groups[2].Labels, []*profilev1.Label{{Key: 18, Str: 19}, {Key: 22, Str: 27}})
assert.Equal(t, len(groups[2].Samples), 150)
}

Expand All @@ -1338,7 +1383,22 @@ func Test_GroupSamplesWithout_dotnet_profile(t *testing.T) {

groups := GroupSamplesWithoutLabels(p.Profile, ProfileIDLabelName)
require.Len(t, groups, 1)
assert.Equal(t, groups[0].Labels, []*profilev1.Label{{Key: 66, Str: 67}, {Key: 64, Str: 65}})
assert.Equal(t, groups[0].Labels, []*profilev1.Label{{Key: 64, Str: 65}, {Key: 66, Str: 67}})
}

func Test_GroupSamplesWithout_single_group_with_optional_span_id(t *testing.T) {
// pprof.Do(context.Background(), pprof.Labels("function", "slow", "qwe", "asd", "asdasd", "zxczxc"), func(c context.Context) {
// work(40000)
// pprof.Do(c, pprof.Labels("span_id", "239"), func(c context.Context) {
// work(40000)
// })
// })
p, err := OpenFile("testdata/single_group_with_optional_span_id.pb.gz")
require.NoError(t, err)

groups := GroupSamplesWithoutLabels(p.Profile, SpanIDLabelName)
require.Len(t, groups, 1)
assert.Equal(t, groups[0].Labels, []*profilev1.Label{{Key: 5, Str: 6}, {Key: 7, Str: 8}, {Key: 9, Str: 10}})
}

func Test_GetProfileLanguage_go_cpu_profile(t *testing.T) {
Expand Down
Binary file not shown.
1 change: 1 addition & 0 deletions pkg/test/integration/helper.go
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,7 @@ func (p *PyroscopeTest) Start(t *testing.T) {
p.config.MemberlistKV.AdvertisePort = p.memberlistPort
p.config.MemberlistKV.TCPTransport.BindPort = p.memberlistPort
p.config.Ingester.LifecyclerConfig.Addr = address
p.config.Ingester.LifecyclerConfig.MinReadyDuration = 0
p.config.QueryScheduler.ServiceDiscovery.SchedulerRing.InstanceAddr = address
p.config.Frontend.Addr = address

Expand Down

0 comments on commit 96cad4a

Please sign in to comment.