Skip to content

Commit 634811b

Browse files
platinummonkeyplatinummonkey
authored
Merge pull request #52 from DataDog/fix/extract-api-error-body
fix(errors): extract API error body from GenericOpenAPIError
2 parents d7e927e + cab8890 commit 634811b

4 files changed

Lines changed: 153 additions & 44 deletions

File tree

‎cmd/logs_simple.go‎

Lines changed: 19 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,6 @@ package cmd
77

88
import (
99
"fmt"
10-
"io"
1110
"regexp"
1211
"strings"
1312
"time"
@@ -789,13 +788,16 @@ func runLogsSearch(cmd *cobra.Command, args []string) error {
789788
// Fetch first page
790789
resp, r, err := api.ListLogs(client.Context(), opts)
791790
if err != nil {
792-
if r != nil && r.Body != nil {
793-
bodyBytes, readErr := io.ReadAll(r.Body)
794-
if readErr == nil && len(bodyBytes) > 0 {
791+
// These inline error handlers use extractAPIErrorBody directly instead of
792+
// formatAPIError because they include domain-specific request details and
793+
// troubleshooting context that the centralized helper does not support.
794+
if r != nil {
795+
apiBody := extractAPIErrorBody(err)
796+
if apiBody != "" {
795797
fromTimeObj := time.UnixMilli(fromTime).UTC()
796798
toTimeObj := time.UnixMilli(toTime).UTC()
797799
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",
798-
err, r.StatusCode, string(bodyBytes),
800+
err, r.StatusCode, apiBody,
799801
logsQuery,
800802
fromTimeObj.Format(time.RFC3339), logsFrom,
801803
toTimeObj.Format(time.RFC3339), logsTo,
@@ -926,10 +928,10 @@ func runLogsList(cmd *cobra.Command, args []string) error {
926928

927929
resp, r, err := api.ListLogs(client.Context(), opts)
928930
if err != nil {
929-
if r != nil && r.Body != nil {
930-
bodyBytes, readErr := io.ReadAll(r.Body)
931-
if readErr == nil && len(bodyBytes) > 0 {
932-
return fmt.Errorf("failed to list logs: %w\nStatus: %d\nAPI Response: %s", err, r.StatusCode, string(bodyBytes))
931+
if r != nil {
932+
apiBody := extractAPIErrorBody(err)
933+
if apiBody != "" {
934+
return fmt.Errorf("failed to list logs: %w\nStatus: %d\nAPI Response: %s", err, r.StatusCode, apiBody)
933935
}
934936
return fmt.Errorf("failed to list logs: %w (status: %d)", err, r.StatusCode)
935937
}
@@ -998,10 +1000,10 @@ func runLogsQuery(cmd *cobra.Command, args []string) error {
9981000

9991001
resp, r, err := api.ListLogs(client.Context(), opts)
10001002
if err != nil {
1001-
if r != nil && r.Body != nil {
1002-
bodyBytes, readErr := io.ReadAll(r.Body)
1003-
if readErr == nil && len(bodyBytes) > 0 {
1004-
return fmt.Errorf("failed to query logs: %w\nStatus: %d\nAPI Response: %s", err, r.StatusCode, string(bodyBytes))
1003+
if r != nil {
1004+
apiBody := extractAPIErrorBody(err)
1005+
if apiBody != "" {
1006+
return fmt.Errorf("failed to query logs: %w\nStatus: %d\nAPI Response: %s", err, r.StatusCode, apiBody)
10051007
}
10061008
return fmt.Errorf("failed to query logs: %w (status: %d)", err, r.StatusCode)
10071009
}
@@ -1088,13 +1090,13 @@ func runLogsAggregate(cmd *cobra.Command, args []string) error {
10881090

10891091
resp, r, err := api.AggregateLogs(client.Context(), body)
10901092
if err != nil {
1091-
if r != nil && r.Body != nil {
1092-
bodyBytes, readErr := io.ReadAll(r.Body)
1093-
if readErr == nil && len(bodyBytes) > 0 {
1093+
if r != nil {
1094+
apiBody := extractAPIErrorBody(err)
1095+
if apiBody != "" {
10941096
fromTimeObj := time.UnixMilli(fromTime).UTC()
10951097
toTimeObj := time.UnixMilli(toTime).UTC()
10961098
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",
1097-
err, r.StatusCode, string(bodyBytes),
1099+
err, r.StatusCode, apiBody,
10981100
logsQuery,
10991101
logsCompute, aggregation, metric,
11001102
logsGroupBy,

‎cmd/metrics.go‎

Lines changed: 24 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,6 @@ package cmd
77

88
import (
99
"fmt"
10-
"io"
1110
"strconv"
1211
"strings"
1312
"time"
@@ -559,11 +558,11 @@ func runMetricsQuery(cmd *cobra.Command, args []string) error {
559558

560559
resp, r, err := api.QueryTimeseriesData(client.Context(), body)
561560
if err != nil {
562-
if r != nil && r.Body != nil {
563-
bodyBytes, readErr := io.ReadAll(r.Body)
564-
if readErr == nil && len(bodyBytes) > 0 {
561+
if r != nil {
562+
apiBody := extractAPIErrorBody(err)
563+
if apiBody != "" {
565564
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",
566-
err, r.StatusCode, string(bodyBytes),
565+
err, r.StatusCode, apiBody,
567566
queryString,
568567
from.Format(time.RFC3339), from.Unix(),
569568
to.Format(time.RFC3339), to.Unix())
@@ -604,11 +603,11 @@ func runMetricsSearch(cmd *cobra.Command, args []string) error {
604603

605604
resp, r, err := api.QueryMetrics(client.Context(), from.Unix(), to.Unix(), queryString)
606605
if err != nil {
607-
if r != nil && r.Body != nil {
608-
bodyBytes, readErr := io.ReadAll(r.Body)
609-
if readErr == nil && len(bodyBytes) > 0 {
606+
if r != nil {
607+
apiBody := extractAPIErrorBody(err)
608+
if apiBody != "" {
610609
return fmt.Errorf("failed to search metrics: %w\nStatus: %d\nAPI Response: %s",
611-
err, r.StatusCode, string(bodyBytes))
610+
err, r.StatusCode, apiBody)
612611
}
613612
return fmt.Errorf("failed to search metrics: %w (status: %d)", err, r.StatusCode)
614613
}
@@ -643,11 +642,11 @@ func runMetricsList(cmd *cobra.Command, args []string) error {
643642

644643
resp, r, err := api.ListActiveMetrics(client.Context(), from, *opts)
645644
if err != nil {
646-
if r != nil && r.Body != nil {
647-
bodyBytes, readErr := io.ReadAll(r.Body)
648-
if readErr == nil && len(bodyBytes) > 0 {
645+
if r != nil {
646+
apiBody := extractAPIErrorBody(err)
647+
if apiBody != "" {
649648
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",
650-
err, r.StatusCode, string(bodyBytes),
649+
err, r.StatusCode, apiBody,
651650
filterPattern,
652651
time.Unix(from, 0).Format(time.RFC3339), from)
653652
}
@@ -677,11 +676,11 @@ func runMetricsMetadataGet(cmd *cobra.Command, args []string) error {
677676

678677
resp, r, err := api.GetMetricMetadata(client.Context(), metricName)
679678
if err != nil {
680-
if r != nil && r.Body != nil {
681-
bodyBytes, readErr := io.ReadAll(r.Body)
682-
if readErr == nil && len(bodyBytes) > 0 {
679+
if r != nil {
680+
apiBody := extractAPIErrorBody(err)
681+
if apiBody != "" {
683682
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",
684-
err, r.StatusCode, string(bodyBytes), metricName)
683+
err, r.StatusCode, apiBody, metricName)
685684
}
686685
return fmt.Errorf("failed to get metric metadata: %w (status: %d)", err, r.StatusCode)
687686
}
@@ -733,11 +732,11 @@ func runMetricsMetadataUpdate(cmd *cobra.Command, args []string) error {
733732

734733
resp, r, err := api.UpdateMetricMetadata(client.Context(), metricName, body)
735734
if err != nil {
736-
if r != nil && r.Body != nil {
737-
bodyBytes, readErr := io.ReadAll(r.Body)
738-
if readErr == nil && len(bodyBytes) > 0 {
735+
if r != nil {
736+
apiBody := extractAPIErrorBody(err)
737+
if apiBody != "" {
739738
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",
740-
err, r.StatusCode, string(bodyBytes), metricName)
739+
err, r.StatusCode, apiBody, metricName)
741740
}
742741
return fmt.Errorf("failed to update metric metadata: %w (status: %d)", err, r.StatusCode)
743742
}
@@ -833,11 +832,11 @@ func runMetricsSubmit(cmd *cobra.Command, args []string) error {
833832

834833
resp, r, err := api.SubmitMetrics(client.Context(), body, *datadogV2.NewSubmitMetricsOptionalParameters())
835834
if err != nil {
836-
if r != nil && r.Body != nil {
837-
bodyBytes, readErr := io.ReadAll(r.Body)
838-
if readErr == nil && len(bodyBytes) > 0 {
835+
if r != nil {
836+
apiBody := extractAPIErrorBody(err)
837+
if apiBody != "" {
839838
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",
840-
err, r.StatusCode, string(bodyBytes),
839+
err, r.StatusCode, apiBody,
841840
submitName, submitValue, submitType, timestamp, tags)
842841
}
843842
return fmt.Errorf("failed to submit metrics: %w (status: %d)", err, r.StatusCode)

‎cmd/root.go‎

Lines changed: 27 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -7,11 +7,13 @@ package cmd
77

88
import (
99
"bufio"
10+
"errors"
1011
"fmt"
1112
"io"
1213
"os"
1314
"strings"
1415

16+
"github.com/DataDog/datadog-api-client-go/v2/api/datadog"
1517
"github.com/DataDog/pup/internal/version"
1618
"github.com/DataDog/pup/pkg/client"
1719
"github.com/DataDog/pup/pkg/config"
@@ -259,7 +261,26 @@ func readConfirmation() (string, error) {
259261
return "", scanner.Err()
260262
}
261263

262-
// formatAPIError creates user-friendly error messages for API errors
264+
// extractAPIErrorBody extracts the API response body from a
265+
// datadog.GenericOpenAPIError. The datadog-api-client-go library consumes
266+
// http.Response.Body during deserialization and stores the bytes in the error.
267+
// Callers that try to re-read http.Response.Body will always get empty data.
268+
func extractAPIErrorBody(err error) string {
269+
if err == nil {
270+
return ""
271+
}
272+
var apiErr datadog.GenericOpenAPIError
273+
if errors.As(err, &apiErr) {
274+
if body := apiErr.Body(); len(body) > 0 {
275+
return string(body)
276+
}
277+
}
278+
return ""
279+
}
280+
281+
// formatAPIError creates user-friendly error messages for API errors.
282+
// It extracts the API response body from GenericOpenAPIError when available
283+
// and appends contextual guidance based on the HTTP status code.
263284
func formatAPIError(operation string, err error, response any) error {
264285
type httpResponse interface {
265286
StatusCode() int
@@ -269,6 +290,11 @@ func formatAPIError(operation string, err error, response any) error {
269290
statusCode := r.StatusCode()
270291
baseMsg := fmt.Sprintf("failed to %s: %v (status: %d)", operation, err, statusCode)
271292

293+
// Include API response body if available
294+
if body := extractAPIErrorBody(err); body != "" {
295+
baseMsg = fmt.Sprintf("failed to %s: %v (status: %d)\nAPI Response: %s", operation, err, statusCode, body)
296+
}
297+
272298
switch {
273299
case statusCode >= 500:
274300
// 5xx Server errors

‎cmd/root_test.go‎

Lines changed: 83 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -7,9 +7,11 @@ package cmd
77

88
import (
99
"errors"
10+
"fmt"
1011
"strings"
1112
"testing"
1213

14+
"github.com/DataDog/datadog-api-client-go/v2/api/datadog"
1315
"github.com/DataDog/pup/pkg/config"
1416
)
1517

@@ -209,7 +211,7 @@ func TestFormatAPIError_AllStatusCodes(t *testing.T) {
209211
}
210212

211213
for _, tt := range statusTests {
212-
t.Run(string(rune(tt.code)), func(t *testing.T) {
214+
t.Run(fmt.Sprintf("%d", tt.code), func(t *testing.T) {
213215
err := formatAPIError("test operation", errors.New("test error"), &mockHTTPResponse{statusCode: tt.code})
214216

215217
if err == nil {
@@ -318,3 +320,83 @@ func TestTestCmd_InvalidSite(t *testing.T) {
318320
t.Errorf("testCmd.RunE() error should mention DD_SITE, got: %v", err)
319321
}
320322
}
323+
324+
func TestExtractAPIErrorBody(t *testing.T) {
325+
tests := []struct {
326+
name string
327+
err error
328+
want string
329+
}{
330+
{
331+
name: "GenericOpenAPIError with body",
332+
err: datadog.GenericOpenAPIError{
333+
ErrorBody: []byte(`{"errors":["Invalid query: avg:nonexistent.metric{*}"]}`),
334+
ErrorMessage: "400 Bad Request",
335+
},
336+
want: `{"errors":["Invalid query: avg:nonexistent.metric{*}"]}`,
337+
},
338+
{
339+
name: "GenericOpenAPIError with empty body",
340+
err: datadog.GenericOpenAPIError{
341+
ErrorBody: []byte{},
342+
ErrorMessage: "400 Bad Request",
343+
},
344+
want: "",
345+
},
346+
{
347+
name: "GenericOpenAPIError with nil body",
348+
err: datadog.GenericOpenAPIError{
349+
ErrorBody: nil,
350+
ErrorMessage: "400 Bad Request",
351+
},
352+
want: "",
353+
},
354+
{
355+
name: "wrapped GenericOpenAPIError",
356+
err: fmt.Errorf("api call failed: %w", datadog.GenericOpenAPIError{
357+
ErrorBody: []byte(`{"errors":["bad query"]}`),
358+
ErrorMessage: "400 Bad Request",
359+
}),
360+
want: `{"errors":["bad query"]}`,
361+
},
362+
{
363+
name: "non-GenericOpenAPIError",
364+
err: errors.New("some other error"),
365+
want: "",
366+
},
367+
{
368+
name: "nil error",
369+
err: nil,
370+
want: "",
371+
},
372+
}
373+
374+
for _, tt := range tests {
375+
t.Run(tt.name, func(t *testing.T) {
376+
got := extractAPIErrorBody(tt.err)
377+
if got != tt.want {
378+
t.Errorf("extractAPIErrorBody() = %q, want %q", got, tt.want)
379+
}
380+
})
381+
}
382+
}
383+
384+
func TestFormatAPIError_IncludesResponseBody(t *testing.T) {
385+
// This test verifies that formatAPIError surfaces the API response body
386+
// from GenericOpenAPIError, which was previously lost because the code
387+
// tried to re-read the already-consumed http.Response.Body.
388+
apiErr := datadog.GenericOpenAPIError{
389+
ErrorBody: []byte(`{"errors":["Query parse error: unknown metric"]}`),
390+
ErrorMessage: "400 Bad Request",
391+
}
392+
393+
err := formatAPIError("query metrics", apiErr, &mockHTTPResponse{statusCode: 400})
394+
errMsg := err.Error()
395+
396+
if !strings.Contains(errMsg, "unknown metric") {
397+
t.Errorf("formatAPIError() should include API response body, got: %q", errMsg)
398+
}
399+
if !strings.Contains(errMsg, "status: 400") {
400+
t.Errorf("formatAPIError() should include status code, got: %q", errMsg)
401+
}
402+
}

0 commit comments

Comments
 (0)