fix(flamegraph): More accurage average computation for flamegraph met…

CHANGELOG.md

@@ -35,7 +35,7 @@
 - Add default non-root user in the Docker image. ([#593](https://github.com/getsentry/vroom/pull/593))
 - Classify macOS frames from an application as application frames. ([#604](https://github.com/getsentry/vroom/pull/604))
 - Return number of occurrences in flamegraph. ([#622](https://github.com/getsentry/vroom/pull/622), [#625](https://github.com/getsentry/vroom/pull/625))
-- 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))
+- 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))
 - Remove Unused metrics endpoint ([#633](https://github.com/getsentry/vroom/pull/633))
 
 **Bug Fixes**:

cmd/vroom/flamegraph.go

@@ -57,7 +57,7 @@ func (env *environment) postFlamegraph(w http.ResponseWriter, r *http.Request) {
 	s = sentry.StartSpan(ctx, "processing")
 	var ma *metrics.Aggregator
 	if body.GenerateMetrics {
-		agg := metrics.NewAggregator(maxUniqueFunctionsPerProfile, 5, minDepth)
+		agg := metrics.NewAggregator(maxUniqueFunctionsPerProfile, 5, int(minDepth), false)
 		ma = &agg
 	}
 	speedscope, err := flamegraph.GetFlamegraphFromCandidates(

internal/flamegraph/flamegraph.go

@@ -49,27 +49,34 @@ func sumNodesSampleCount(nodes []*nodetree.Node) int {
 	return c
 }
 
-func annotateWithProfileExample(example examples.ExampleMetadata) func(n *nodetree.Node) {
-	return func(n *nodetree.Node) {
+func annotateWithProfileExample(example examples.ExampleMetadata) func(n, m *nodetree.Node) {
+	return func(n, m *nodetree.Node) {
 		n.Profiles[example] = void
+		if n.WorstSelfTime < m.SelfTimeNS {
+			n.WorstSelfTime = m.SelfTimeNS
+			n.WorstProfile = example
+		}
 	}
 }
 
-func addCallTreeToFlamegraph(flamegraphTree *[]*nodetree.Node, callTree []*nodetree.Node, annotate func(n *nodetree.Node)) {
+func addCallTreeToFlamegraph(flamegraphTree *[]*nodetree.Node, callTree []*nodetree.Node, annotate func(n, m *nodetree.Node)) {
 	for _, node := range callTree {
 		var currentNode *nodetree.Node
 		if existingNode := getMatchingNode(flamegraphTree, node); existingNode != nil {
 			currentNode = existingNode
 			currentNode.Occurrence += node.Occurrence
 			currentNode.SampleCount += node.SampleCount
 			currentNode.DurationNS += node.DurationNS
+			currentNode.SelfTimeNS += node.SelfTimeNS
+			currentNode.DurationsNS = append(currentNode.DurationsNS, node.DurationNS)
 		} else {
 			currentNode = node.ShallowCopyWithoutChildren()
+			currentNode.DurationsNS = []uint64{node.DurationNS}
 			*flamegraphTree = append(*flamegraphTree, currentNode)
 		}
 		addCallTreeToFlamegraph(&currentNode.Children, node.Children, annotate)
 		if node.SampleCount > sumNodesSampleCount(node.Children) {
-			annotate(currentNode)
+			annotate(currentNode, node)
 		}
 	}
 }
@@ -436,12 +443,6 @@ func GetFlamegraphFromCandidates(
 			for _, callTree := range result.CallTrees {
 				addCallTreeToFlamegraph(&flamegraphTree, callTree, annotate)
 			}
-			// if metrics aggregator is not null, while we're at it,
-			// compute the metrics as well
-			if ma != nil {
-				functions := metrics.CapAndFilterFunctions(metrics.ExtractFunctionsFromCallTrees(result.CallTrees, ma.MinDepth), int(ma.MaxUniqueFunctions), true)
-				ma.AddFunctions(functions, example)
-			}
 
 			transactionProfileSpan.Finish()
 		} else if result, ok := res.(chunk.CallTreesReadJobResult); ok {
@@ -469,13 +470,6 @@ func GetFlamegraphFromCandidates(
 				annotate := annotateWithProfileExample(example)
 
 				addCallTreeToFlamegraph(&flamegraphTree, callTree, annotate)
-
-				// if metrics aggregator is not null, while we're at it,
-				// compute the metrics as well
-				if ma != nil {
-					functions := metrics.CapAndFilterFunctions(metrics.ExtractFunctionsFromCallTreesForThread(callTree, ma.MinDepth), int(ma.MaxUniqueFunctions), true)
-					ma.AddFunctions(functions, example)
-				}
 			}
 			chunkProfileSpan.Finish()
 		} else {
@@ -489,10 +483,21 @@ func GetFlamegraphFromCandidates(
 	serializeSpan := span.StartChild("serialize")
 	defer serializeSpan.Finish()
 
+	speedscopeSpan := span.StartChild("processing speedscope")
 	sp := toSpeedscope(ctx, flamegraphTree, 1000, 0)
+	speedscopeSpan.Finish()
+
+	// if metrics aggregator is not null, while we're at it,
+	// compute the metrics as well
 	if ma != nil {
+		metricsSpan := span.StartChild("processing metrics")
+		for _, tree := range flamegraphTree {
+			tree.Visit(ma.AddFunction)
+		}
 		fm := ma.ToMetrics()
 		sp.Metrics = &fm
+		metricsSpan.Finish()
 	}
+
 	return sp, nil
 }

internal/flamegraph/flamegraph_test.go

@@ -127,6 +127,10 @@ func TestFlamegraphAggregation(t *testing.T) {
 									ElapsedSinceStartNS: 20,
 									StackID:             2,
 								},
+								{
+									ElapsedSinceStartNS: 30,
+									StackID:             2,
+								},
 							}, // end Samples
 						}, // end Trace
 					},
@@ -140,7 +144,7 @@ func TestFlamegraphAggregation(t *testing.T) {
 				},
 				Profiles: []interface{}{
 					speedscope.SampledProfile{
-						EndValue:     6,
+						EndValue:     7,
 						IsMainThread: true,
 						Samples: [][]int{
 							{2},
@@ -152,13 +156,13 @@ func TestFlamegraphAggregation(t *testing.T) {
 							{0},
 							{1},
 							{1},
-							{0},
+							{0, 1},
 						},
 						Type:              "sampled",
 						Unit:              "count",
-						Weights:           []uint64{1, 1, 1, 3},
-						SampleCounts:      []uint64{1, 1, 1, 3},
-						SampleDurationsNs: []uint64{10, 10, 10, 30},
+						Weights:           []uint64{1, 1, 1, 4},
+						SampleCounts:      []uint64{1, 1, 1, 4},
+						SampleDurationsNs: []uint64{10, 10, 10, 40},
 					},
 				},
 				Shared: speedscope.SharedData{
@@ -169,8 +173,8 @@ func TestFlamegraphAggregation(t *testing.T) {
 						{Image: "test.package", Name: "e", Fingerprint: 2430275448},
 					},
 					FrameInfos: []speedscope.FrameInfo{
+						{Count: 4, Weight: 50},
 						{Count: 3, Weight: 40},
-						{Count: 2, Weight: 30},
 						{Count: 2, Weight: 20},
 						{Count: 1, Weight: 10},
 					},
@@ -181,17 +185,62 @@ func TestFlamegraphAggregation(t *testing.T) {
 				},
 			},
 			metrics: []examples.FunctionMetrics{
+				{
+					Name:        "a",
+					Package:     "test.package",
+					Fingerprint: 2430275452,
+					P75:         10,
+					P95:         20,
+					P99:         20,
+					Avg:         float64(50) / float64(4),
+					Sum:         50,
+					SumSelfTime: 10,
+					Count:       5,
+					Worst:       examples.ExampleMetadata{ProfileID: "cd2"},
+					Examples:    []examples.ExampleMetadata{{ProfileID: "cd2"}},
+				},
 				{
 					Name:        "b",
 					Package:     "test.package",
 					Fingerprint: 2430275455,
 					P75:         20,
 					P95:         20,
 					P99:         20,
-					Avg:         15,
-					Sum:         30,
-					SumSelfTime: 30,
-					Count:       3,
+					Avg:         float64(40) / float64(3),
+					Sum:         40,
+					SumSelfTime: 40,
+					Count:       4,
+					Worst:       examples.ExampleMetadata{ProfileID: "ab1"},
+					Examples:    []examples.ExampleMetadata{{ProfileID: "ab1"}, {ProfileID: "cd2"}},
+				},
+				{
+					Name:        "c",
+					Package:     "test.package",
+					Fingerprint: 2430275454,
+					InApp:       true,
+					P75:         10,
+					P95:         10,
+					P99:         10,
+					Avg:         10,
+					Sum:         20,
+					SumSelfTime: 20,
+					Count:       2,
+					Worst:       examples.ExampleMetadata{ProfileID: "ab1"},
+					Examples:    []examples.ExampleMetadata{{ProfileID: "ab1"}},
+				},
+				{
+					Name:        "e",
+					Package:     "test.package",
+					Fingerprint: 2430275448,
+					P75:         10,
+					P95:         10,
+					P99:         10,
+					Avg:         10,
+					Sum:         10,
+					SumSelfTime: 10,
+					Count:       1,
+					Worst:       examples.ExampleMetadata{ProfileID: "cd2"},
+					Examples:    []examples.ExampleMetadata{{ProfileID: "cd2"}},
 				},
 			},
 		},
@@ -316,19 +365,6 @@ func TestFlamegraphAggregation(t *testing.T) {
 				},
 			},
 			metrics: []examples.FunctionMetrics{
-				{
-					Name:        "c",
-					Package:     "test.package",
-					Fingerprint: 2430275454,
-					InApp:       true,
-					P75:         30,
-					P95:         30,
-					P99:         30,
-					Avg:         25,
-					Sum:         50,
-					SumSelfTime: 50,
-					Count:       5,
-				},
 				{
 					Name:        "a",
 					Package:     "test.package",
@@ -341,6 +377,8 @@ func TestFlamegraphAggregation(t *testing.T) {
 					Sum:         90,
 					SumSelfTime: 20,
 					Count:       9,
+					Worst:       examples.ExampleMetadata{ProfileID: "ab1"},
+					Examples:    []examples.ExampleMetadata{{ProfileID: "ab1"}},
 				},
 				{
 					Name:        "b",
@@ -354,6 +392,23 @@ func TestFlamegraphAggregation(t *testing.T) {
 					Sum:         70,
 					SumSelfTime: 20,
 					Count:       7,
+					Worst:       examples.ExampleMetadata{ProfileID: "ab1"},
+					Examples:    []examples.ExampleMetadata{{ProfileID: "ab1"}},
+				},
+				{
+					Name:        "c",
+					Package:     "test.package",
+					Fingerprint: 2430275454,
+					InApp:       true,
+					P75:         30,
+					P95:         30,
+					P99:         30,
+					Avg:         25,
+					Sum:         50,
+					SumSelfTime: 50,
+					Count:       5,
+					Worst:       examples.ExampleMetadata{ProfileID: "ab1"},
+					Examples:    []examples.ExampleMetadata{{ProfileID: "ab1"}},
 				},
 			},
 		},
@@ -460,30 +515,34 @@ func TestFlamegraphAggregation(t *testing.T) {
 			},
 			metrics: []examples.FunctionMetrics{
 				{
-					Name:        "c",
+					Name:        "b",
 					Package:     "test.package",
-					Fingerprint: 2430275454,
+					Fingerprint: 2430275455,
 					InApp:       true,
 					P75:         30,
 					P95:         30,
 					P99:         30,
-					Avg:         30,
-					Sum:         30,
-					SumSelfTime: 30,
-					Count:       3,
+					Avg:         25,
+					Sum:         50,
+					SumSelfTime: 20,
+					Count:       5,
+					Worst:       examples.ExampleMetadata{ProfileID: "ab1"},
+					Examples:    []examples.ExampleMetadata{{ProfileID: "ab1"}},
 				},
 				{
-					Name:        "b",
+					Name:        "c",
 					Package:     "test.package",
-					Fingerprint: 2430275455,
+					Fingerprint: 2430275454,
 					InApp:       true,
 					P75:         30,
 					P95:         30,
 					P99:         30,
-					Avg:         25,
-					Sum:         50,
-					SumSelfTime: 20,
-					Count:       5,
+					Avg:         30,
+					Sum:         30,
+					SumSelfTime: 30,
+					Count:       3,
+					Worst:       examples.ExampleMetadata{ProfileID: "ab1"},
+					Examples:    []examples.ExampleMetadata{{ProfileID: "ab1"}},
 				},
 			},
 		},
@@ -492,6 +551,7 @@ func TestFlamegraphAggregation(t *testing.T) {
 	for _, test := range tests {
 		t.Run(test.name, func(t *testing.T) {
 			var ft []*nodetree.Node
+
 			for _, sp := range test.profiles {
 				p := profile.New(&sp)
 				callTrees, err := p.CallTrees()
@@ -508,44 +568,13 @@ func TestFlamegraphAggregation(t *testing.T) {
 				t.Fatalf("Result mismatch: got - want +\n%s", diff)
 			}
 
-			ma := metrics.NewAggregator(
-				100,
-				5,
-				0,
-			)
-
-			for _, sp := range test.profiles {
-				p := profile.New(&sp)
-				callTrees, err := p.CallTrees()
-				if err != nil {
-					t.Fatalf("error when generating calltrees: %v", err)
-				}
-
-				start, end := p.StartAndEndEpoch()
-				example := examples.NewExampleFromProfileID(
-					p.ProjectID(),
-					p.ID(),
-					start,
-					end,
-				)
-
-				functions := metrics.CapAndFilterFunctions(
-					metrics.ExtractFunctionsFromCallTrees(
-						callTrees,
-						ma.MinDepth,
-					),
-					int(ma.MaxUniqueFunctions),
-					false,
-				)
-				ma.AddFunctions(functions, example)
+			ma := metrics.NewAggregator(100, 5, 0, false)
+			for _, tree := range ft {
+				tree.Visit(ma.AddFunction)
 			}
 			m := ma.ToMetrics()
 
-			options := cmp.Options{
-				cmpopts.IgnoreFields(examples.FunctionMetrics{}, "Worst"),
-				cmpopts.IgnoreFields(examples.FunctionMetrics{}, "Examples"),
-			}
-			if diff := testutil.Diff(m, test.metrics, options); diff != "" {
+			if diff := testutil.Diff(m, test.metrics); diff != "" {
 				t.Fatalf("Result mismatch: got - want +\n%s", diff)
 			}
 		})