runtime/pprof: fix TestGoroutineProfileDebug3Creators on linux/amd64 - #18
Conversation
The debug=3 protobuf profile unconditionally emits go::goroutine_created_by from parentGoid, while debug=2 text output only prints "created by" when the creating function passes showframe. Allow debug=3 to be a superset of debug=2's created_by data instead of requiring an exact match. Fixes cockroachdb/cockroach#165528.
There was a problem hiding this comment.
Pull request overview
This PR updates the goroutine profile test expectations so that debug=3 protobuf output can include additional go::goroutine_created_by labels beyond what debug=2 text output reports (due to showframe filtering), and re-enables the previously skipped test.
Changes:
- Removed the unconditional skip for
TestGoroutineProfileDebug3Creators. - Relaxed the assertion so
debug=3may emitcreated_bylabels even whendebug=2did not.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if createdBy[id] == "" { | ||
| if len(got) != 0 { | ||
| t.Fatalf("goroutine %s: got created_by %q, want none", id, got) | ||
| } | ||
| // debug=3 may report created_by for goroutines whose | ||
| // creating function is filtered by showframe in debug=2 | ||
| // (e.g. runtime-internal goroutines). This is expected: | ||
| // debug=3 provides a superset of debug=2's created_by data. | ||
| continue | ||
| } else { | ||
| if e := createdBy[id]; len(got) != 1 || strconv.Itoa(int(got[0])) != e { | ||
| t.Fatalf("goroutine %s: got created_by %q, want %q", id, got, e) |
There was a problem hiding this comment.
if createdBy[id] == "" { continue } conflates “no entry in createdBy” with an (unexpected) empty creator value, which could hide parsing/format regressions. Consider using the map presence (e, ok := createdBy[id]) to decide whether to skip, and then compare against e.
If you want the test to more directly assert that debug=3 is a superset of debug=2, it’s also more robust to verify every createdBy entry exists in the parsed debug=3 samples (e.g., index samples by goroutine id and then iterate over createdBy), rather than iterating only over parsed.Sample.
There was a problem hiding this comment.
eh, I think treating empty as missing is perfectly fine for the test's purposes and a shorter, more concise test is easier to read.
tbg
left a comment
tbg
left a comment
There was a problem hiding this comment.
I'll let you decide on the fate of the Copilot comment.
The debug=3 protobuf profile unconditionally emits go::goroutine_created_by from parentGoid, while debug=2 text output only prints "created by" when the creating function passes showframe. Allow debug=3 to be a superset of debug=2's created_by data instead of requiring an exact match.