From 698d447ab49ba1a3d78beabab3c11ed58e98314f Mon Sep 17 00:00:00 2001
From: Ro Santalla <roberto@santalla.io>
Date: Wed, 26 Jun 2024 13:56:58 +0200
Subject: [PATCH] k6runner: send logs even if metrics are malformed

---
 internal/k6runner/k6runner.go      | 14 ++++++-------
 internal/k6runner/k6runner_test.go | 33 +++++++++++++++++++++++++++++-
 2 files changed, 39 insertions(+), 8 deletions(-)

diff --git a/internal/k6runner/k6runner.go b/internal/k6runner/k6runner.go
index 0f743107a..6d548a47c 100644
--- a/internal/k6runner/k6runner.go
+++ b/internal/k6runner/k6runner.go
@@ -99,6 +99,13 @@ func (r Processor) Run(ctx context.Context, registry *prometheus.Registry, logge
 		return false, err
 	}
 
+	if err := k6LogsToLogger(result.Logs, logger); err != nil {
+		internalLogger.Debug().
+			Err(err).
+			Msg("cannot load logs to logger")
+		return false, err
+	}
+
 	var (
 		collector       sampleCollector
 		resultCollector checkResultCollector
@@ -118,13 +125,6 @@ func (r Processor) Run(ctx context.Context, registry *prometheus.Registry, logge
 		return false, err
 	}
 
-	if err := k6LogsToLogger(result.Logs, logger); err != nil {
-		internalLogger.Debug().
-			Err(err).
-			Msg("cannot load logs to logger")
-		return false, err
-	}
-
 	// If only one of Error and ErrorCode are non-empty, the proxy is misbehaving.
 	switch {
 	case result.Error == "" && result.ErrorCode != "":
diff --git a/internal/k6runner/k6runner_test.go b/internal/k6runner/k6runner_test.go
index 49a9ab8ce..6c4ce02d3 100644
--- a/internal/k6runner/k6runner_test.go
+++ b/internal/k6runner/k6runner_test.go
@@ -193,6 +193,7 @@ func TestScriptHTTPRun(t *testing.T) {
 		statusCode    int
 		expectSuccess bool
 		expectError   error
+		expectErrorAs any // To accomodate return of unnamed errors. If set, expectError is ignored.
 		expectLogs    string
 	}{
 		{
@@ -242,6 +243,32 @@ func TestScriptHTTPRun(t *testing.T) {
 			expectError:   nil,
 			expectLogs:    nonDebugLogLine,
 		},
+		{
+			name: "borked logs are sent best-effort",
+			response: &RunResponse{
+				Metrics:   testMetrics,
+				Logs:      []byte(`level=error foo="b` + "\n"),
+				Error:     "we killed k6",
+				ErrorCode: "user",
+			},
+			statusCode:    http.StatusUnprocessableEntity,
+			expectSuccess: false,
+			expectError:   nil,
+			expectLogs:    `level="error"` + "\n",
+		},
+		{
+			name: "logs are sent on borked metrics",
+			response: &RunResponse{
+				Metrics:   []byte("probe_succ{"),
+				Logs:      testLogs,
+				Error:     "we killed k6",
+				ErrorCode: "user",
+			},
+			statusCode:    http.StatusUnprocessableEntity,
+			expectSuccess: false,
+			expectErrorAs: &expfmt.ParseError{},
+			expectLogs:    nonDebugLogLine,
+		},
 		{
 			name: "inconsistent runner response A",
 			response: &RunResponse{
@@ -314,7 +341,11 @@ func TestScriptHTTPRun(t *testing.T) {
 			success, err := script.Run(ctx, registry, logger, zlogger)
 			require.Equal(t, tc.expectSuccess, success)
 			require.Equal(t, tc.expectLogs, logger.buf.String())
-			require.ErrorIs(t, err, tc.expectError)
+			if tc.expectErrorAs == nil {
+				require.ErrorIs(t, err, tc.expectError)
+			} else {
+				require.ErrorAs(t, err, tc.expectErrorAs)
+			}
 		})
 	}
 }