Repository: yarpc/yab
Pull request #403 (2023-02-23 15:12:08)

Output errors in JSON format
So far, the errors encountered during a YAB benchmark run would be printed as text and not json, when format is json. This PR fixes that.

I'm rearranging the error printing a bit to align it with the other info printed. Lastly, I've added a test that verifies the output when errors are encountered (something that was not tested before).


CLAassistant: [![CLA assistant check](https://cla-assistant.io/pull/badge/not_signed)](https://cla-assistant.io/yarpc/yab?pullRequest=403) <br/>Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our [Contributor License Agreement](https://cla-assistant.io/yarpc/yab?pullRequest=403) before we can accept your contribution.<br/><sub>You have signed the CLA already but the status is still pending? Let us [recheck](https://cla-assistant.io/check/yarpc/yab?pullRequest=403) it.</sub>

Patch:
diff --git a/bench_state.go b/bench_state.go
index f795ead..b727b16 100644
--- a/bench_state.go
+++ b/bench_state.go
@@ -25,7 +25,6 @@ import (
 	"sort"
 	"time"
 
-	"github.com/yarpc/yab/sorted"
 	"github.com/yarpc/yab/statsd"
 )
 
@@ -99,17 +98,20 @@ func (s *benchmarkState) getLatencies() map[float64]time.Duration {
 	return latencyValues
 }
 
-func (s *benchmarkState) printErrors(out output) {
+func (s *benchmarkState) getErrorSummary() *ErrorSummary {
 	if len(s.errors) == 0 {
-		return
+		return nil
 	}
-	out.Printf("Errors:\n")
-	for _, k := range sorted.MapKeys(s.errors) {
-		v := s.errors[k]
-		out.Printf("  %4d: %v\n", v, k)
+	sum := &ErrorSummary{
+		TotalErrors: s.totalErrors,
+		ErrorRate:   100 * float64(s.totalErrors) / float64(s.totalRequests),
+		ErrorsCount: map[string]int{},
 	}
-	out.Printf("Total errors: %v\n", s.totalErrors)
-	out.Printf("Error rate: %.4f%%\n", 100*float32(s.totalErrors)/float32(s.totalRequests))
+
+	for k, v := range s.errors {
+		sum.ErrorsCount[k] = v
+	}
+	return sum
 }
 
 func (s *benchmarkState) getQuantile(q float64) time.Duration {
diff --git a/benchmark.go b/benchmark.go
index d6c8654..0ff0f65 100644
--- a/benchmark.go
+++ b/benchmark.go
@@ -34,6 +34,7 @@ import (
 
 	"github.com/yarpc/yab/encoding"
 	"github.com/yarpc/yab/limiter"
+	"github.com/yarpc/yab/sorted"
 	"github.com/yarpc/yab/statsd"
 	"github.com/yarpc/yab/transport"
 
@@ -66,6 +67,13 @@ type Summary struct {
 	RPS                float64 `json:"rps"`
 }
 
+// ErrorSummary stores the summary of the errors encountered
+type ErrorSummary struct {
+	TotalErrors int            `json:"totalErrors"`
+	ErrorRate   float64        `json:"errorRate"`
+	ErrorsCount map[string]int `json:"errorsCount"`
+}
+
 // StreamSummary stores summary of stream messages sent and received
 type StreamSummary struct {
 	TotalStreamMessagesSent     int `json:"totalStreamMessagesSent"`
@@ -78,6 +86,9 @@ type BenchmarkOutput struct {
 	Latencies  map[string]string `json:"latencies"`
 	Summary    Summary           `json:"summary"`
 
+	// ErrorSummary sums up the errors encountered (if any). Is nil if no errors have been encountered
+	ErrorSummary *ErrorSummary `json:"errorSummary,omitempty"`
+
 	// StreamSummary is available only for streaming benchmark. It is nil and
 	// omitted in unary benchmark.
 	StreamSummary *StreamSummary `json:"streamSummary,omitempty"`
@@ -245,9 +256,7 @@ func runBenchmark(out output, logger *zap.Logger, allOpts Options, resolved reso
 		zap.Time("startTime", start),
 	)
 
-	// Print out errors
-	// TODO: allow errors to be printed in JSON format for output consistency
-	overall.printErrors(out)
+	errors := overall.getErrorSummary()
 
 	latencyValues := overall.getLatencies()
 
@@ -272,13 +281,13 @@ func runBenchmark(out output, logger *zap.Logger, allOpts Options, resolved reso
 	}
 
 	if formatAsJSON {
-		outputJSON(out, parameters, latencyValues, summary, streamSummary)
+		outputJSON(out, parameters, latencyValues, summary, streamSummary, errors)
 	} else {
-		outputPlaintext(out, latencyValues, summary, streamSummary)
+		outputPlaintext(out, latencyValues, summary, streamSummary, errors)
 	}
 }
 
-func outputJSON(out output, parameters Parameters, latencyValues map[float64]time.Duration, summary Summary, streamSummary *StreamSummary) {
+func outputJSON(out output, parameters Parameters, latencyValues map[float64]time.Duration, summary Summary, streamSummary *StreamSummary, errorSummary *ErrorSummary) {
 	latencies := make(map[string]string, len(_quantiles))
 	for _, quantile := range _quantiles {
 		latencies[fmt.Sprintf("%.4f", quantile)] = latencyValues[quantile].String()
@@ -288,6 +297,7 @@ func outputJSON(out output, parameters Parameters, latencyValues map[float64]tim
 		Parameters:    parameters,
 		Latencies:     latencies,
 		Summary:       summary,
+		ErrorSummary:  errorSummary,
 		StreamSummary: streamSummary,
 	}
 
@@ -298,7 +308,10 @@ func outputJSON(out output, parameters Parameters, latencyValues map[float64]tim
 	out.Printf("%s\n", jsonOutput)
 }
 
-func outputPlaintext(out output, latencyValues map[float64]time.Duration, summary Summary, streamSummary *StreamSummary) {
+func outputPlaintext(out output, latencyValues map[float64]time.Duration, summary Summary, streamSummary *StreamSummary, errorSummary *ErrorSummary) {
+	// Print errors
+	printErrors(out, errorSummary)
+
 	// Print out latencies
 	printLatencies(out, latencyValues)
 
@@ -330,6 +343,19 @@ func printLatencies(out output, latencyValues map[float64]time.Duration) {
 	}
 }
 
+func printErrors(out output, errorSum *ErrorSummary) {
+	if errorSum == nil {
+		return
+	}
+	out.Printf("Errors:\n")
+	for _, k := range sorted.MapKeys(errorSum.ErrorsCount) {
+		v := errorSum.ErrorsCount[k]
+		out.Printf("  %4d: %v\n", v, k)
+	}
+	out.Printf("Total errors: %v\n", errorSum.TotalErrors)
+	out.Printf("Error rate: %.4f%%\n", errorSum.ErrorRate)
+}
+
 // stopOnInterrupt sets up a signal that will trigger the run to stop.
 func stopOnInterrupt(out output, r *limiter.Run) {
 	c := make(chan os.Signal, 1)


Test patch:
diff --git a/bench_state_test.go b/bench_state_test.go
index 37ad019..dc7a48e 100644
--- a/bench_state_test.go
+++ b/bench_state_test.go
@@ -62,7 +62,7 @@ func TestBenchmarkStateErrors(t *testing.T) {
 	assert.Equal(t, state1.totalSuccess, 0, "Success count mismatch")
 	assert.Equal(t, state1.totalRequests, 7, "Request count mismatch")
 
-	state1.printErrors(out)
+	printErrors(out, state1.getErrorSummary())
 
 	expected := map[string]int{
 		"failed after Xms": 7,
@@ -81,7 +81,7 @@ func TestBenchmarkStateErrors(t *testing.T) {
 func TestBenchmarkStateNoError(t *testing.T) {
 	state := newBenchmarkState(statsd.Noop)
 	buf, _, out := getOutput(t)
-	state.printErrors(out)
+	printErrors(out, state.getErrorSummary())
 	assert.Equal(t, 0, buf.Len(), "Expected no output with no errors, got: %s", buf.String())
 }
 
diff --git a/benchmark_test.go b/benchmark_test.go
index 798b570..06cb9dd 100644
--- a/benchmark_test.go
+++ b/benchmark_test.go
@@ -315,3 +315,75 @@ func TestBenchmarkOutput(t *testing.T) {
 		}
 	}
 }
+
+func TestBenchmarkErrorsOutput(t *testing.T) {
+	// Testing the error output in text and json output format
+	tests := []struct {
+		format        string
+		wantJSON      bool
+		wantOutput    []string
+		notWantOutput []string
+	}{
+		{
+			format:   "json",
+			wantJSON: true,
+			wantOutput: []string{"summary", "benchmarkParameters", "maxRPS", "latencies", "errorSummary",
+				"errorRate", "totalErrors", "errorsCount"},
+			notWantOutput: []string{"Errors:", "Benchmark parameters", "Max RPS", "Unrecognized format option"},
+		},
+		{
+			format:        "text",
+			wantJSON:      false,
+			wantOutput:    []string{"Benchmark parameters", "Max RPS", "Errors:"},
+			notWantOutput: []string{"summary", "maxRPS", "Unrecognized format option", "errorRate", "totalErrors", "errorsCount"},
+		},
+	}
+
+	var requests atomic.Int32
+	s := newServer(t)
+	defer s.shutdown()
+	// GIVEN: The endpoint returns an error
+	s.register(fooMethod, methods.errorIf(func() bool {
+		requests.Inc()
+		return true
+	}))
+	m := benchmarkMethodForTest(t, fooMethod, transport.TChannel)
+
+	for _, tt := range tests {
+		t.Run(tt.format, func(t *testing.T) {
+			requests.Store(0)
+			buf, _, out := getOutput(t)
+			opts := Options{
+				BOpts: BenchmarkOptions{
+					MaxRequests:    1,
+					MaxDuration:    100 * time.Millisecond,
+					Connections:    1,
+					WarmupRequests: 0,
+					Concurrency:    1,
+					Format:         tt.format,
+				},
+				TOpts: s.transportOpts(),
+			}
+
+			runBenchmark(out, _testLogger, opts, _resolvedTChannelThrift, fooMethod, m)
+			bufStr := buf.String()
+
+			for _, want := range tt.wantOutput {
+				assert.Contains(t, bufStr, want)
+			}
+			for _, notWant := range tt.notWantOutput {
+				assert.NotContains(t, bufStr, notWant)
+			}
+
+			if tt.wantJSON {
+				// Creating struct from JSON output string
+				var benchmarkOutput BenchmarkOutput
+				b := []byte(bufStr)
+				err := json.Unmarshal(b, &benchmarkOutput)
+				require.NoError(t, err)
+				// EXPECT: All requests were errors
+				assert.GreaterOrEqual(t, opts.BOpts.MaxRequests, benchmarkOutput.ErrorSummary.TotalErrors)
+			}
+		})
+	}
+}