feat(telemetry): surface per-integration health-check failure reasons#512
feat(telemetry): surface per-integration health-check failure reasons#512mayankpande88 wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
Code Review
This pull request enhances the health-checking mechanism for various datasources (such as Prometheus, AlertManager, OpenCost, and logs providers) by capturing and propagating detailed failure reasons (e.g., HTTP status codes and truncated response bodies) to the telemetry service and UI. Feedback on these changes highlights an inefficiency in both httpProbeErr and httpHealth where the response body is read on successful probes. This unnecessarily allocates memory and prevents TCP connection reuse; the reviewer recommends checking the status code first and draining the body to io.Discard on success.
| body, _ := io.ReadAll(io.LimitReader(resp.Body, 512)) | ||
| if resp.StatusCode >= 200 && resp.StatusCode < 300 { | ||
| return true, "" | ||
| } |
There was a problem hiding this comment.
Reading the response body on every successful health check probe is inefficient as it allocates memory unnecessarily. Additionally, if the response body of a successful probe is larger than 512 bytes, limiting the read to 512 bytes without fully draining the remaining body prevents the HTTP client from reusing the underlying TCP connection (keep-alive).
Checking the status code first allows us to fully drain the body to io.Discard on success, ensuring connection reuse and avoiding unnecessary allocations.
| body, _ := io.ReadAll(io.LimitReader(resp.Body, 512)) | |
| if resp.StatusCode >= 200 && resp.StatusCode < 300 { | |
| return true, "" | |
| } | |
| if resp.StatusCode >= 200 && resp.StatusCode < 300 { | |
| _, _ = io.Copy(io.Discard, resp.Body) | |
| return true, "" | |
| } | |
| body, _ := io.ReadAll(io.LimitReader(resp.Body, 512)) |
| body, _ := io.ReadAll(io.LimitReader(resp.Body, 512)) | ||
| if resp.StatusCode >= 200 && resp.StatusCode < 300 { | ||
| return true, "" | ||
| } |
There was a problem hiding this comment.
Reading the response body on every successful health check probe is inefficient as it allocates memory unnecessarily. Additionally, if the response body of a successful probe is larger than 512 bytes, limiting the read to 512 bytes without fully draining the remaining body prevents the HTTP client from reusing the underlying TCP connection (keep-alive).
Checking the status code first allows us to fully drain the body to io.Discard on success, ensuring connection reuse and avoiding unnecessary allocations.
| body, _ := io.ReadAll(io.LimitReader(resp.Body, 512)) | |
| if resp.StatusCode >= 200 && resp.StatusCode < 300 { | |
| return true, "" | |
| } | |
| if resp.StatusCode >= 200 && resp.StatusCode < 300 { | |
| _, _ = io.Copy(io.Discard, resp.Body) | |
| return true, "" | |
| } | |
| body, _ := io.ReadAll(io.LimitReader(resp.Body, 512)) |
Health probes previously collapsed every failure to a bare boolean, so the
agent health UI could only show "Disconnected" with no reason — a probe
returning e.g. 401 "token is expired" logged locally but never reached the
backend.
Capture the failure reason from each probe and emit it on activity_stats as
prometheusConnectionError / alertManagerConnectionError / logsConnectionError /
opencostConnectionError (populated only on failure, omitted when healthy). The
in-package httpHealth and the logs-provider probe (httpProbeErr) now return a
compact one-line reason ("HTTP <status>: <body-snippet>" or transport error)
that the backend stores in connection_status and the UI renders next to the
Disconnected status.
9528743 to
f2ad782
Compare
Description
Agent health probes previously collapsed every outcome to a boolean, so the Agent Health UI could only show Disconnected with no reason — a probe returning e.g.
401 token is expiredwas logged locally but never reached the backend.This captures the failure reason from each probe and emits it on
activity_stats(stored in the agent'sconnection_statusJSONB):prometheusConnectionError,alertManagerConnectionError,logsConnectionError,opencostConnectionErrorhttpHealth(in-package) and the logs-provider probe (httpProbeErr) now return a compact one-line reason:HTTP <status>: <body-snippet>(whitespace-collapsed, truncated to 200 chars) or the transport error.The backend pass-through + UI rendering ship in the paired nudgebee-enterprise PR (Fixes #33783 there).
Type of change
Risks
/api/v1/healthis probed unauthenticated, so a JWT-expiry 401 that happens at query time is not seen by this probe — the badge can still read "Connected" while queries 401. Making the Signoz probe authenticated (like the ES probe already is) is a sensible follow-up, related to thefeat/signoz-password-authwork.Testing
go build ./...passes,gofmtclean,go test ./pkg/telemetry/green.