Fix flaky 'TestAggregateTopByFunction`

[importhuman]

This sorts the list by the meta location address, so the flaky TestAggregateTopByFunction passes every time. (#770)

I ran the test multiple times without any changes, and encountered 2 types of failures: UniqueFunction and UniqueAddress.

Logs for failing `UniqueFunction` test

Running tool: /usr/local/go/bin/go test -timeout 30s -run ^TestAggregateTopByFunction$ github.com/parca-dev/parca/pkg/query

--- FAIL: TestAggregateTopByFunction (0.00s)
--- FAIL: TestAggregateTopByFunction/UniqueFunction (0.00s)
/home/ujjwal/oss/parca-dev/parca/pkg/query/top_test.go:300:
Error Trace: top_test.go:300
Error: Not equal:
expected: &queryv1alpha1.Top{state:impl.MessageState{NoUnkeyedLiterals:pragma.NoUnkeyedLiterals{}, DoNotCompare:pragma.DoNotCompare{}, DoNotCopy:pragma.DoNotCopy{}, atomicMessageInfo:(*impl.MessageInfo)(nil)}, sizeCache:0, unknownFields:[]uint8(nil), List:[]*queryv1alpha1.TopNode{(*queryv1alpha1.TopNode)(0xc00012a280), (*queryv1alpha1.TopNode)(0xc00012a2c0)}, Reported:2, Total:2, Unit:""}
actual : &queryv1alpha1.Top{state:impl.MessageState{NoUnkeyedLiterals:pragma.NoUnkeyedLiterals{}, DoNotCompare:pragma.DoNotCompare{}, DoNotCopy:pragma.DoNotCopy{}, atomicMessageInfo:(*impl.MessageInfo)(nil)}, sizeCache:0, unknownFields:[]uint8(nil), List:[]*queryv1alpha1.TopNode{(*queryv1alpha1.TopNode)(0xc00012a240), (*queryv1alpha1.TopNode)(0xc00012a200)}, Reported:2, Total:2, Unit:""}

        	            	Diff:
        	            	--- Expected
        	            	+++ Actual
        	            	@@ -50,5 +50,5 @@
        	            	      Id: ([]uint8) (len=16) {
        	            	-      00000000  00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 02  |................|
        	            	-     },
        	            	-     Address: (uint64) 2,
        	            	+      00000000  00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 03  |................|
        	            	+     },
        	            	+     Address: (uint64) 3,
        	            	      MappingId: ([]uint8) <nil>,
        	            	@@ -94,6 +94,6 @@
        	            	      Id: ([]uint8) (len=16) {
        	            	-      00000000  00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 02  |................|
        	            	+      00000000  00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 03  |................|
        	            	      },
        	            	      StartLine: (int64) 0,
        	            	-     Name: (string) (len=5) "func2",
        	            	+     Name: (string) (len=5) "func3",
        	            	      SystemName: (string) "",
        	            	@@ -143,5 +143,5 @@
        	            	      Id: ([]uint8) (len=16) {
        	            	-      00000000  00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 03  |................|
        	            	-     },
        	            	-     Address: (uint64) 3,
        	            	+      00000000  00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 02  |................|
        	            	+     },
        	            	+     Address: (uint64) 2,
        	            	      MappingId: ([]uint8) <nil>,
        	            	@@ -187,6 +187,6 @@
        	            	      Id: ([]uint8) (len=16) {
        	            	-      00000000  00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 03  |................|
        	            	+      00000000  00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 02  |................|
        	            	      },
        	            	      StartLine: (int64) 0,
        	            	-     Name: (string) (len=5) "func3",
        	            	+     Name: (string) (len=5) "func2",
        	            	      SystemName: (string) "",
        	Test:       	TestAggregateTopByFunction/UniqueFunction

FAIL
FAIL github.com/parca-dev/parca/pkg/query 0.008s
FAIL

Logs for failing `UniqueAddress` test

Running tool: /usr/local/go/bin/go test -timeout 30s -run ^TestAggregateTopByFunction$ github.com/parca-dev/parca/pkg/query

--- FAIL: TestAggregateTopByFunction (0.00s)
--- FAIL: TestAggregateTopByFunction/UniqueAddress (0.00s)
/home/ujjwal/oss/parca-dev/parca/pkg/query/top_test.go:300:
Error Trace: top_test.go:300
Error: Not equal:
expected: &queryv1alpha1.Top{state:impl.MessageState{NoUnkeyedLiterals:pragma.NoUnkeyedLiterals{}, DoNotCompare:pragma.DoNotCompare{}, DoNotCopy:pragma.DoNotCopy{}, atomicMessageInfo:(*impl.MessageInfo)(nil)}, sizeCache:0, unknownFields:[]uint8(nil), List:[]*queryv1alpha1.TopNode{(*queryv1alpha1.TopNode)(0xc0004b6180), (*queryv1alpha1.TopNode)(0xc0004b61c0)}, Reported:2, Total:2, Unit:""}
actual : &queryv1alpha1.Top{state:impl.MessageState{NoUnkeyedLiterals:pragma.NoUnkeyedLiterals{}, DoNotCompare:pragma.DoNotCompare{}, DoNotCopy:pragma.DoNotCopy{}, atomicMessageInfo:(*impl.MessageInfo)(nil)}, sizeCache:0, unknownFields:[]uint8(nil), List:[]*queryv1alpha1.TopNode{(*queryv1alpha1.TopNode)(0xc0004b6140), (*queryv1alpha1.TopNode)(0xc0004b6100)}, Reported:2, Total:2, Unit:""}

        	            	Diff:
        	            	--- Expected
        	            	+++ Actual
        	            	@@ -13,2 +13,76 @@
        	            	  List: ([]*queryv1alpha1.TopNode) (len=2) {
        	            	+  (*queryv1alpha1.TopNode)({
        	            	+   state: (impl.MessageState) {
        	            	+    NoUnkeyedLiterals: (pragma.NoUnkeyedLiterals) {
        	            	+    },
        	            	+    DoNotCompare: (pragma.DoNotCompare) {
        	            	+    },
        	            	+    DoNotCopy: (pragma.DoNotCopy) {
        	            	+    },
        	            	+    atomicMessageInfo: (*impl.MessageInfo)(<nil>)
        	            	+   },
        	            	+   sizeCache: (int32) 0,
        	            	+   unknownFields: ([]uint8) <nil>,
        	            	+   Meta: (*queryv1alpha1.TopNodeMeta)({
        	            	+    state: (impl.MessageState) {
        	            	+     NoUnkeyedLiterals: (pragma.NoUnkeyedLiterals) {
        	            	+     },
        	            	+     DoNotCompare: (pragma.DoNotCompare) {
        	            	+     },
        	            	+     DoNotCopy: (pragma.DoNotCopy) {
        	            	+     },
        	            	+     atomicMessageInfo: (*impl.MessageInfo)(<nil>)
        	            	+    },
        	            	+    sizeCache: (int32) 0,
        	            	+    unknownFields: ([]uint8) <nil>,
        	            	+    Location: (*metastorev1alpha1.Location)({
        	            	+     state: (impl.MessageState) {
        	            	+      NoUnkeyedLiterals: (pragma.NoUnkeyedLiterals) {
        	            	+      },
        	            	+      DoNotCompare: (pragma.DoNotCompare) {
        	            	+      },
        	            	+      DoNotCopy: (pragma.DoNotCopy) {
        	            	+      },
        	            	+      atomicMessageInfo: (*impl.MessageInfo)(<nil>)
        	            	+     },
        	            	+     sizeCache: (int32) 0,
        	            	+     unknownFields: ([]uint8) <nil>,
        	            	+     Id: ([]uint8) (len=16) {
        	            	+      00000000  00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 03  |................|
        	            	+     },
        	            	+     Address: (uint64) 3,
        	            	+     MappingId: ([]uint8) <nil>,
        	            	+     IsFolded: (bool) false
        	            	+    }),
        	            	+    Mapping: (*metastorev1alpha1.Mapping)({
        	            	+     state: (impl.MessageState) {
        	            	+      NoUnkeyedLiterals: (pragma.NoUnkeyedLiterals) {
        	            	+      },
        	            	+      DoNotCompare: (pragma.DoNotCompare) {
        	            	+      },
        	            	+      DoNotCopy: (pragma.DoNotCopy) {
        	            	+      },
        	            	+      atomicMessageInfo: (*impl.MessageInfo)(<nil>)
        	            	+     },
        	            	+     sizeCache: (int32) 0,
        	            	+     unknownFields: ([]uint8) <nil>,
        	            	+     Id: ([]uint8) (len=16) {
        	            	+      00000000  00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 01  |................|
        	            	+     },
        	            	+     Start: (uint64) 0,
        	            	+     Limit: (uint64) 0,
        	            	+     Offset: (uint64) 0,
        	            	+     File: (string) "",
        	            	+     BuildId: (string) "",
        	            	+     HasFunctions: (bool) false,
        	            	+     HasFilenames: (bool) false,
        	            	+     HasLineNumbers: (bool) false,
        	            	+     HasInlineFrames: (bool) false
        	            	+    }),
        	            	+    Function: (*metastorev1alpha1.Function)(<nil>),
        	            	+    Line: (*metastorev1alpha1.Line)(<nil>)
        	            	+   }),
        	            	+   Cumulative: (int64) 1,
        	            	+   Flat: (int64) 1
        	            	+  }),
        	            	   (*queryv1alpha1.TopNode)({
        	            	@@ -86,76 +160,2 @@
        	            	    Flat: (int64) 1
        	            	-  }),
        	            	-  (*queryv1alpha1.TopNode)({
        	            	-   state: (impl.MessageState) {
        	            	-    NoUnkeyedLiterals: (pragma.NoUnkeyedLiterals) {
        	            	-    },
        	            	-    DoNotCompare: (pragma.DoNotCompare) {
        	            	-    },
        	            	-    DoNotCopy: (pragma.DoNotCopy) {
        	            	-    },
        	            	-    atomicMessageInfo: (*impl.MessageInfo)(<nil>)
        	            	-   },
        	            	-   sizeCache: (int32) 0,
        	            	-   unknownFields: ([]uint8) <nil>,
        	            	-   Meta: (*queryv1alpha1.TopNodeMeta)({
        	            	-    state: (impl.MessageState) {
        	            	-     NoUnkeyedLiterals: (pragma.NoUnkeyedLiterals) {
        	            	-     },
        	            	-     DoNotCompare: (pragma.DoNotCompare) {
        	            	-     },
        	            	-     DoNotCopy: (pragma.DoNotCopy) {
        	            	-     },
        	            	-     atomicMessageInfo: (*impl.MessageInfo)(<nil>)
        	            	-    },
        	            	-    sizeCache: (int32) 0,
        	            	-    unknownFields: ([]uint8) <nil>,
        	            	-    Location: (*metastorev1alpha1.Location)({
        	            	-     state: (impl.MessageState) {
        	            	-      NoUnkeyedLiterals: (pragma.NoUnkeyedLiterals) {
        	            	-      },
        	            	-      DoNotCompare: (pragma.DoNotCompare) {
        	            	-      },
        	            	-      DoNotCopy: (pragma.DoNotCopy) {
        	            	-      },
        	            	-      atomicMessageInfo: (*impl.MessageInfo)(<nil>)
        	            	-     },
        	            	-     sizeCache: (int32) 0,
        	            	-     unknownFields: ([]uint8) <nil>,
        	            	-     Id: ([]uint8) (len=16) {
        	            	-      00000000  00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 03  |................|
        	            	-     },
        	            	-     Address: (uint64) 3,
        	            	-     MappingId: ([]uint8) <nil>,
        	            	-     IsFolded: (bool) false
        	            	-    }),
        	            	-    Mapping: (*metastorev1alpha1.Mapping)({
        	            	-     state: (impl.MessageState) {
        	            	-      NoUnkeyedLiterals: (pragma.NoUnkeyedLiterals) {
        	            	-      },
        	            	-      DoNotCompare: (pragma.DoNotCompare) {
        	            	-      },
        	            	-      DoNotCopy: (pragma.DoNotCopy) {
        	            	-      },
        	            	-      atomicMessageInfo: (*impl.MessageInfo)(<nil>)
        	            	-     },
        	            	-     sizeCache: (int32) 0,
        	            	-     unknownFields: ([]uint8) <nil>,
        	            	-     Id: ([]uint8) (len=16) {
        	            	-      00000000  00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 01  |................|
        	            	-     },
        	            	-     Start: (uint64) 0,
        	            	-     Limit: (uint64) 0,
        	            	-     Offset: (uint64) 0,
        	            	-     File: (string) "",
        	            	-     BuildId: (string) "",
        	            	-     HasFunctions: (bool) false,
        	            	-     HasFilenames: (bool) false,
        	            	-     HasLineNumbers: (bool) false,
        	            	-     HasInlineFrames: (bool) false
        	            	-    }),
        	            	-    Function: (*metastorev1alpha1.Function)(<nil>),
        	            	-    Line: (*metastorev1alpha1.Line)(<nil>)
        	            	-   }),
        	            	-   Cumulative: (int64) 1,
        	            	-   Flat: (int64) 1
        	            	   })
        	Test:       	TestAggregateTopByFunction/UniqueAddress

FAIL
FAIL github.com/parca-dev/parca/pkg/query 0.008s
FAIL

As had been suggested in the comments of the issue, I first sorted by Flat value for each TopNode in the List, but eventually I encountered the errors. I then added a second sort condition to sort based on the meta location address, as follows:

sort.Slice(list, func(i, j int) bool {
	// If flat value is equal, sort by address, ascending
	if list[i].Flat == list[j].Flat {
		return list[i].Meta.Location.Address < list[j].Meta.Location.Address
	}

	// Sort by flat value, descending
	return list[i].Flat > list[j].Flat
})

I ran the TestAggregateTopByFunction about 20 times (clearing testcache for each run) to ensure this worked. I didn't encounter any errors, and I decided to try only sorting by the address (the current code). I ran it 20 times again, and still didn't find any errors.

If this is only required for tests, we could add the sorting to only the test instead of changing the function. If the sorting is required for regular usage as well, however, I would suggest that we use a more detailed sort:

• If flat value is equal, sort by meta location address
• If meta location address is also equal, sort by meta function id/name
• Else, sort by flat value

Please let me know which approach to take.

[metalmatze]

Verrry nice work!
I'm wondering if we can only have the sorting stable in the test without always sorting the real data later on. It seems the order doesn't really matter, so it might end up being busy work for most other cases.

[brancz]

Until the database is able to do the sorting for us I would say this type of sorting is fine, but I think the first thing I almost always do is sort by descending by flat value (so the highest value is at the top). I would say let's make that the default sorting here.

[importhuman]

Sure, sorting only in tests though, correct?

[brancz]

No, I think it should be in the actual API as I want a consistent result as a user as well.

[metalmatze]

Alright, I wasn't sure if that's really necessary. In that case, we can go ahead with this change.
Thank you so much @importhuman! 🎉

[brancz]

I think that merge was a bit hasty since this PR sorted by address, but I think we should have sorted descending by flat value.

[metalmatze]

Oops. Sorry. Do you want to follow up with that @importhuman? Otherwise, I can take a look at it in the next few days.

[importhuman]

No worries, I'll raise a PR.