Skip to content

Commit c0df078

Browse files
bryanhuhtabryanhuhta
authored
Don't 500 on bad profile types (#4537)
1 parent 166e8d3 commit c0df078

File tree

8 files changed

+296
-3
lines changed

8 files changed

+296
-3
lines changed

‎.gitignore‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,7 @@ data-metastore/
2424
.cursor
2525
.idea
2626
.vscode
27+
.zed
2728

2829
# Frontend
2930
public/build

‎pkg/frontend/readpath/queryfrontend/query_diff.go‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,16 @@ func (q *QueryFrontend) Diff(
3232
c.Msg.Left.MaxNodes = &maxNodes
3333
c.Msg.Right.MaxNodes = &maxNodes
3434

35+
_, err = phlaremodel.ParseProfileTypeSelector(c.Msg.Left.ProfileTypeID)
36+
if err != nil {
37+
return nil, connect.NewError(connect.CodeInvalidArgument, err)
38+
}
39+
40+
_, err = phlaremodel.ParseProfileTypeSelector(c.Msg.Right.ProfileTypeID)
41+
if err != nil {
42+
return nil, connect.NewError(connect.CodeInvalidArgument, err)
43+
}
44+
3545
var left, right []byte
3646
g, ctx := errgroup.WithContext(ctx)
3747
g.Go(func() error {

‎pkg/frontend/readpath/queryfrontend/query_select_merge_profile.go‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@ import (
99
profilev1 "github.com/grafana/pyroscope/api/gen/proto/go/google/v1"
1010
querierv1 "github.com/grafana/pyroscope/api/gen/proto/go/querier/v1"
1111
queryv1 "github.com/grafana/pyroscope/api/gen/proto/go/query/v1"
12+
phlaremodel "github.com/grafana/pyroscope/pkg/model"
1213
"github.com/grafana/pyroscope/pkg/pprof"
1314
"github.com/grafana/pyroscope/pkg/validation"
1415
)
@@ -29,6 +30,11 @@ func (q *QueryFrontend) SelectMergeProfile(
2930
return connect.NewResponse(&profilev1.Profile{}), nil
3031
}
3132

33+
_, err = phlaremodel.ParseProfileTypeSelector(c.Msg.ProfileTypeID)
34+
if err != nil {
35+
return nil, connect.NewError(connect.CodeInvalidArgument, err)
36+
}
37+
3238
// NOTE: Max nodes limit is not set by default:
3339
// the method is used for pprof export and
3440
// truncation might not be desirable.

‎pkg/frontend/readpath/queryfrontend/query_select_merge_span_profile.go‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,11 @@ func (q *QueryFrontend) SelectMergeSpanProfile(
3030
return connect.NewResponse(&querierv1.SelectMergeSpanProfileResponse{}), nil
3131
}
3232

33+
_, err = phlaremodel.ParseProfileTypeSelector(c.Msg.ProfileTypeID)
34+
if err != nil {
35+
return nil, connect.NewError(connect.CodeInvalidArgument, err)
36+
}
37+
3338
maxNodes, err := validation.ValidateMaxNodes(q.limits, tenantIDs, c.Msg.GetMaxNodes())
3439
if err != nil {
3540
return nil, connect.NewError(connect.CodeInvalidArgument, err)

‎pkg/frontend/readpath/queryfrontend/query_select_merge_stacktraces.go‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -54,6 +54,12 @@ func (q *QueryFrontend) selectMergeStacktracesTree(
5454
if err != nil {
5555
return nil, connect.NewError(connect.CodeInvalidArgument, err)
5656
}
57+
58+
_, err = phlaremodel.ParseProfileTypeSelector(c.Msg.ProfileTypeID)
59+
if err != nil {
60+
return nil, connect.NewError(connect.CodeInvalidArgument, err)
61+
}
62+
5763
labelSelector, err := buildLabelSelectorWithProfileType(c.Msg.LabelSelector, c.Msg.ProfileTypeID)
5864
if err != nil {
5965
return nil, err

‎pkg/frontend/readpath/queryfrontend/query_select_time_series.go‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,11 @@ func (q *QueryFrontend) SelectSeries(
2929
return connect.NewResponse(&querierv1.SelectSeriesResponse{}), nil
3030
}
3131

32+
_, err = phlaremodel.ParseProfileTypeSelector(c.Msg.ProfileTypeID)
33+
if err != nil {
34+
return nil, connect.NewError(connect.CodeInvalidArgument, err)
35+
}
36+
3237
stepMs := time.Duration(c.Msg.Step * float64(time.Second)).Milliseconds()
3338
start := c.Msg.Start - stepMs
3439

‎pkg/model/profile.go‎

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -8,9 +8,7 @@ import (
88
"strings"
99

1010
"github.com/cespare/xxhash/v2"
11-
"github.com/gogo/status"
1211
"github.com/prometheus/prometheus/model/labels"
13-
"google.golang.org/grpc/codes"
1412

1513
profilev1 "github.com/grafana/pyroscope/api/gen/proto/go/google/v1"
1614
ingestv1 "github.com/grafana/pyroscope/api/gen/proto/go/ingester/v1"
@@ -31,7 +29,7 @@ func ParseProfileTypeSelector(id string) (*typesv1.ProfileType, error) {
3129
parts := strings.Split(id, ":")
3230

3331
if len(parts) != 5 && len(parts) != 6 {
34-
return nil, status.Errorf(codes.InvalidArgument, "profile-type selection must be of the form <name>:<sample-type>:<sample-unit>:<period-type>:<period-unit>(:delta), got(%d): %q", len(parts), id)
32+
return nil, fmt.Errorf("profile-type selection must be of the form <name>:<sample-type>:<sample-unit>:<period-type>:<period-unit>(:delta), got(%d): %q", len(parts), id)
3533
}
3634
name, sampleType, sampleUnit, periodType, periodUnit := parts[0], parts[1], parts[2], parts[3], parts[4]
3735
return &typesv1.ProfileType{

‎pkg/model/profile_test.go‎

Lines changed: 262 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,8 +4,10 @@ import (
44
"testing"
55

66
"github.com/stretchr/testify/assert"
7+
"github.com/stretchr/testify/require"
78

89
profilev1 "github.com/grafana/pyroscope/api/gen/proto/go/google/v1"
10+
typesv1 "github.com/grafana/pyroscope/api/gen/proto/go/types/v1"
911
)
1012

1113
func Test_extractMappingFilename(t *testing.T) {
@@ -97,3 +99,263 @@ func Test_symbolsPartitionKeyForProfile(t *testing.T) {
9799
})
98100
}
99101
}
102+
103+
func Test_ParseProfileTypeSelector(t *testing.T) {
104+
tests := []struct {
105+
Name string
106+
ID string
107+
Want *typesv1.ProfileType
108+
WantErr string
109+
}{
110+
{
111+
Name: "block_contentions_count",
112+
ID: "block:contentions:count:contentions:count",
113+
Want: &typesv1.ProfileType{
114+
Name: "block",
115+
ID: "block:contentions:count:contentions:count",
116+
SampleType: "contentions",
117+
SampleUnit: "count",
118+
PeriodType: "contentions",
119+
PeriodUnit: "count",
120+
},
121+
},
122+
{
123+
Name: "mutex_delay_nanoseconds",
124+
ID: "mutex:delay:nanoseconds:contentions:count",
125+
Want: &typesv1.ProfileType{
126+
Name: "mutex",
127+
ID: "mutex:delay:nanoseconds:contentions:count",
128+
SampleType: "delay",
129+
SampleUnit: "nanoseconds",
130+
PeriodType: "contentions",
131+
PeriodUnit: "count",
132+
},
133+
},
134+
{
135+
Name: "memory_alloc_space_bytes",
136+
ID: "memory:alloc_space:bytes:space:bytes",
137+
Want: &typesv1.ProfileType{
138+
Name: "memory",
139+
ID: "memory:alloc_space:bytes:space:bytes",
140+
SampleType: "alloc_space",
141+
SampleUnit: "bytes",
142+
PeriodType: "space",
143+
PeriodUnit: "bytes",
144+
},
145+
},
146+
{
147+
Name: "too_few_parts",
148+
ID: "memory:alloc_space:bytes:space",
149+
WantErr: `profile-type selection must be of the form <name>:<sample-type>:<sample-unit>:<period-type>:<period-unit>(:delta), got(4): "memory:alloc_space:bytes:space"`,
150+
},
151+
{
152+
Name: "too_many_parts",
153+
ID: "memory:alloc_space:bytes:space:bytes:extra:part",
154+
WantErr: `profile-type selection must be of the form <name>:<sample-type>:<sample-unit>:<period-type>:<period-unit>(:delta), got(7): "memory:alloc_space:bytes:space:bytes:extra:part"`,
155+
},
156+
{
157+
Name: "empty_string",
158+
ID: "",
159+
WantErr: `profile-type selection must be of the form <name>:<sample-type>:<sample-unit>:<period-type>:<period-unit>(:delta), got(1): ""`,
160+
},
161+
{
162+
Name: "valid_with_delta",
163+
ID: "cpu:samples:count:cpu:nanoseconds:delta",
164+
Want: &typesv1.ProfileType{
165+
Name: "cpu",
166+
ID: "cpu:samples:count:cpu:nanoseconds:delta",
167+
SampleType: "samples",
168+
SampleUnit: "count",
169+
PeriodType: "cpu",
170+
PeriodUnit: "nanoseconds",
171+
},
172+
},
173+
}
174+
175+
for _, tt := range tests {
176+
t.Run(tt.Name, func(t *testing.T) {
177+
got, err := ParseProfileTypeSelector(tt.ID)
178+
if tt.WantErr != "" {
179+
require.Error(t, err)
180+
require.EqualError(t, err, tt.WantErr)
181+
} else {
182+
require.NoError(t, err)
183+
require.Equal(t, tt.Want, got)
184+
}
185+
})
186+
}
187+
}
188+
189+
func Test_ParseProfileTypeSelector_ValidProfileTypes(t *testing.T) {
190+
// Shamelessly copied from: https://github.com/grafana/profiles-drilldown/blob/4e261a8744034bddefdec757d5d2e1d8dc0ec2bb/src/shared/infrastructure/profile-metrics/profile-metrics.json#L93
191+
var validProfileTypes = map[string]*typesv1.ProfileType{
192+
"block:contentions:count:contentions:count": {
193+
ID: "block:contentions:count:contentions:count",
194+
Name: "block",
195+
SampleType: "contentions",
196+
SampleUnit: "count",
197+
PeriodType: "contentions",
198+
PeriodUnit: "count",
199+
},
200+
"block:delay:nanoseconds:contentions:count": {
201+
ID: "block:delay:nanoseconds:contentions:count",
202+
Name: "block",
203+
SampleType: "delay",
204+
SampleUnit: "nanoseconds",
205+
PeriodType: "contentions",
206+
PeriodUnit: "count",
207+
},
208+
"goroutine:goroutine:count:goroutine:count": {
209+
ID: "goroutine:goroutine:count:goroutine:count",
210+
Name: "goroutine",
211+
SampleType: "goroutine",
212+
SampleUnit: "count",
213+
PeriodType: "goroutine",
214+
PeriodUnit: "count",
215+
},
216+
"goroutines:goroutine:count:goroutine:count": {
217+
ID: "goroutines:goroutine:count:goroutine:count",
218+
Name: "goroutines",
219+
SampleType: "goroutine",
220+
SampleUnit: "count",
221+
PeriodType: "goroutine",
222+
PeriodUnit: "count",
223+
},
224+
"memory:alloc_in_new_tlab_bytes:bytes::": {
225+
ID: "memory:alloc_in_new_tlab_bytes:bytes::",
226+
Name: "memory",
227+
SampleType: "alloc_in_new_tlab_bytes",
228+
SampleUnit: "bytes",
229+
PeriodType: "",
230+
PeriodUnit: "",
231+
},
232+
"memory:alloc_in_new_tlab_objects:count::": {
233+
ID: "memory:alloc_in_new_tlab_objects:count::",
234+
Name: "memory",
235+
SampleType: "alloc_in_new_tlab_objects",
236+
SampleUnit: "count",
237+
PeriodType: "",
238+
PeriodUnit: "",
239+
},
240+
"memory:alloc_objects:count:space:bytes": {
241+
ID: "memory:alloc_objects:count:space:bytes",
242+
Name: "memory",
243+
SampleType: "alloc_objects",
244+
SampleUnit: "count",
245+
PeriodType: "space",
246+
PeriodUnit: "bytes",
247+
},
248+
"memory:alloc_space:bytes:space:bytes": {
249+
ID: "memory:alloc_space:bytes:space:bytes",
250+
Name: "memory",
251+
SampleType: "alloc_space",
252+
SampleUnit: "bytes",
253+
PeriodType: "space",
254+
PeriodUnit: "bytes",
255+
},
256+
"memory:inuse_objects:count:space:bytes": {
257+
ID: "memory:inuse_objects:count:space:bytes",
258+
Name: "memory",
259+
SampleType: "inuse_objects",
260+
SampleUnit: "count",
261+
PeriodType: "space",
262+
PeriodUnit: "bytes",
263+
},
264+
"memory:inuse_space:bytes:space:bytes": {
265+
ID: "memory:inuse_space:bytes:space:bytes",
266+
Name: "memory",
267+
SampleType: "inuse_space",
268+
SampleUnit: "bytes",
269+
PeriodType: "space",
270+
PeriodUnit: "bytes",
271+
},
272+
"mutex:contentions:count:contentions:count": {
273+
ID: "mutex:contentions:count:contentions:count",
274+
Name: "mutex",
275+
SampleType: "contentions",
276+
SampleUnit: "count",
277+
PeriodType: "contentions",
278+
PeriodUnit: "count",
279+
},
280+
"mutex:delay:nanoseconds:contentions:count": {
281+
ID: "mutex:delay:nanoseconds:contentions:count",
282+
Name: "mutex",
283+
SampleType: "delay",
284+
SampleUnit: "nanoseconds",
285+
PeriodType: "contentions",
286+
PeriodUnit: "count",
287+
},
288+
"process_cpu:alloc_samples:count:cpu:nanoseconds": {
289+
ID: "process_cpu:alloc_samples:count:cpu:nanoseconds",
290+
Name: "process_cpu",
291+
SampleType: "alloc_samples",
292+
SampleUnit: "count",
293+
PeriodType: "cpu",
294+
PeriodUnit: "nanoseconds",
295+
},
296+
"process_cpu:alloc_size:bytes:cpu:nanoseconds": {
297+
ID: "process_cpu:alloc_size:bytes:cpu:nanoseconds",
298+
Name: "process_cpu",
299+
SampleType: "alloc_size",
300+
SampleUnit: "bytes",
301+
PeriodType: "cpu",
302+
PeriodUnit: "nanoseconds",
303+
},
304+
"process_cpu:cpu:nanoseconds:cpu:nanoseconds": {
305+
ID: "process_cpu:cpu:nanoseconds:cpu:nanoseconds",
306+
Name: "process_cpu",
307+
SampleType: "cpu",
308+
SampleUnit: "nanoseconds",
309+
PeriodType: "cpu",
310+
PeriodUnit: "nanoseconds",
311+
},
312+
"process_cpu:exception:count:cpu:nanoseconds": {
313+
ID: "process_cpu:exception:count:cpu:nanoseconds",
314+
Name: "process_cpu",
315+
SampleType: "exception",
316+
SampleUnit: "count",
317+
PeriodType: "cpu",
318+
PeriodUnit: "nanoseconds",
319+
},
320+
"process_cpu:lock_count:count:cpu:nanoseconds": {
321+
ID: "process_cpu:lock_count:count:cpu:nanoseconds",
322+
Name: "process_cpu",
323+
SampleType: "lock_count",
324+
SampleUnit: "count",
325+
PeriodType: "cpu",
326+
PeriodUnit: "nanoseconds",
327+
},
328+
"process_cpu:lock_time:nanoseconds:cpu:nanoseconds": {
329+
ID: "process_cpu:lock_time:nanoseconds:cpu:nanoseconds",
330+
Name: "process_cpu",
331+
SampleType: "lock_time",
332+
SampleUnit: "nanoseconds",
333+
PeriodType: "cpu",
334+
PeriodUnit: "nanoseconds",
335+
},
336+
"process_cpu:samples:count::milliseconds": {
337+
ID: "process_cpu:samples:count::milliseconds",
338+
Name: "process_cpu",
339+
SampleType: "samples",
340+
SampleUnit: "count",
341+
PeriodType: "",
342+
PeriodUnit: "milliseconds",
343+
},
344+
"process_cpu:samples:count:cpu:nanoseconds": {
345+
ID: "process_cpu:samples:count:cpu:nanoseconds",
346+
Name: "process_cpu",
347+
SampleType: "samples",
348+
SampleUnit: "count",
349+
PeriodType: "cpu",
350+
PeriodUnit: "nanoseconds",
351+
},
352+
}
353+
354+
for id, want := range validProfileTypes {
355+
t.Run(id, func(t *testing.T) {
356+
got, err := ParseProfileTypeSelector(id)
357+
require.NoError(t, err)
358+
require.Equal(t, want, got)
359+
})
360+
}
361+
}

0 commit comments

Comments
 (0)