Skip to content

Commit 0829ec7

Browse files
ZylphrexZylphrex
authored
Merge pull request #637 from getsentry/revert-635-txiao/fix/more-accurate-average-computation-for-flamegraph-metrics
Revert "fix(flamegraph): More accurage average computation for flamegraph met…"
2 parents 69119e6 + 9a60c13 commit 0829ec7

File tree

9 files changed

+312
-535
lines changed

9 files changed

+312
-535
lines changed

‎CHANGELOG.md‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -35,7 +35,7 @@
3535
- Add default non-root user in the Docker image. ([#593](https://github.com/getsentry/vroom/pull/593))
3636
- Classify macOS frames from an application as application frames. ([#604](https://github.com/getsentry/vroom/pull/604))
3737
- Return number of occurrences in flamegraph. ([#622](https://github.com/getsentry/vroom/pull/622), [#625](https://github.com/getsentry/vroom/pull/625))
38-
- Use function duration when computing metrics ([#627](https://github.com/getsentry/vroom/pull/627), [#628](https://github.com/getsentry/vroom/pull/628), [#629](https://github.com/getsentry/vroom/pull/629), [#630](https://github.com/getsentry/vroom/pull/630), [#635](https://github.com/getsentry/vroom/pull/635))
38+
- Use function duration when computing metrics ([#627](https://github.com/getsentry/vroom/pull/627), [#628](https://github.com/getsentry/vroom/pull/628), [#629](https://github.com/getsentry/vroom/pull/629), [#630](https://github.com/getsentry/vroom/pull/630))
3939
- Remove Unused metrics endpoint ([#633](https://github.com/getsentry/vroom/pull/633))
4040

4141
**Bug Fixes**:

‎cmd/vroom/flamegraph.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -57,7 +57,7 @@ func (env *environment) postFlamegraph(w http.ResponseWriter, r *http.Request) {
5757
s = sentry.StartSpan(ctx, "processing")
5858
var ma *metrics.Aggregator
5959
if body.GenerateMetrics {
60-
agg := metrics.NewAggregator(maxUniqueFunctionsPerProfile, 5, int(minDepth), false)
60+
agg := metrics.NewAggregator(maxUniqueFunctionsPerProfile, 5, minDepth)
6161
ma = &agg
6262
}
6363
speedscope, err := flamegraph.GetFlamegraphFromCandidates(

‎internal/flamegraph/flamegraph.go‎

Lines changed: 17 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -49,34 +49,27 @@ func sumNodesSampleCount(nodes []*nodetree.Node) int {
4949
return c
5050
}
5151

52-
func annotateWithProfileExample(example examples.ExampleMetadata) func(n, m *nodetree.Node) {
53-
return func(n, m *nodetree.Node) {
52+
func annotateWithProfileExample(example examples.ExampleMetadata) func(n *nodetree.Node) {
53+
return func(n *nodetree.Node) {
5454
n.Profiles[example] = void
55-
if n.WorstSelfTime < m.SelfTimeNS {
56-
n.WorstSelfTime = m.SelfTimeNS
57-
n.WorstProfile = example
58-
}
5955
}
6056
}
6157

62-
func addCallTreeToFlamegraph(flamegraphTree *[]*nodetree.Node, callTree []*nodetree.Node, annotate func(n, m *nodetree.Node)) {
58+
func addCallTreeToFlamegraph(flamegraphTree *[]*nodetree.Node, callTree []*nodetree.Node, annotate func(n *nodetree.Node)) {
6359
for _, node := range callTree {
6460
var currentNode *nodetree.Node
6561
if existingNode := getMatchingNode(flamegraphTree, node); existingNode != nil {
6662
currentNode = existingNode
6763
currentNode.Occurrence += node.Occurrence
6864
currentNode.SampleCount += node.SampleCount
6965
currentNode.DurationNS += node.DurationNS
70-
currentNode.SelfTimeNS += node.SelfTimeNS
71-
currentNode.DurationsNS = append(currentNode.DurationsNS, node.DurationNS)
7266
} else {
7367
currentNode = node.ShallowCopyWithoutChildren()
74-
currentNode.DurationsNS = []uint64{node.DurationNS}
7568
*flamegraphTree = append(*flamegraphTree, currentNode)
7669
}
7770
addCallTreeToFlamegraph(&currentNode.Children, node.Children, annotate)
7871
if node.SampleCount > sumNodesSampleCount(node.Children) {
79-
annotate(currentNode, node)
72+
annotate(currentNode)
8073
}
8174
}
8275
}
@@ -443,6 +436,12 @@ func GetFlamegraphFromCandidates(
443436
for _, callTree := range result.CallTrees {
444437
addCallTreeToFlamegraph(&flamegraphTree, callTree, annotate)
445438
}
439+
// if metrics aggregator is not null, while we're at it,
440+
// compute the metrics as well
441+
if ma != nil {
442+
functions := metrics.CapAndFilterFunctions(metrics.ExtractFunctionsFromCallTrees(result.CallTrees, ma.MinDepth), int(ma.MaxUniqueFunctions), true)
443+
ma.AddFunctions(functions, example)
444+
}
446445

447446
transactionProfileSpan.Finish()
448447
} else if result, ok := res.(chunk.CallTreesReadJobResult); ok {
@@ -470,6 +469,13 @@ func GetFlamegraphFromCandidates(
470469
annotate := annotateWithProfileExample(example)
471470

472471
addCallTreeToFlamegraph(&flamegraphTree, callTree, annotate)
472+
473+
// if metrics aggregator is not null, while we're at it,
474+
// compute the metrics as well
475+
if ma != nil {
476+
functions := metrics.CapAndFilterFunctions(metrics.ExtractFunctionsFromCallTreesForThread(callTree, ma.MinDepth), int(ma.MaxUniqueFunctions), true)
477+
ma.AddFunctions(functions, example)
478+
}
473479
}
474480
chunkProfileSpan.Finish()
475481
} else {
@@ -483,21 +489,10 @@ func GetFlamegraphFromCandidates(
483489
serializeSpan := span.StartChild("serialize")
484490
defer serializeSpan.Finish()
485491

486-
speedscopeSpan := span.StartChild("processing speedscope")
487492
sp := toSpeedscope(ctx, flamegraphTree, 1000, 0)
488-
speedscopeSpan.Finish()
489-
490-
// if metrics aggregator is not null, while we're at it,
491-
// compute the metrics as well
492493
if ma != nil {
493-
metricsSpan := span.StartChild("processing metrics")
494-
for _, tree := range flamegraphTree {
495-
tree.Visit(ma.AddFunction)
496-
}
497494
fm := ma.ToMetrics()
498495
sp.Metrics = &fm
499-
metricsSpan.Finish()
500496
}
501-
502497
return sp, nil
503498
}

‎internal/flamegraph/flamegraph_test.go‎

Lines changed: 70 additions & 99 deletions
Original file line numberDiff line numberDiff line change
@@ -127,10 +127,6 @@ func TestFlamegraphAggregation(t *testing.T) {
127127
ElapsedSinceStartNS: 20,
128128
StackID: 2,
129129
},
130-
{
131-
ElapsedSinceStartNS: 30,
132-
StackID: 2,
133-
},
134130
}, // end Samples
135131
}, // end Trace
136132
},
@@ -144,7 +140,7 @@ func TestFlamegraphAggregation(t *testing.T) {
144140
},
145141
Profiles: []interface{}{
146142
speedscope.SampledProfile{
147-
EndValue: 7,
143+
EndValue: 6,
148144
IsMainThread: true,
149145
Samples: [][]int{
150146
{2},
@@ -156,13 +152,13 @@ func TestFlamegraphAggregation(t *testing.T) {
156152
{0},
157153
{1},
158154
{1},
159-
{0, 1},
155+
{0},
160156
},
161157
Type: "sampled",
162158
Unit: "count",
163-
Weights: []uint64{1, 1, 1, 4},
164-
SampleCounts: []uint64{1, 1, 1, 4},
165-
SampleDurationsNs: []uint64{10, 10, 10, 40},
159+
Weights: []uint64{1, 1, 1, 3},
160+
SampleCounts: []uint64{1, 1, 1, 3},
161+
SampleDurationsNs: []uint64{10, 10, 10, 30},
166162
},
167163
},
168164
Shared: speedscope.SharedData{
@@ -173,8 +169,8 @@ func TestFlamegraphAggregation(t *testing.T) {
173169
{Image: "test.package", Name: "e", Fingerprint: 2430275448},
174170
},
175171
FrameInfos: []speedscope.FrameInfo{
176-
{Count: 4, Weight: 50},
177172
{Count: 3, Weight: 40},
173+
{Count: 2, Weight: 30},
178174
{Count: 2, Weight: 20},
179175
{Count: 1, Weight: 10},
180176
},
@@ -185,62 +181,17 @@ func TestFlamegraphAggregation(t *testing.T) {
185181
},
186182
},
187183
metrics: []examples.FunctionMetrics{
188-
{
189-
Name: "a",
190-
Package: "test.package",
191-
Fingerprint: 2430275452,
192-
P75: 10,
193-
P95: 20,
194-
P99: 20,
195-
Avg: float64(50) / float64(4),
196-
Sum: 50,
197-
SumSelfTime: 10,
198-
Count: 5,
199-
Worst: examples.ExampleMetadata{ProfileID: "cd2"},
200-
Examples: []examples.ExampleMetadata{{ProfileID: "cd2"}},
201-
},
202184
{
203185
Name: "b",
204186
Package: "test.package",
205187
Fingerprint: 2430275455,
206188
P75: 20,
207189
P95: 20,
208190
P99: 20,
209-
Avg: float64(40) / float64(3),
210-
Sum: 40,
211-
SumSelfTime: 40,
212-
Count: 4,
213-
Worst: examples.ExampleMetadata{ProfileID: "ab1"},
214-
Examples: []examples.ExampleMetadata{{ProfileID: "ab1"}, {ProfileID: "cd2"}},
215-
},
216-
{
217-
Name: "c",
218-
Package: "test.package",
219-
Fingerprint: 2430275454,
220-
InApp: true,
221-
P75: 10,
222-
P95: 10,
223-
P99: 10,
224-
Avg: 10,
225-
Sum: 20,
226-
SumSelfTime: 20,
227-
Count: 2,
228-
Worst: examples.ExampleMetadata{ProfileID: "ab1"},
229-
Examples: []examples.ExampleMetadata{{ProfileID: "ab1"}},
230-
},
231-
{
232-
Name: "e",
233-
Package: "test.package",
234-
Fingerprint: 2430275448,
235-
P75: 10,
236-
P95: 10,
237-
P99: 10,
238-
Avg: 10,
239-
Sum: 10,
240-
SumSelfTime: 10,
241-
Count: 1,
242-
Worst: examples.ExampleMetadata{ProfileID: "cd2"},
243-
Examples: []examples.ExampleMetadata{{ProfileID: "cd2"}},
191+
Avg: 15,
192+
Sum: 30,
193+
SumSelfTime: 30,
194+
Count: 3,
244195
},
245196
},
246197
},
@@ -365,6 +316,19 @@ func TestFlamegraphAggregation(t *testing.T) {
365316
},
366317
},
367318
metrics: []examples.FunctionMetrics{
319+
{
320+
Name: "c",
321+
Package: "test.package",
322+
Fingerprint: 2430275454,
323+
InApp: true,
324+
P75: 30,
325+
P95: 30,
326+
P99: 30,
327+
Avg: 25,
328+
Sum: 50,
329+
SumSelfTime: 50,
330+
Count: 5,
331+
},
368332
{
369333
Name: "a",
370334
Package: "test.package",
@@ -377,8 +341,6 @@ func TestFlamegraphAggregation(t *testing.T) {
377341
Sum: 90,
378342
SumSelfTime: 20,
379343
Count: 9,
380-
Worst: examples.ExampleMetadata{ProfileID: "ab1"},
381-
Examples: []examples.ExampleMetadata{{ProfileID: "ab1"}},
382344
},
383345
{
384346
Name: "b",
@@ -392,23 +354,6 @@ func TestFlamegraphAggregation(t *testing.T) {
392354
Sum: 70,
393355
SumSelfTime: 20,
394356
Count: 7,
395-
Worst: examples.ExampleMetadata{ProfileID: "ab1"},
396-
Examples: []examples.ExampleMetadata{{ProfileID: "ab1"}},
397-
},
398-
{
399-
Name: "c",
400-
Package: "test.package",
401-
Fingerprint: 2430275454,
402-
InApp: true,
403-
P75: 30,
404-
P95: 30,
405-
P99: 30,
406-
Avg: 25,
407-
Sum: 50,
408-
SumSelfTime: 50,
409-
Count: 5,
410-
Worst: examples.ExampleMetadata{ProfileID: "ab1"},
411-
Examples: []examples.ExampleMetadata{{ProfileID: "ab1"}},
412357
},
413358
},
414359
},
@@ -515,34 +460,30 @@ func TestFlamegraphAggregation(t *testing.T) {
515460
},
516461
metrics: []examples.FunctionMetrics{
517462
{
518-
Name: "b",
463+
Name: "c",
519464
Package: "test.package",
520-
Fingerprint: 2430275455,
465+
Fingerprint: 2430275454,
521466
InApp: true,
522467
P75: 30,
523468
P95: 30,
524469
P99: 30,
525-
Avg: 25,
526-
Sum: 50,
527-
SumSelfTime: 20,
528-
Count: 5,
529-
Worst: examples.ExampleMetadata{ProfileID: "ab1"},
530-
Examples: []examples.ExampleMetadata{{ProfileID: "ab1"}},
470+
Avg: 30,
471+
Sum: 30,
472+
SumSelfTime: 30,
473+
Count: 3,
531474
},
532475
{
533-
Name: "c",
476+
Name: "b",
534477
Package: "test.package",
535-
Fingerprint: 2430275454,
478+
Fingerprint: 2430275455,
536479
InApp: true,
537480
P75: 30,
538481
P95: 30,
539482
P99: 30,
540-
Avg: 30,
541-
Sum: 30,
542-
SumSelfTime: 30,
543-
Count: 3,
544-
Worst: examples.ExampleMetadata{ProfileID: "ab1"},
545-
Examples: []examples.ExampleMetadata{{ProfileID: "ab1"}},
483+
Avg: 25,
484+
Sum: 50,
485+
SumSelfTime: 20,
486+
Count: 5,
546487
},
547488
},
548489
},
@@ -551,7 +492,6 @@ func TestFlamegraphAggregation(t *testing.T) {
551492
for _, test := range tests {
552493
t.Run(test.name, func(t *testing.T) {
553494
var ft []*nodetree.Node
554-
555495
for _, sp := range test.profiles {
556496
p := profile.New(&sp)
557497
callTrees, err := p.CallTrees()
@@ -568,13 +508,44 @@ func TestFlamegraphAggregation(t *testing.T) {
568508
t.Fatalf("Result mismatch: got - want +\n%s", diff)
569509
}
570510

571-
ma := metrics.NewAggregator(100, 5, 0, false)
572-
for _, tree := range ft {
573-
tree.Visit(ma.AddFunction)
511+
ma := metrics.NewAggregator(
512+
100,
513+
5,
514+
0,
515+
)
516+
517+
for _, sp := range test.profiles {
518+
p := profile.New(&sp)
519+
callTrees, err := p.CallTrees()
520+
if err != nil {
521+
t.Fatalf("error when generating calltrees: %v", err)
522+
}
523+
524+
start, end := p.StartAndEndEpoch()
525+
example := examples.NewExampleFromProfileID(
526+
p.ProjectID(),
527+
p.ID(),
528+
start,
529+
end,
530+
)
531+
532+
functions := metrics.CapAndFilterFunctions(
533+
metrics.ExtractFunctionsFromCallTrees(
534+
callTrees,
535+
ma.MinDepth,
536+
),
537+
int(ma.MaxUniqueFunctions),
538+
false,
539+
)
540+
ma.AddFunctions(functions, example)
574541
}
575542
m := ma.ToMetrics()
576543

577-
if diff := testutil.Diff(m, test.metrics); diff != "" {
544+
options := cmp.Options{
545+
cmpopts.IgnoreFields(examples.FunctionMetrics{}, "Worst"),
546+
cmpopts.IgnoreFields(examples.FunctionMetrics{}, "Examples"),
547+
}
548+
if diff := testutil.Diff(m, test.metrics, options); diff != "" {
578549
t.Fatalf("Result mismatch: got - want +\n%s", diff)
579550
}
580551
})

0 commit comments

Comments
 (0)