From d9f23cfe78eadcdbde31fd931e90bebb72455648 Mon Sep 17 00:00:00 2001 From: Austin Clements Date: Wed, 4 Jan 2023 12:31:23 -0500 Subject: [PATCH] runtime/pprof: improve output of TestLabelSystemstack MIME-Version: 1.0 Content-Type: text/plain; charset=utf8 Content-Transfer-Encoding: 8bit The current output of TestLabelSystemstack is a bit cryptic. This CL improves various messages and hopefully simplifies the logic in the test. Simplifying the logic leads to three changes in possible outcomes, which I verified by running the logic before and after this change through all 2^4 possibilities (https://go.dev/play/p/bnfb-OQCT4j): 1. If a sample both must be labeled and must not be labeled, the test now reports that explicitly rather than giving other confusing output. 2. If a sample must not be labeled but is, the current logic will print two identical error messages. The new logic prints only one. 3. If the test finds no frames at all that it recognizes, but the sample is labeled, it will currently print a confusing "Sample labeled got true want false" message. The new logic prints nothing. We've seen this triggered by empty stacks in profiles. Fixes #51550. This bug was caused by case 3 above, where it was triggered by a profile label on an empty stack. It's valid for empty stacks to appear in a profile if we sample a goroutine just as it's exiting (and that goroutine may have a profile label), so the test shouldn't fail in this case. Change-Id: I1593ec4ac33eced5bb89572a3ba7623e56f2fb3d Reviewed-on: https://go-review.googlesource.com/c/go/+/460516 Run-TryBot: Austin Clements Reviewed-by: Felix Geisendörfer Reviewed-by: Michael Pratt TryBot-Result: Gopher Robot --- src/runtime/pprof/pprof_test.go | 46 +++++++++++++++++++-------------- 1 file changed, 26 insertions(+), 20 deletions(-) diff --git a/src/runtime/pprof/pprof_test.go b/src/runtime/pprof/pprof_test.go index 39bc9d11f1..53688ad825 100644 --- a/src/runtime/pprof/pprof_test.go +++ b/src/runtime/pprof/pprof_test.go @@ -607,7 +607,7 @@ func matchAndAvoidStacks(matches sampleMatchFunc, need []string, avoid []string) var total uintptr for i, name := range need { total += have[i] - t.Logf("%s: %d\n", name, have[i]) + t.Logf("found %d samples in expected function %s\n", have[i], name) } if total == 0 { t.Logf("no samples in expected functions") @@ -727,6 +727,9 @@ func TestGoroutineSwitch(t *testing.T) { } func fprintStack(w io.Writer, stk []*profile.Location) { + if len(stk) == 0 { + fmt.Fprintf(w, " (stack empty)") + } for _, loc := range stk { fmt.Fprintf(w, " %#x", loc.Address) fmt.Fprintf(w, " (") @@ -1867,14 +1870,14 @@ func TestLabelSystemstack(t *testing.T) { isLabeled := s.Label != nil && contains(s.Label["key"], "value") var ( mayBeLabeled bool - mustBeLabeled bool - mustNotBeLabeled bool + mustBeLabeled string + mustNotBeLabeled string ) for _, loc := range s.Location { for _, l := range loc.Line { switch l.Function.Name { case "runtime/pprof.labelHog", "runtime/pprof.parallelLabelHog", "runtime/pprof.parallelLabelHog.func1": - mustBeLabeled = true + mustBeLabeled = l.Function.Name case "runtime/pprof.Do": // Do sets the labels, so samples may // or may not be labeled depending on @@ -1886,7 +1889,7 @@ func TestLabelSystemstack(t *testing.T) { // (such as those identified by // runtime.isSystemGoroutine). These // should never be labeled. - mustNotBeLabeled = true + mustNotBeLabeled = l.Function.Name case "gogo", "gosave_systemstack_switch", "racecall": // These are context switch/race // critical that we can't do a full @@ -1908,25 +1911,28 @@ func TestLabelSystemstack(t *testing.T) { } } } - if mustNotBeLabeled { - // If this must not be labeled, then mayBeLabeled hints - // are not relevant. - mayBeLabeled = false - } - if mustBeLabeled && !isLabeled { + errorStack := func(f string, args ...any) { var buf strings.Builder fprintStack(&buf, s.Location) - t.Errorf("Sample labeled got false want true: %s", buf.String()) + t.Errorf("%s: %s", fmt.Sprintf(f, args...), buf.String()) } - if mustNotBeLabeled && isLabeled { - var buf strings.Builder - fprintStack(&buf, s.Location) - t.Errorf("Sample labeled got true want false: %s", buf.String()) + if mustBeLabeled != "" && mustNotBeLabeled != "" { + errorStack("sample contains both %s, which must be labeled, and %s, which must not be labeled", mustBeLabeled, mustNotBeLabeled) + continue } - if isLabeled && !(mayBeLabeled || mustBeLabeled) { - var buf strings.Builder - fprintStack(&buf, s.Location) - t.Errorf("Sample labeled got true want false: %s", buf.String()) + if mustBeLabeled != "" || mustNotBeLabeled != "" { + // We found a definitive frame, so mayBeLabeled hints are not relevant. + mayBeLabeled = false + } + if mayBeLabeled { + // This sample may or may not be labeled, so there's nothing we can check. + continue + } + if mustBeLabeled != "" && !isLabeled { + errorStack("sample must be labeled because of %s, but is not", mustBeLabeled) + } + if mustNotBeLabeled != "" && isLabeled { + errorStack("sample must not be labeled because of %s, but is", mustNotBeLabeled) } } } -- 2.48.1