cli/debug: narrow scope of --include-goroutine-stacks to just debug=2

Release note: none.
Epic: none.

Author: dt

pkg/cli/cliflags/flags.go

@@ -1825,12 +1825,13 @@ Labs support.
 	ZipIncludeGoroutineStacks = FlagInfo{
 		Name: "include-goroutine-stacks",
 		Description: `
-Fetch stack traces for all goroutines running on each targeted node in nodes/*/stacks.txt
-and nodes/*/stacks_with_labels.txt files. Note that fetching stack traces for all goroutines is
-a "stop-the-world" operation, which can momentarily have negative impacts on SQL service
-latency. Note that any periodic goroutine dumps previously taken on the node will still be
-included in nodes/*/goroutines/*.txt.gz, as these would have already been generated and don't
-require any additional stop-the-world operations to be collected.
+Fetch full stack traces for all goroutines running on each targeted node in nodes/*/stacks.txt files.
+Note that fetching text stack traces for all goroutines incurs a brief "stop-the-world" pause of each
+node which can momentarily have negative impacts on SQL service latency. This flag only controls
+collection of new full dump of all current goroutine stacks -- any previously recorded, periodic
+goroutine dumps retained in the logs directories are still included (in nodes/*/goroutines/*.txt.gz)
+and collection of aggregate counts of current goroutine stacks -- which does not incur a stop-the-world
+pause -- remains enabled regardless of this flag's value.
 `,
 	}
 

pkg/cli/context.go

@@ -359,10 +359,9 @@ type zipContext struct {
 	includeRangeInfo bool
 
 	// includeStacks fetches all goroutines running on each targeted node in
-	// nodes/*/stacks.txt and nodes/*/stacks_with_labels.txt files. Note that
-	// fetching stack traces for all goroutines is a temporary "stop the world"
-	// operation, which can momentarily have negative impacts on SQL service
-	// latency.
+	// nodes/*/stacks.txt. Note that fetching debug=2 stack traces for all
+	// goroutines incurs a temporary "stop the world" operation, which can
+	// momentarily have negative impacts on SQL service latency.
 	includeStacks bool
 
 	// includeRunningJobTraces includes the active traces of each running

pkg/cli/testdata/zip/testzip_exclude_goroutine_stacks

@@ -25,6 +25,7 @@ debug zip --concurrency=1 --cpu-profile-duration=1s --validate-zip-file=false --
 [node 1] requesting data for debug/nodes/1/details... received response... writing JSON output: debug/nodes/1/details.json... done
 [node 1] requesting data for debug/nodes/1/gossip... received response... writing JSON output: debug/nodes/1/gossip.json... done
 [node 1] Skipping fetching goroutine stacks. Enable via the --include-goroutine-stacks flag.
+[node 1] requesting stacks with labels... received response... writing binary output: debug/nodes/1/stacks_with_labels.txt... done
 [node 1] requesting heap profile... received response... writing binary output: debug/nodes/1/heap.pprof... done
 [node 1] requesting engine stats... received response... writing binary output: debug/nodes/1/lsm.txt... done
 [node 1] requesting heap profile list... received response... done

pkg/cli/zip_per_node.go

@@ -579,6 +579,12 @@ func (zc *debugZipContext) getCurrentHeapProfile(
 func (zc *debugZipContext) getStackInformation(
 	ctx context.Context, nodePrinter *zipReporter, id string, prefix string,
 ) error {
+	// zipCtx.includeStacks controls specifically the inclusion of stacks.txt, as
+	// the debug=2 collection scheme has a "stop the world" implementation that
+	// makes it potentially disruptive and thus has an option to skip it. This
+	// option does _not_ need to skip collection of stack groups (debug=1) or the
+	// binary goroutine profile (debug=3 or debug=0), as these do not have that
+	// same stop-the-world disruption.
 	if zipCtx.includeStacks {
 		if zipCtx.files.shouldIncludeFile(stacksFileName) {
 			var stacksData []byte
@@ -600,32 +606,32 @@ func (zc *debugZipContext) getStackInformation(
 		} else {
 			nodePrinter.info("skipping %s due to file filters", stacksFileName)
 		}
+	} else {
+		nodePrinter.info("Skipping fetching goroutine stacks. Enable via the --%s flag.", cliflags.ZipIncludeGoroutineStacks.Name)
+	}
 
-		var stacksDataWithLabels []byte
-		if zipCtx.files.shouldIncludeFile(stacksWithLabelFileName) {
-			s := nodePrinter.start("requesting stacks with labels")
-			requestErr := zc.runZipFn(ctx, s,
-				func(ctx context.Context) error {
-					stacks, err := zc.status.Stacks(ctx, &serverpb.StacksRequest{
-						NodeId: id,
-						Type:   serverpb.StacksType_GOROUTINE_STACKS_DEBUG_1,
-					})
-					if err == nil {
-						stacksDataWithLabels = stacks.Data
-					}
-					return err
+	var stacksDataWithLabels []byte
+	if zipCtx.files.shouldIncludeFile(stacksWithLabelFileName) {
+		s := nodePrinter.start("requesting stacks with labels")
+		requestErr := zc.runZipFn(ctx, s,
+			func(ctx context.Context) error {
+				stacks, err := zc.status.Stacks(ctx, &serverpb.StacksRequest{
+					NodeId: id,
+					Type:   serverpb.StacksType_GOROUTINE_STACKS_DEBUG_1,
 				})
-			if zipCtx.redact {
-				stacksDataWithLabels = redactStackTrace(stacksDataWithLabels)
-			}
-			if err := zc.z.createRawOrError(s, prefix+"/"+stacksWithLabelFileName, stacksDataWithLabels, requestErr); err != nil {
+				if err == nil {
+					stacksDataWithLabels = stacks.Data
+				}
 				return err
-			}
-		} else {
-			nodePrinter.info("skipping %s due to file filters", stacksWithLabelFileName)
+			})
+		if zipCtx.redact {
+			stacksDataWithLabels = redactStackTrace(stacksDataWithLabels)
+		}
+		if err := zc.z.createRawOrError(s, prefix+"/"+stacksWithLabelFileName, stacksDataWithLabels, requestErr); err != nil {
+			return err
 		}
 	} else {
-		nodePrinter.info("Skipping fetching goroutine stacks. Enable via the --%s flag.", cliflags.ZipIncludeGoroutineStacks.Name)
+		nodePrinter.info("skipping %s due to file filters", stacksWithLabelFileName)
 	}
 	return nil
 }