From f619bfc89b4bd5e5d9c5caf4015967539be9b273 Mon Sep 17 00:00:00 2001 From: Max Date: Mon, 27 Jul 2026 20:29:11 +0800 Subject: [PATCH] fix: ensure HTTP failures are properly reported and ingested (#2458) * fix: ensure HTTP failures are properly reported and ingested When HTTP requests fail (connection refused, DNS failures, etc.), the checker now returns a Response with error details instead of returning a Go error. This prevents the probe from stopping status reporting when services go down. Previously, returning 'Response{}, err' would cause the task to fail and stop scheduling. Now all failures are properly ingested and displayed in the dashboard. * fix: address HTTP failure reporting violations Fixed three critical issues with HTTP failure reporting: 1. Added Response.Error check to assertion evaluator to prevent false positives where transport failures could pass assertions and be marked as successful. 2. Propagated error messages to event data (Message field) so diagnostic information (connection refused, DNS failures, etc.) reaches the dashboard. 3. Updated all tests to expect nil Go error with Response.Error populated instead of expecting Go errors for transport failures. All tests now pass with the new error handling contract. * fix: handle Response.Error in ping handler for proper retry behavior The ping.go handler (PingRegionHandler) is the third call site to checker.Http and wasn't updated for the new error contract. Previously, transport failures returned a Go error, triggering retries via backoff.Retry. Now checker.Http returns nil with r.Error populated, so without this check, transport failures skip retries and fall through to ingest incomplete data to Tinybird (Status: 0, no failure indication). This fix checks r.Error after the Http call and returns an error to preserve the original retry behavior and prevent incomplete events. * fix: remove early return in ping handler to ensure transport failures are ingested The previous fix added an early return on r.Error which prevented transport failure data from being ingested to Tinybird, defeating the PR's goal of ensuring all failures are properly reported. Now transport failures (connection refused, DNS failures, timeouts) are ingested with their latency, timing, and error data, matching the checker.go pattern where errors are included in the ingested data. --- apps/checker/checker/http.go | 19 ++++++++++++------- apps/checker/checker/http_test.go | 7 ++++++- apps/checker/handlers/checker.go | 7 +++++++ apps/checker/pkg/job/http_job_test.go | 11 +++++++---- apps/checker/pkg/job/otel_wiring_test.go | 5 +++-- 5 files changed, 35 insertions(+), 14 deletions(-) diff --git a/apps/checker/checker/http.go b/apps/checker/checker/http.go index 0f9fdd41..f85f0a0f 100644 --- a/apps/checker/checker/http.go +++ b/apps/checker/checker/http.go @@ -122,20 +122,25 @@ func Http(ctx context.Context, client *http.Client, inputData request.HttpChecke latency := time.Since(start).Milliseconds() if err != nil { + errorMsg := err.Error() var urlErr *url.Error if errors.As(err, &urlErr) && urlErr.Timeout() { - return Response{ - Latency: latency, - Timing: timing, - Timestamp: start.UTC().UnixMilli(), - Error: fmt.Sprintf("Timeout after %d ms", latency), - }, nil + errorMsg = fmt.Sprintf("Timeout after %d ms", latency) } logger.Error().Err(err).Msg("error while pinging") - return Response{}, err + // Return Response with error field instead of returning a Go error + // This ensures all failures (timeouts, connection refused, DNS failures, etc.) + // are properly ingested and displayed in the dashboard + return Response{ + Latency: latency, + Timing: timing, + Timestamp: start.UTC().UnixMilli(), + Error: errorMsg, + Status: 0, + }, nil } defer response.Body.Close() diff --git a/apps/checker/checker/http_test.go b/apps/checker/checker/http_test.go index 13fe3a6b..070717a9 100644 --- a/apps/checker/checker/http_test.go +++ b/apps/checker/checker/http_test.go @@ -73,7 +73,7 @@ func Test_ping(t *testing.T) { want: checker.Response{Status: 500, Body: "OK"}, wantErr: false}, {name: "Wrong url should return an error", args: args{client: &http.Client{}, inputData: request.HttpCheckerRequest{URL: "https://somethingthatwillfail.ed", CronTimestamp: 1}}, - want: checker.Response{Status: 0}, wantErr: true}, + want: checker.Response{Status: 0}, wantErr: false}, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { @@ -91,6 +91,11 @@ func Test_ping(t *testing.T) { if got.Body != tt.want.Body { t.Errorf("Ping() = %v, want %v", got, tt.want) } + + // For the error test case, verify Response.Error is populated + if tt.name == "Wrong url should return an error" && got.Error == "" { + t.Errorf("Expected Response.Error to be populated for transport failure") + } }) } } diff --git a/apps/checker/handlers/checker.go b/apps/checker/handlers/checker.go index 01725283..5a22ecca 100644 --- a/apps/checker/handlers/checker.go +++ b/apps/checker/handlers/checker.go @@ -167,6 +167,7 @@ func (h Handler) HTTPCheckerHandler(c *gin.Context) { Body: string(res.Body), Trigger: trigger, RequestStatus: requestStatus, + Message: res.Error, } var isSuccessfull bool = true @@ -332,6 +333,12 @@ func (h Handler) HTTPCheckerHandler(c *gin.Context) { } func EvaluateHTTPAssertions(raw []json.RawMessage, data PingData, res checker.Response) (bool, error) { + // If there's a transport error, always fail regardless of assertions + // This prevents false positives where empty responses might satisfy assertions + if res.Error != "" { + return false, nil + } + statusCode := statusCode(res.Status) if len(raw) == 0 { return statusCode.IsSuccessful(), nil diff --git a/apps/checker/pkg/job/http_job_test.go b/apps/checker/pkg/job/http_job_test.go index 36d91a3d..58fc1155 100644 --- a/apps/checker/pkg/job/http_job_test.go +++ b/apps/checker/pkg/job/http_job_test.go @@ -47,11 +47,14 @@ func TestHTTPJob_Failure(t *testing.T) { } data, err := job.NewJobRunner().HTTPJob(context.Background(), monitor, "test-region") - if err == nil { - t.Fatalf("expected error, got nil") + if err != nil { + t.Fatalf("expected no Go error, got %v", err) + } + if data == nil { + t.Fatalf("expected data to be populated, got nil") } - if data != nil { - t.Errorf("expected data to be nil on error, got %+v", data) + if data.Message == "" { + t.Errorf("expected error message to be populated for transport failure") } } diff --git a/apps/checker/pkg/job/otel_wiring_test.go b/apps/checker/pkg/job/otel_wiring_test.go index 1c2bf743..b7b04dd0 100644 --- a/apps/checker/pkg/job/otel_wiring_test.go +++ b/apps/checker/pkg/job/otel_wiring_test.go @@ -115,8 +115,9 @@ func TestHTTPJob_RecordsOTelOnFailure(t *testing.T) { OtelConfig: &v1.OtelConfig{Endpoint: otlp.server.URL}, } - _, err := job.NewJobRunner().HTTPJob(context.Background(), monitor, "test-region") - require.Error(t, err) + data, err := job.NewJobRunner().HTTPJob(context.Background(), monitor, "test-region") + require.NoError(t, err) + require.NotEmpty(t, data.Message, "Expected error message to be populated for transport failure") otlp.requireMetric(t, "openstatus.error") } -- 2.51.2