Merge pull request #43 from cockroachdb/20800812-report

report,redact: incrementally improve the ergonomics of reports

Author: knz

contexttags/contexttags_test.go

@@ -107,8 +107,8 @@ func TestTagRedaction(t *testing.T) {
 
 	// This will be our reference expected value.
 	refStrings := [][]string{
-		[]string{"planet1=<string>", "planet2=universe"},
-		[]string{"foo1=<int>", "x<int>", "bar1", "foo2=123", "y456", "bar2"},
+		[]string{"planet1=string:<redacted>", "planet2=universe"},
+		[]string{"foo1=int:<redacted>", "xint:<redacted>", "bar1", "foo2=123", "y456", "bar2"},
 	}
 
 	// Construct the error object.

report/report.go

@@ -269,7 +269,7 @@ func BuildSentryReport(err error) (event *sentry.Event, extraDetails map[string]
 			stKey := fmt.Sprintf("%d: details", extraNum)
 			var extraStr bytes.Buffer
 			for _, d := range details[i].SafeDetails {
-				fmt.Fprintln(&extraStr, d)
+				fmt.Fprintln(&extraStr, strings.ReplaceAll(d, "\n", "\n   "))
 			}
 			extras[stKey] = extraStr.String()
 			fmt.Fprintf(&longMsgBuf, " (%d)", extraNum)

report/report_test.go

@@ -53,7 +53,8 @@ func TestReport(t *testing.T) {
 	thisDomain := domains.NamedDomain("thisdomain")
 
 	err = goErr.New("hello")
-	err = safedetails.WithSafeDetails(err, "universe %d", safedetails.Safe(123))
+	err = safedetails.WithSafeDetails(err, "universe %d %s",
+		safedetails.Safe(123), safedetails.Safe("multi\nline"))
 	err = withstack.WithStack(err)
 	err = domains.WithDomain(err, thisDomain)
 	defer errbase.TestingWithEmptyMigrationRegistry()()
@@ -73,7 +74,7 @@ func TestReport(t *testing.T) {
 
 	tt.Run("long message payload", func(tt testutils.T) {
 		expectedLongMessage := `^\*errors.errorString
-\*safedetails.withSafeDetails: universe %d \(1\)
+\*safedetails.withSafeDetails: format: "universe %d %s" \(1\)
 report_test.go:\d+: \*withstack.withStack \(top exception\)
 \*domains\.withDomain: error domain: "thisdomain" \(2\)
 \*report_test\.myWrapper
@@ -91,7 +92,10 @@ github.com/cockroachdb/errors/report_test/*report_test.myWrapper (some/previous/
 		types := fmt.Sprintf("%s", e.Extra["error types"])
 		tt.CheckEqual(types, expectedTypes)
 
-		expectedDetail := "universe %d\n-- arg 1: 123"
+		expectedDetail := `format: "universe %d %s"
+-- arg 1: 123
+-- arg 2: multi
+   line`
 		detail := fmt.Sprintf("%s", e.Extra["1: details"])
 		tt.CheckEqual(strings.TrimSpace(detail), expectedDetail)
 

safedetails/redact.go

@@ -42,7 +42,7 @@ func Redact(r interface{}) string {
 		}
 		redactErr(&buf, t)
 	default:
-		typAnd(&buf, r, "")
+		typRedacted(&buf, r)
 	}
 
 	return buf.String()
@@ -60,7 +60,7 @@ func redactErr(buf *strings.Builder, err error) {
 
 	// Add any additional safe strings from the wrapper, if present.
 	if payload := errbase.GetSafeDetails(err); len(payload.SafeDetails) > 0 {
-		buf.WriteString("\n(more details:)")
+		buf.WriteString("\n(more details about this error:)")
 		for _, sd := range payload.SafeDetails {
 			buf.WriteByte('\n')
 			buf.WriteString(strings.TrimSpace(sd))
@@ -69,7 +69,7 @@ func redactErr(buf *strings.Builder, err error) {
 }
 
 func redactWrapper(buf *strings.Builder, err error) {
-	buf.WriteString("\nwrapper: ")
+	buf.WriteString("\n")
 	switch t := err.(type) {
 	case *os.SyscallError:
 		typAnd(buf, t, t.Syscall)
@@ -83,16 +83,16 @@ func redactWrapper(buf *strings.Builder, err error) {
 			fmt.Fprintf(buf, " %s", t.Net)
 		}
 		if t.Source != nil {
-			buf.WriteString("<redacted>")
+			buf.WriteString(" <redacted>")
 		}
 		if t.Addr != nil {
 			if t.Source != nil {
-				buf.WriteString("->")
+				buf.WriteString(" ->")
 			}
-			buf.WriteString("<redacted>")
+			buf.WriteString(" <redacted>")
 		}
 	default:
-		typAnd(buf, err, "")
+		typRedacted(buf, err)
 	}
 }
 
@@ -126,14 +126,17 @@ func redactLeafErr(buf *strings.Builder, err error) {
 		typAnd(buf, t, t.SafeMessage())
 	default:
 		// No further information about this error, simply report its type.
-		typAnd(buf, err, "")
+		typRedacted(buf, err)
 	}
 }
 
+func typRedacted(buf *strings.Builder, r interface{}) {
+	fmt.Fprintf(buf, "%T:<redacted>", r)
+}
+
 func typAnd(buf *strings.Builder, r interface{}, msg string) {
-	if msg == "" {
-		fmt.Fprintf(buf, "<%T>", r)
-	} else {
-		fmt.Fprintf(buf, "%T: %s", r, msg)
+	if len(msg) == 0 {
+		msg = "(empty string)"
 	}
+	fmt.Fprintf(buf, "%T: %s", r, msg)
 }

safedetails/redact_test.go

@@ -37,8 +37,8 @@ func TestRedact(t *testing.T) {
 	}{
 		// Redacting non-error values.
 
-		{123, `<int>`},
-		{"secret", `<string>`},
+		{123, `int:<redacted>`},
+		{"secret", `string:<redacted>`},
 
 		// Redacting SafeMessagers.
 
@@ -47,24 +47,24 @@ func TestRedact(t *testing.T) {
 		{mySafeError{}, `hello`},
 		{&werrFmt{mySafeError{}, "unseen"},
 			`safedetails_test.mySafeError: hello
-wrapper: <*safedetails_test.werrFmt>`},
+*safedetails_test.werrFmt:<redacted>`},
 
 		// Redacting errors.
 
 		// Unspecial cases, get redacted.
-		{errors.New("secret"), `<*errors.errorString>`},
+		{errors.New("secret"), `*errors.errorString:<redacted>`},
 
 		// Stack trace in error retrieves some info about the context.
 		{withstack.WithStack(errors.New("secret")),
-			`<path>: <*errors.errorString>
-wrapper: <*withstack.withStack>
-(more details:)
+			`...path...: *errors.errorString:<redacted>
+*withstack.withStack:<redacted>
+(more details about this error:)
 github.com/cockroachdb/errors/safedetails_test.TestRedact
-	<path>
+	...path...
 testing.tRunner
-	<path>
+	...path...
 runtime.goexit
-	<path>`},
+	...path...`},
 
 		// Special cases, unredacted.
 		{os.ErrInvalid, `*errors.errorString: invalid argument`},
@@ -83,22 +83,22 @@ runtime.goexit
 			`*runtime.TypeAssertionError: interface conversion: interface {} is nil, not int`},
 
 		{errSentinel, // explodes if Error() called
-			`<struct { error }>`},
+			`struct { error }:<redacted>`},
 
 		{&werrFmt{&werrFmt{os.ErrClosed, "unseen"}, "unsung"},
 			`*errors.errorString: file already closed
-wrapper: <*safedetails_test.werrFmt>
-wrapper: <*safedetails_test.werrFmt>`},
+*safedetails_test.werrFmt:<redacted>
+*safedetails_test.werrFmt:<redacted>`},
 
 		// Special cases, get partly redacted.
 
 		{os.NewSyscallError("rename", os.ErrNotExist),
 			`*errors.errorString: file does not exist
-wrapper: *os.SyscallError: rename`},
+*os.SyscallError: rename`},
 
 		{&os.PathError{Op: "rename", Path: "secret", Err: os.ErrNotExist},
 			`*errors.errorString: file does not exist
-wrapper: *os.PathError: rename`},
+*os.PathError: rename`},
 
 		{&os.LinkError{
 			Op:  "moo",
@@ -107,23 +107,23 @@ wrapper: *os.PathError: rename`},
 			Err: os.ErrNotExist,
 		},
 			`*errors.errorString: file does not exist
-wrapper: *os.LinkError: moo <redacted> <redacted>`},
+*os.LinkError: moo <redacted> <redacted>`},
 
 		{&net.OpError{
 			Op:     "write",
 			Net:    "tcp",
 			Source: &net.IPAddr{IP: net.IP("sensitive-source")},
 			Addr:   &net.IPAddr{IP: net.IP("sensitive-addr")},
 			Err:    errors.New("not safe"),
-		}, `<*errors.errorString>
-wrapper: *net.OpError: write tcp<redacted>-><redacted>`},
+		}, `*errors.errorString:<redacted>
+*net.OpError: write tcp <redacted> -> <redacted>`},
 	}
 
 	tt := testutils.T{T: t}
 
 	for _, tc := range testData {
 		s := safedetails.Redact(tc.obj)
-		s = fileref.ReplaceAllString(s, "<path>")
+		s = fileref.ReplaceAllString(s, "...path...")
 
 		tt.CheckStringEqual(s, tc.expected)
 	}

safedetails/safedetails.go

@@ -33,8 +33,18 @@ func WithSafeDetails(err error, format string, args ...interface{}) error {
 
 	details := make([]string, 1, 1+len(args))
 	details[0] = format
+	if len(format) > 0 && len(args) > 0 {
+		// Call out that this string was a formatting string.
+		details[0] = fmt.Sprintf("format: %q", details[0])
+	}
 	for i, a := range args {
-		details = append(details, fmt.Sprintf("-- arg %d: %s", i+1, Redact(a)))
+		what := ""
+		if _, ok := a.(error); ok {
+			// Call out that this argument was an error, to facilitate
+			// interpretation of subsequent lines.
+			what = " (error)"
+		}
+		details = append(details, fmt.Sprintf("-- arg %d%s: %s", i+1, what, Redact(a)))
 	}
 	return &withSafeDetails{cause: err, safeDetails: details}
 }