From d83fd5f3847d3afa230b026672d466f5779e9363 Mon Sep 17 00:00:00 2001 From: Victor Lyuboslavsky <2685025+getvictor@users.noreply.github.com> Date: Thu, 19 Feb 2026 16:06:00 -0600 Subject: [PATCH] Fixed client-side errors being incorrectly reported as server errors in OTEL telemetry (#40051) **Related issue:** Resolves #40028 # Checklist for submitter - [x] Changes file added for user-visible changes in `changes/`, `orbit/changes/` or `ee/fleetd-chrome/changes`. ## Testing - [x] Added/updated automated tests - [x] QA'd all new/changed functionality manually ## Summary by CodeRabbit ## Release Notes * **Bug Fixes** * Fixed telemetry misclassification where client-side errors were incorrectly reported as server errors. Client-side errors and request cancellations are now properly categorized for improved error tracking and observability. * **Tests** * Added test coverage for client error detection and context cancellation handling. --- changes/40028-otel-client-errors | 1 + server/authz/errors.go | 4 +++ server/contexts/ctxerr/ctxerr.go | 19 ++++++++++- server/contexts/ctxerr/ctxerr_test.go | 32 +++++++++++++++++++ server/datastore/mysql/errors.go | 4 +++ server/datastore/mysql/nanomdm_storage.go | 4 +++ server/fleet/cron_schedules.go | 8 +++++ .../service/androidmgmt/google_client.go | 4 +++ server/platform/endpointer/transport_error.go | 11 +++++++ .../endpointer/transport_error_test.go | 11 +++++++ .../middleware/ratelimit/ratelimit.go | 4 +++ .../conditional_access_microsoft_proxy.go | 4 +++ server/service/orbit.go | 19 +++++++++++ server/service/service_errors.go | 8 +++++ server/service/vulnerabilities.go | 4 +++ 15 files changed, 136 insertions(+), 1 deletion(-) create mode 100644 changes/40028-otel-client-errors diff --git a/changes/40028-otel-client-errors b/changes/40028-otel-client-errors new file mode 100644 index 0000000000..28e8a4f45e --- /dev/null +++ b/changes/40028-otel-client-errors @@ -0,0 +1 @@ +- Fixed client-side errors being incorrectly reported as server errors in OTEL telemetry. diff --git a/server/authz/errors.go b/server/authz/errors.go index 6fc24a8733..a4f117466a 100644 --- a/server/authz/errors.go +++ b/server/authz/errors.go @@ -46,6 +46,10 @@ func (e *Forbidden) StatusCode() int { return http.StatusForbidden } +func (e *Forbidden) IsClientError() bool { + return true +} + // Forbidden implements platform_authz.Forbidden interface. func (e *Forbidden) Forbidden() {} diff --git a/server/contexts/ctxerr/ctxerr.go b/server/contexts/ctxerr/ctxerr.go index 1e5da73f66..c56dc175b8 100644 --- a/server/contexts/ctxerr/ctxerr.go +++ b/server/contexts/ctxerr/ctxerr.go @@ -398,13 +398,30 @@ func collectTelemetryContext(ctx context.Context) map[string]any { } // isClientError checks if the error is a client error (4xx). +// Error types that represent client errors should implement ErrWithIsClientError. func isClientError(err error) bool { - // Check for explicit client error interface + if err == nil { + return false + } + + // Check for explicit client error interface. All 4xx error types + // (not found, already exists, conflict, validation, permission, + // bad request, foreign key, etc.) should implement this interface. var clientErr platform_http.ErrWithIsClientError if errors.As(err, &clientErr) { return clientErr.IsClientError() } + // Check for errors with an explicit HTTP status code in the 4xx range + type statusCoder interface{ StatusCode() int } + var sc statusCoder + if errors.As(err, &sc) { + code := sc.StatusCode() + if code >= 400 && code < 500 { + return true + } + } + // Treat context.Canceled as a client error. In HTTP handlers, this typically // indicates client disconnection. While it could theoretically come from // server-side cancellation, detecting true client disconnection at the diff --git a/server/contexts/ctxerr/ctxerr_test.go b/server/contexts/ctxerr/ctxerr_test.go index 74e9b5c607..fb59694b02 100644 --- a/server/contexts/ctxerr/ctxerr_test.go +++ b/server/contexts/ctxerr/ctxerr_test.go @@ -388,6 +388,18 @@ func TestLogFields(t *testing.T) { } } +// mockClientError implements ErrWithIsClientError for testing. +type mockClientError struct{ isClient bool } + +func (e *mockClientError) Error() string { return "mock error" } +func (e *mockClientError) IsClientError() bool { return e.isClient } + +// mockStatusCoder implements StatusCode() for testing. +type mockStatusCoder struct{ code int } + +func (e *mockStatusCoder) Error() string { return "status error" } +func (e *mockStatusCoder) StatusCode() int { return e.code } + func TestIsClientError(t *testing.T) { tests := []struct { name string @@ -424,6 +436,26 @@ func TestIsClientError(t *testing.T) { err: &fleet.InvalidArgumentError{}, expected: true, }, + { + name: "IsClientError returns true", + err: &mockClientError{isClient: true}, + expected: true, + }, + { + name: "IsClientError returns false", + err: &mockClientError{isClient: false}, + expected: false, + }, + { + name: "status coder 4xx", + err: &mockStatusCoder{code: 422}, + expected: true, + }, + { + name: "status coder 5xx", + err: &mockStatusCoder{code: 500}, + expected: false, + }, } for _, tt := range tests { diff --git a/server/datastore/mysql/errors.go b/server/datastore/mysql/errors.go index e5ef73c3f3..952cfadfbf 100644 --- a/server/datastore/mysql/errors.go +++ b/server/datastore/mysql/errors.go @@ -71,6 +71,10 @@ func (e *existsError) IsExists() bool { return true } +func (e *existsError) IsClientError() bool { + return true +} + func (e *existsError) Resource() string { return e.ResourceType } diff --git a/server/datastore/mysql/nanomdm_storage.go b/server/datastore/mysql/nanomdm_storage.go index ceb2cceedd..05ef2bf610 100644 --- a/server/datastore/mysql/nanomdm_storage.go +++ b/server/datastore/mysql/nanomdm_storage.go @@ -35,6 +35,10 @@ func (e lockConflictError) IsConflict() bool { return true } +func (e lockConflictError) IsClientError() bool { + return true +} + // isConflict checks if an error implements the IsConflict() interface func isConflict(err error) bool { type conflictInterface interface { diff --git a/server/fleet/cron_schedules.go b/server/fleet/cron_schedules.go index 943a0bff4e..8a391e9993 100644 --- a/server/fleet/cron_schedules.go +++ b/server/fleet/cron_schedules.go @@ -148,6 +148,10 @@ func (e triggerConflictError) IsConflict() bool { return true } +func (e triggerConflictError) IsClientError() bool { + return true +} + func (e triggerConflictError) StatusCode() int { return http.StatusConflict } @@ -165,6 +169,10 @@ func (e triggerNotFoundError) IsNotFound() bool { return true } +func (e triggerNotFoundError) IsClientError() bool { + return true +} + func (e triggerNotFoundError) StatusCode() int { return http.StatusNotFound } diff --git a/server/mdm/android/service/androidmgmt/google_client.go b/server/mdm/android/service/androidmgmt/google_client.go index f00c26327f..8f0ffa439b 100644 --- a/server/mdm/android/service/androidmgmt/google_client.go +++ b/server/mdm/android/service/androidmgmt/google_client.go @@ -339,6 +339,10 @@ func (p appNotFoundError) IsNotFound() bool { return true } +func (p appNotFoundError) IsClientError() bool { + return true +} + func (g *GoogleClient) EnterprisesApplications(ctx context.Context, enterpriseName, packageName string) (*androidmanagement.Application, error) { path := fmt.Sprintf("%s/applications/%s", enterpriseName, packageName) app, err := g.mgmt.Enterprises.Applications.Get(path).Context(ctx).Do() diff --git a/server/platform/endpointer/transport_error.go b/server/platform/endpointer/transport_error.go index c1cc28c932..238b8ee48b 100644 --- a/server/platform/endpointer/transport_error.go +++ b/server/platform/endpointer/transport_error.go @@ -159,6 +159,17 @@ func EncodeError(ctx context.Context, err error, w http.ResponseWriter, domainEn return } + // context.Canceled typically means the client disconnected before the server finished + // processing. Return 499 (Client Closed Request, nginx convention) so observability tools + // correctly classify it as a client error rather than a server error. + if errors.Is(origErr, context.Canceled) { + jsonErr.Message = "Client Closed Request" + jsonErr.Errors = baseError(origErr.Error()) + w.WriteHeader(499) + enc.Encode(jsonErr) //nolint:errcheck + return + } + // Get specific status code if it is available from this error type, // defaulting to HTTP 500 status := http.StatusInternalServerError diff --git a/server/platform/endpointer/transport_error_test.go b/server/platform/endpointer/transport_error_test.go index 5231b289a6..1b70a1c1a5 100644 --- a/server/platform/endpointer/transport_error_test.go +++ b/server/platform/endpointer/transport_error_test.go @@ -2,6 +2,7 @@ package endpointer import ( "context" + "fmt" "net/http" "net/http/httptest" "testing" @@ -98,6 +99,16 @@ func TestHandlesErrorsCode(t *testing.T) { platform_http.NewAuthFailedError(""), http.StatusUnauthorized, }, + { + "context canceled", + context.Canceled, + 499, + }, + { + "wrapped context canceled", + fmt.Errorf("db query: %w", context.Canceled), + 499, + }, { "default", newAndExciting{}, diff --git a/server/platform/middleware/ratelimit/ratelimit.go b/server/platform/middleware/ratelimit/ratelimit.go index 29736a9e27..53df8fbe7a 100644 --- a/server/platform/middleware/ratelimit/ratelimit.go +++ b/server/platform/middleware/ratelimit/ratelimit.go @@ -151,6 +151,10 @@ func (r rateLimitError) RetryAfter() int { return int(r.result.RetryAfter.Seconds()) } +func (r rateLimitError) IsClientError() bool { + return true +} + func (r rateLimitError) Result() throttled.RateLimitResult { return r.result } diff --git a/server/service/conditional_access_microsoft_proxy/conditional_access_microsoft_proxy.go b/server/service/conditional_access_microsoft_proxy/conditional_access_microsoft_proxy.go index 4a17a69d62..0a64c90349 100644 --- a/server/service/conditional_access_microsoft_proxy/conditional_access_microsoft_proxy.go +++ b/server/service/conditional_access_microsoft_proxy/conditional_access_microsoft_proxy.go @@ -296,6 +296,10 @@ func (e *notFoundError) IsNotFound() bool { return true } +func (e *notFoundError) IsClientError() bool { + return true +} + func (p *Proxy) setHeaders(r *http.Request) error { origin, err := p.originGetter() if err != nil { diff --git a/server/service/orbit.go b/server/service/orbit.go index 57caf09469..fee1ac7bd4 100644 --- a/server/service/orbit.go +++ b/server/service/orbit.go @@ -2,13 +2,16 @@ package service import ( "context" + "crypto/x509" "database/sql" "encoding/json" "errors" "fmt" + "io" "log/slog" "net/http" "net/url" + "os" "github.com/fleetdm/fleet/v4/ee/server/service/hostidentity/httpsig" "github.com/fleetdm/fleet/v4/server" @@ -48,6 +51,22 @@ func (r *orbitGetConfigRequest) orbitHostNodeKey() string { return r.OrbitNodeKey } +// DecodeBody implements the bodyDecoder interface for custom request body decoding. +// This endpoint is susceptible to client read timeouts (poll.DeadlineExceededError). +// By implementing DecodeBody, we classify those network errors as client errors. +func (r *orbitGetConfigRequest) DecodeBody(_ context.Context, reader io.Reader, _ url.Values, _ []*x509.Certificate) error { + if err := json.NewDecoder(reader).Decode(r); err != nil { + if errors.Is(err, os.ErrDeadlineExceeded) { + return &fleet.BadRequestError{ + Message: "request body read timeout", + InternalErr: err, + } + } + return err + } + return nil +} + type orbitGetConfigResponse struct { fleet.OrbitConfig Err error `json:"error,omitempty"` diff --git a/server/service/service_errors.go b/server/service/service_errors.go index 199238b364..7e7a238bec 100644 --- a/server/service/service_errors.go +++ b/server/service/service_errors.go @@ -19,6 +19,10 @@ func (a *alreadyExistsError) IsExists() bool { return true } +func (a *alreadyExistsError) IsClientError() bool { + return true +} + func newAlreadyExistsError() *alreadyExistsError { return &alreadyExistsError{} } @@ -35,6 +39,10 @@ func (e *notFoundError) IsNotFound() bool { return true } +func (e *notFoundError) IsClientError() bool { + return true +} + func newNotFoundError() *notFoundError { return ¬FoundError{} } diff --git a/server/service/vulnerabilities.go b/server/service/vulnerabilities.go index c77af170ea..ca402c7f3a 100644 --- a/server/service/vulnerabilities.go +++ b/server/service/vulnerabilities.go @@ -31,6 +31,10 @@ func (p cveNotFoundError) IsNotFound() bool { return true } +func (p cveNotFoundError) IsClientError() bool { + return true +} + type listVulnerabilitiesRequest struct { fleet.VulnListOptions }