runtime: simplify pprof labels in tracebacks

Per discussion on #76349, move the traceback labels outside the
goroutine status block and remove the quoting if the key and value
strings are completely ASCII alphanumeric.

Also allow [._/] because those are generally benign and may show up in
a lot of use-cases if these goroutine labels become more visible.

Updates #76349

Change-Id: I338e18d7ca48bbc7504f7c699f17adade2d291f9
Reviewed-on: https://go-review.googlesource.com/c/go/+/742580
Reviewed-by: Michael Pratt <mpratt@google.com>
Reviewed-by: Florian Lehner <lehner.florian86@gmail.com>
Auto-Submit: Michael Pratt <mpratt@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Alan Donovan <adonovan@google.com>

2 files changed, 37 insertions, 10 deletions

diff --git a/src/runtime/traceback.go b/src/runtime/traceback.go
index 4fe0fd89e2..efea4d87b2 100644
--- a/src/runtime/traceback.go
+++ b/src/runtime/traceback.go
@@ -1271,12 +1271,23 @@ func goroutineheader(gp *g) {
 	if bubble := gp.bubble; bubble != nil {
 		print(", synctest bubble ", bubble.id)
 	}
+	print("]")
 	if gp.labels != nil && debug.tracebacklabels.Load() == 1 {
 		labels := (*label.Set)(gp.labels).List
 		if len(labels) > 0 {
-			print(" labels:{")
+			print(" {")
 			for i, kv := range labels {
-				print(quoted(kv.Key), ": ", quoted(kv.Value))
+				// Try to be nice and only quote the keys/values if one of them has characters that need quoting or escaping.
+				printq := func(s string) {
+					if tracebackStringNeedsQuoting(s) {
+						print(quoted(s))
+					} else {
+						print(s)
+					}
+				}
+				printq(kv.Key)
+				print(": ")
+				printq(kv.Value)
 				if i < len(labels)-1 {
 					print(", ")
 				}
@@ -1284,7 +1295,19 @@ func goroutineheader(gp *g) {
 			print("}")
 		}
 	}
-	print("]:\n")
+	print(":\n")
+}
+
+func tracebackStringNeedsQuoting(s string) bool {
+	for _, r := range s {
+		if !('a' <= r && r <= 'z' ||
+			'A' <= r && r <= 'Z' ||
+			'0' <= r && r <= '9' ||
+			r == '.' || r == '/' || r == '_') {
+			return true
+		}
+	}
+	return false
 }
 
 func tracebackothers(me *g) {
diff --git a/src/runtime/traceback_test.go b/src/runtime/traceback_test.go
index d47f4ab745..9662b2cd5a 100644
--- a/src/runtime/traceback_test.go
+++ b/src/runtime/traceback_test.go
@@ -891,15 +891,19 @@ func TestTracebackGoroutineLabels(t *testing.T) {
 		l     pprof.LabelSet
 		expTB string
 	}{
-		{l: pprof.Labels("foobar", "baz"), expTB: `{"foobar": "baz"}`},
+		{l: pprof.Labels("foobar", "baz"), expTB: `{foobar: baz}`},
 		// Make sure the keys are sorted because the runtime/pprof package sorts for consistency
-		{l: pprof.Labels("foobar", "baz", "fizzle", "bit"), expTB: `{"fizzle": "bit", "foobar": "baz"}`},
+		{l: pprof.Labels("foobar", "baz", "fizzle", "bit"), expTB: `{fizzle: bit, foobar: baz}`},
+		// allow [./_] as well without quoting
+		{l: pprof.Labels("foo_bar", "baz.", "/fizzle", "bit"), expTB: `{/fizzle: bit, foo_bar: baz.}`},
+		// Make sure the keys & values get quoted if there's a non-alnum character
+		{l: pprof.Labels("foobar:", "baz", "fizzle", "bit"), expTB: `{fizzle: bit, "foobar:": baz}`},
 		// make sure newlines get escaped
-		{l: pprof.Labels("fizzle", "bit", "foobar", "baz\n"), expTB: `{"fizzle": "bit", "foobar": "baz\n"}`},
+		{l: pprof.Labels("fizzle", "bit", "foobar", "baz\n"), expTB: `{fizzle: bit, foobar: "baz\n"}`},
 		// make sure null and escape bytes are properly escaped
-		{l: pprof.Labels("fizzle", "b\033it", "foo\"ba\x00r", "baz\n"), expTB: `{"fizzle": "b\x1bit", "foo\"ba\x00r": "baz\n"}`},
+		{l: pprof.Labels("fizzle", "b\033it", "foo\"ba\x00r", "baz\n"), expTB: `{fizzle: "b\x1bit", "foo\"ba\x00r": "baz\n"}`},
 		// verify that simple 16-bit unicode runes are escaped with \u, including a greek upper-case sigma and an arbitrary unicode character.
-		{l: pprof.Labels("fizzle", "\u1234Σ", "fooba\x00r", "baz\n"), expTB: `{"fizzle": "\u1234\u03a3", "fooba\x00r": "baz\n"}`},
+		{l: pprof.Labels("fizzle", "\u1234Σ", "fooba\x00r", "baz\n"), expTB: `{fizzle: "\u1234\u03a3", "fooba\x00r": "baz\n"}`},
 		// verify that 32-bit unicode runes are escaped with \U along with tabs
 		{l: pprof.Labels("fizz\tle", "\U00045678boop", "fooba\x00r", "baz\n"), expTB: `{"fizz\tle": "\U00045678boop", "fooba\x00r": "baz\n"}`},
 		// verify carriage returns and backslashes get escaped along with our nulls, newlines and a 32-bit unicode character
@@ -912,7 +916,7 @@ func TestTracebackGoroutineLabels(t *testing.T) {
 				// We collect the stack only for this goroutine (by passing
 				// false to runtime.Stack). We expect to see the parent's goroutine labels in the traceback.
 				stack := string(buf[:runtime.Stack(buf, false)])
-				if !strings.Contains(stack, "labels:"+tbl.expTB) {
+				if !strings.Contains(stack, tbl.expTB+":") {
 					t.Errorf("failed to find goroutine labels with labels %s (as %s) got:\n%s\n---", tbl.l, tbl.expTB, stack)
 				}
 			}
@@ -938,7 +942,7 @@ func TestTracebackGoroutineLabelsDisabledGODEBUG(t *testing.T) {
 		// We collect the stack only for this goroutine (by passing
 		// false to runtime.Stack).
 		stack := string(buf[:runtime.Stack(buf, false)])
-		if strings.Contains(stack, "labels:") {
+		if strings.Contains(stack, " {foobar: baz}:") {
 			t.Errorf("found goroutine labels with labels %s  got:\n%s\n---", lbls, stack)
 		}
 	}