Fixed client-side errors being incorrectly reported as server errors in OTEL telemetry (#40051)
<!-- Add the related story/sub-task/bug number, like Resolves #123, or remove if NA --> **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 <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## 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. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
This commit is contained in:
@@ -0,0 +1 @@
|
||||
- Fixed client-side errors being incorrectly reported as server errors in OTEL telemetry.
|
||||
@@ -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() {}
|
||||
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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 {
|
||||
|
||||
@@ -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
|
||||
}
|
||||
|
||||
@@ -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 {
|
||||
|
||||
@@ -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
|
||||
}
|
||||
|
||||
@@ -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()
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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{},
|
||||
|
||||
@@ -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
|
||||
}
|
||||
|
||||
+4
@@ -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 {
|
||||
|
||||
@@ -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"`
|
||||
|
||||
@@ -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{}
|
||||
}
|
||||
|
||||
@@ -31,6 +31,10 @@ func (p cveNotFoundError) IsNotFound() bool {
|
||||
return true
|
||||
}
|
||||
|
||||
func (p cveNotFoundError) IsClientError() bool {
|
||||
return true
|
||||
}
|
||||
|
||||
type listVulnerabilitiesRequest struct {
|
||||
fleet.VulnListOptions
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user