diff --git a/cmd/logs_simple.go b/cmd/logs_simple.go index edddcec5..8c23fda3 100644 --- a/cmd/logs_simple.go +++ b/cmd/logs_simple.go @@ -7,7 +7,6 @@ package cmd import ( "fmt" - "io" "regexp" "strings" "time" @@ -789,13 +788,16 @@ func runLogsSearch(cmd *cobra.Command, args []string) error { // Fetch first page resp, r, err := api.ListLogs(client.Context(), opts) if err != nil { - if r != nil && r.Body != nil { - bodyBytes, readErr := io.ReadAll(r.Body) - if readErr == nil && len(bodyBytes) > 0 { + // These inline error handlers use extractAPIErrorBody directly instead of + // formatAPIError because they include domain-specific request details and + // troubleshooting context that the centralized helper does not support. + if r != nil { + apiBody := extractAPIErrorBody(err) + if apiBody != "" { fromTimeObj := time.UnixMilli(fromTime).UTC() toTimeObj := time.UnixMilli(toTime).UTC() return fmt.Errorf("failed to search logs: %w\nStatus: %d\nAPI Response: %s\n\nRequest Details:\n- Query: %s\n- From: %s UTC (parsed from: %s)\n- To: %s UTC (parsed from: %s)\n- Limit: %d\n\nTroubleshooting:\n- Verify your time range is valid\n- Check that your query syntax is correct\n- Ensure you have proper permissions", - err, r.StatusCode, string(bodyBytes), + err, r.StatusCode, apiBody, logsQuery, fromTimeObj.Format(time.RFC3339), logsFrom, toTimeObj.Format(time.RFC3339), logsTo, @@ -926,10 +928,10 @@ func runLogsList(cmd *cobra.Command, args []string) error { resp, r, err := api.ListLogs(client.Context(), opts) if err != nil { - if r != nil && r.Body != nil { - bodyBytes, readErr := io.ReadAll(r.Body) - if readErr == nil && len(bodyBytes) > 0 { - return fmt.Errorf("failed to list logs: %w\nStatus: %d\nAPI Response: %s", err, r.StatusCode, string(bodyBytes)) + if r != nil { + apiBody := extractAPIErrorBody(err) + if apiBody != "" { + return fmt.Errorf("failed to list logs: %w\nStatus: %d\nAPI Response: %s", err, r.StatusCode, apiBody) } return fmt.Errorf("failed to list logs: %w (status: %d)", err, r.StatusCode) } @@ -998,10 +1000,10 @@ func runLogsQuery(cmd *cobra.Command, args []string) error { resp, r, err := api.ListLogs(client.Context(), opts) if err != nil { - if r != nil && r.Body != nil { - bodyBytes, readErr := io.ReadAll(r.Body) - if readErr == nil && len(bodyBytes) > 0 { - return fmt.Errorf("failed to query logs: %w\nStatus: %d\nAPI Response: %s", err, r.StatusCode, string(bodyBytes)) + if r != nil { + apiBody := extractAPIErrorBody(err) + if apiBody != "" { + return fmt.Errorf("failed to query logs: %w\nStatus: %d\nAPI Response: %s", err, r.StatusCode, apiBody) } return fmt.Errorf("failed to query logs: %w (status: %d)", err, r.StatusCode) } @@ -1088,13 +1090,13 @@ func runLogsAggregate(cmd *cobra.Command, args []string) error { resp, r, err := api.AggregateLogs(client.Context(), body) if err != nil { - if r != nil && r.Body != nil { - bodyBytes, readErr := io.ReadAll(r.Body) - if readErr == nil && len(bodyBytes) > 0 { + if r != nil { + apiBody := extractAPIErrorBody(err) + if apiBody != "" { fromTimeObj := time.UnixMilli(fromTime).UTC() toTimeObj := time.UnixMilli(toTime).UTC() return fmt.Errorf("failed to aggregate logs: %w\nStatus: %d\nAPI Response: %s\n\nRequest Details:\n- Query: %s\n- Compute: %s (parsed as: aggregation=%q, metric=%q)\n- Group By: %s\n- From: %s UTC (parsed from: %s)\n- To: %s UTC (parsed from: %s)\n- Limit: %d\n\nTroubleshooting:\n- Verify the aggregation function is supported\n- Ensure the metric field exists in your logs (e.g., @duration, @bytes)\n- Check your query syntax\n- Verify your time range is valid", - err, r.StatusCode, string(bodyBytes), + err, r.StatusCode, apiBody, logsQuery, logsCompute, aggregation, metric, logsGroupBy, diff --git a/cmd/metrics.go b/cmd/metrics.go index 3e5eeb8f..5ce4df99 100644 --- a/cmd/metrics.go +++ b/cmd/metrics.go @@ -7,7 +7,6 @@ package cmd import ( "fmt" - "io" "strconv" "strings" "time" @@ -559,11 +558,11 @@ func runMetricsQuery(cmd *cobra.Command, args []string) error { resp, r, err := api.QueryTimeseriesData(client.Context(), body) if err != nil { - if r != nil && r.Body != nil { - bodyBytes, readErr := io.ReadAll(r.Body) - if readErr == nil && len(bodyBytes) > 0 { + if r != nil { + apiBody := extractAPIErrorBody(err) + if apiBody != "" { return fmt.Errorf("failed to query metrics: %w\nStatus: %d\nAPI Response: %s\n\nRequest Details:\n- Query: %s\n- From: %s (Unix: %d)\n- To: %s (Unix: %d)\n\nTroubleshooting:\n- Verify your query syntax is correct (e.g., avg:metric.name{filter})\n- Check that the time range is valid\n- Ensure the metric exists and has data in the specified time range\n- Confirm you have proper permissions to access the metric", - err, r.StatusCode, string(bodyBytes), + err, r.StatusCode, apiBody, queryString, from.Format(time.RFC3339), from.Unix(), to.Format(time.RFC3339), to.Unix()) @@ -604,11 +603,11 @@ func runMetricsSearch(cmd *cobra.Command, args []string) error { resp, r, err := api.QueryMetrics(client.Context(), from.Unix(), to.Unix(), queryString) if err != nil { - if r != nil && r.Body != nil { - bodyBytes, readErr := io.ReadAll(r.Body) - if readErr == nil && len(bodyBytes) > 0 { + if r != nil { + apiBody := extractAPIErrorBody(err) + if apiBody != "" { return fmt.Errorf("failed to search metrics: %w\nStatus: %d\nAPI Response: %s", - err, r.StatusCode, string(bodyBytes)) + err, r.StatusCode, apiBody) } return fmt.Errorf("failed to search metrics: %w (status: %d)", err, r.StatusCode) } @@ -643,11 +642,11 @@ func runMetricsList(cmd *cobra.Command, args []string) error { resp, r, err := api.ListActiveMetrics(client.Context(), from, *opts) if err != nil { - if r != nil && r.Body != nil { - bodyBytes, readErr := io.ReadAll(r.Body) - if readErr == nil && len(bodyBytes) > 0 { + if r != nil { + apiBody := extractAPIErrorBody(err) + if apiBody != "" { return fmt.Errorf("failed to list metrics: %w\nStatus: %d\nAPI Response: %s\n\nRequest Details:\n- Filter: %s\n- From: %s (Unix: %d)\n\nTroubleshooting:\n- Check that your filter pattern is valid\n- Verify you have permissions to list metrics", - err, r.StatusCode, string(bodyBytes), + err, r.StatusCode, apiBody, filterPattern, time.Unix(from, 0).Format(time.RFC3339), from) } @@ -677,11 +676,11 @@ func runMetricsMetadataGet(cmd *cobra.Command, args []string) error { resp, r, err := api.GetMetricMetadata(client.Context(), metricName) if err != nil { - if r != nil && r.Body != nil { - bodyBytes, readErr := io.ReadAll(r.Body) - if readErr == nil && len(bodyBytes) > 0 { + if r != nil { + apiBody := extractAPIErrorBody(err) + if apiBody != "" { return fmt.Errorf("failed to get metric metadata: %w\nStatus: %d\nAPI Response: %s\n\nMetric: %s\n\nTroubleshooting:\n- Verify the metric name is correct\n- Ensure the metric exists in your account\n- Check that you have permissions to view metadata", - err, r.StatusCode, string(bodyBytes), metricName) + err, r.StatusCode, apiBody, metricName) } return fmt.Errorf("failed to get metric metadata: %w (status: %d)", err, r.StatusCode) } @@ -733,11 +732,11 @@ func runMetricsMetadataUpdate(cmd *cobra.Command, args []string) error { resp, r, err := api.UpdateMetricMetadata(client.Context(), metricName, body) if err != nil { - if r != nil && r.Body != nil { - bodyBytes, readErr := io.ReadAll(r.Body) - if readErr == nil && len(bodyBytes) > 0 { + if r != nil { + apiBody := extractAPIErrorBody(err) + if apiBody != "" { return fmt.Errorf("failed to update metric metadata: %w\nStatus: %d\nAPI Response: %s\n\nMetric: %s\n\nTroubleshooting:\n- Verify the metric name is correct\n- Check that the metadata values are valid (unit, type, etc.)\n- Ensure you have permissions to update metadata", - err, r.StatusCode, string(bodyBytes), metricName) + err, r.StatusCode, apiBody, metricName) } return fmt.Errorf("failed to update metric metadata: %w (status: %d)", err, r.StatusCode) } @@ -833,11 +832,11 @@ func runMetricsSubmit(cmd *cobra.Command, args []string) error { resp, r, err := api.SubmitMetrics(client.Context(), body, *datadogV2.NewSubmitMetricsOptionalParameters()) if err != nil { - if r != nil && r.Body != nil { - bodyBytes, readErr := io.ReadAll(r.Body) - if readErr == nil && len(bodyBytes) > 0 { + if r != nil { + apiBody := extractAPIErrorBody(err) + if apiBody != "" { return fmt.Errorf("failed to submit metrics: %w\nStatus: %d\nAPI Response: %s\n\nRequest Details:\n- Metric: %s\n- Value: %f\n- Type: %s\n- Timestamp: %d\n- Tags: %v\n\nTroubleshooting:\n- Verify the metric name follows naming conventions (lowercase, dots/underscores)\n- Check that the metric type is valid (gauge, count, rate)\n- Ensure your API key has permission to submit metrics\n- Verify tags are in key:value format", - err, r.StatusCode, string(bodyBytes), + err, r.StatusCode, apiBody, submitName, submitValue, submitType, timestamp, tags) } return fmt.Errorf("failed to submit metrics: %w (status: %d)", err, r.StatusCode) diff --git a/cmd/root.go b/cmd/root.go index 8bb089e4..4d102b80 100644 --- a/cmd/root.go +++ b/cmd/root.go @@ -7,11 +7,13 @@ package cmd import ( "bufio" + "errors" "fmt" "io" "os" "strings" + "github.com/DataDog/datadog-api-client-go/v2/api/datadog" "github.com/DataDog/pup/internal/version" "github.com/DataDog/pup/pkg/client" "github.com/DataDog/pup/pkg/config" @@ -259,7 +261,26 @@ func readConfirmation() (string, error) { return "", scanner.Err() } -// formatAPIError creates user-friendly error messages for API errors +// extractAPIErrorBody extracts the API response body from a +// datadog.GenericOpenAPIError. The datadog-api-client-go library consumes +// http.Response.Body during deserialization and stores the bytes in the error. +// Callers that try to re-read http.Response.Body will always get empty data. +func extractAPIErrorBody(err error) string { + if err == nil { + return "" + } + var apiErr datadog.GenericOpenAPIError + if errors.As(err, &apiErr) { + if body := apiErr.Body(); len(body) > 0 { + return string(body) + } + } + return "" +} + +// formatAPIError creates user-friendly error messages for API errors. +// It extracts the API response body from GenericOpenAPIError when available +// and appends contextual guidance based on the HTTP status code. func formatAPIError(operation string, err error, response any) error { type httpResponse interface { StatusCode() int @@ -269,6 +290,11 @@ func formatAPIError(operation string, err error, response any) error { statusCode := r.StatusCode() baseMsg := fmt.Sprintf("failed to %s: %v (status: %d)", operation, err, statusCode) + // Include API response body if available + if body := extractAPIErrorBody(err); body != "" { + baseMsg = fmt.Sprintf("failed to %s: %v (status: %d)\nAPI Response: %s", operation, err, statusCode, body) + } + switch { case statusCode >= 500: // 5xx Server errors diff --git a/cmd/root_test.go b/cmd/root_test.go index d24bcbd0..adeee021 100644 --- a/cmd/root_test.go +++ b/cmd/root_test.go @@ -7,9 +7,11 @@ package cmd import ( "errors" + "fmt" "strings" "testing" + "github.com/DataDog/datadog-api-client-go/v2/api/datadog" "github.com/DataDog/pup/pkg/config" ) @@ -209,7 +211,7 @@ func TestFormatAPIError_AllStatusCodes(t *testing.T) { } for _, tt := range statusTests { - t.Run(string(rune(tt.code)), func(t *testing.T) { + t.Run(fmt.Sprintf("%d", tt.code), func(t *testing.T) { err := formatAPIError("test operation", errors.New("test error"), &mockHTTPResponse{statusCode: tt.code}) if err == nil { @@ -318,3 +320,83 @@ func TestTestCmd_InvalidSite(t *testing.T) { t.Errorf("testCmd.RunE() error should mention DD_SITE, got: %v", err) } } + +func TestExtractAPIErrorBody(t *testing.T) { + tests := []struct { + name string + err error + want string + }{ + { + name: "GenericOpenAPIError with body", + err: datadog.GenericOpenAPIError{ + ErrorBody: []byte(`{"errors":["Invalid query: avg:nonexistent.metric{*}"]}`), + ErrorMessage: "400 Bad Request", + }, + want: `{"errors":["Invalid query: avg:nonexistent.metric{*}"]}`, + }, + { + name: "GenericOpenAPIError with empty body", + err: datadog.GenericOpenAPIError{ + ErrorBody: []byte{}, + ErrorMessage: "400 Bad Request", + }, + want: "", + }, + { + name: "GenericOpenAPIError with nil body", + err: datadog.GenericOpenAPIError{ + ErrorBody: nil, + ErrorMessage: "400 Bad Request", + }, + want: "", + }, + { + name: "wrapped GenericOpenAPIError", + err: fmt.Errorf("api call failed: %w", datadog.GenericOpenAPIError{ + ErrorBody: []byte(`{"errors":["bad query"]}`), + ErrorMessage: "400 Bad Request", + }), + want: `{"errors":["bad query"]}`, + }, + { + name: "non-GenericOpenAPIError", + err: errors.New("some other error"), + want: "", + }, + { + name: "nil error", + err: nil, + want: "", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := extractAPIErrorBody(tt.err) + if got != tt.want { + t.Errorf("extractAPIErrorBody() = %q, want %q", got, tt.want) + } + }) + } +} + +func TestFormatAPIError_IncludesResponseBody(t *testing.T) { + // This test verifies that formatAPIError surfaces the API response body + // from GenericOpenAPIError, which was previously lost because the code + // tried to re-read the already-consumed http.Response.Body. + apiErr := datadog.GenericOpenAPIError{ + ErrorBody: []byte(`{"errors":["Query parse error: unknown metric"]}`), + ErrorMessage: "400 Bad Request", + } + + err := formatAPIError("query metrics", apiErr, &mockHTTPResponse{statusCode: 400}) + errMsg := err.Error() + + if !strings.Contains(errMsg, "unknown metric") { + t.Errorf("formatAPIError() should include API response body, got: %q", errMsg) + } + if !strings.Contains(errMsg, "status: 400") { + t.Errorf("formatAPIError() should include status code, got: %q", errMsg) + } +}