Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore: include request id in debug logs and diagnostics #581

Closed
wants to merge 11 commits into from
+21 −11
Closed

chore: include request id in debug logs and diagnostics #581

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
5f7cd41
chore: include request id in debug logs and diagnostics
cewkrupa Nov 20, 2024
65ea57b
woops don't commit that
cewkrupa Nov 20, 2024
e2c901b
remove spaces
cewkrupa Nov 20, 2024
af49bec
update comment
cewkrupa Nov 20, 2024
4a6eb63
remove dependency from testing
cewkrupa Nov 20, 2024
1433b23
add failing test for example
cewkrupa Nov 20, 2024
0ddd75a
populate request id
cewkrupa Nov 20, 2024
c81e65d
include request id in test errors
cewkrupa Nov 20, 2024
272be03
what if we just turn debug on in tests always
cewkrupa Nov 20, 2024
bd2a59e
update v2 client
cewkrupa Nov 20, 2024
17f6cf7
Merge branch 'main' into cewkrupa/include-request-id-in-errors
jharley Dec 6, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 1 addition & 2 deletions client/client.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -138,10 +138,9 @@ func NewClientWithConfig(config *Config) (*Client, error) {
}

if config.Debug {
// if enabled we log all requests and responses to sterr
client.httpClient.Logger = log.New(os.Stderr, "", log.LstdFlags)
client.httpClient.ResponseLogHook = func(l retryablehttp.Logger, resp *http.Response) {
l.Printf("[DEBUG] Response: %s %s", resp.Request.Method, resp.Request.URL.String())
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I changed this to say "Request" because we weren't actually logging much about the response, just the request. I added the request ID and the status as well.

l.Printf("[DEBUG] %s \"%s\" %s %s", resp.Request.Method, resp.Request.URL.String(), resp.Status, resp.Header.Get("Request-Id"))
}
}

Expand Down
3 changes: 1 addition & 2 deletions client/client_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,11 +24,10 @@ func newTestClient(t *testing.T) *client.Client {
if !ok {
t.Fatal("expected environment variable " + client.DefaultAPIKeyEnv)
}
_, debug := os.LookupEnv("HONEYCOMBIO_DEBUG")

c, err := client.NewClientWithConfig(&client.Config{
APIKey: apiKey,
Debug: debug,
Debug: true,
UserAgent: testUserAgent,
})
require.NoError(t, err, "failed to create client")
Expand Down
16 changes: 14 additions & 2 deletions client/errors.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,8 @@ type DetailedError struct {
Status int `json:"status,omitempty"`
// The error message
Message string `json:"error,omitempty"`
// ID is the unique ID of the HTTP request that caused this error.
ID string `json:"request_id,omitempty"`
// Type is a URI used to uniquely identify the type of error.
Type string `json:"type,omitempty"`
// Title is a human-readable summary that explains the type of the problem.
Expand Down Expand Up @@ -63,9 +65,8 @@ func (e *DetailedError) IsNotFound() bool {

// Error returns a pretty-printed representation of the error
func (e DetailedError) Error() string {
response := ""
if len(e.Details) > 0 {
var response string

for index, details := range e.Details {
response += details.String()

Expand All @@ -87,6 +88,8 @@ func ErrorFromResponse(r *http.Response) error {
return errors.New("invalid response")
}

requestID := r.Header.Get("Request-Id")

switch r.Header.Get("Content-Type") {
case jsonapi.MediaType:
var detailedError DetailedError
Expand All @@ -97,10 +100,12 @@ func ErrorFromResponse(r *http.Response) error {
return DetailedError{
Status: r.StatusCode,
Message: r.Status,
ID: requestID,
}
}

detailedError = DetailedError{
ID: requestID,
Status: r.StatusCode,
Title: errPayload.Errors[0].Title,
}
Expand Down Expand Up @@ -130,6 +135,7 @@ func ErrorFromResponse(r *http.Response) error {
if err := json.NewDecoder(r.Body).Decode(&detailedError); err != nil {
// If we can't decode the error, return a generic error
return DetailedError{
ID: requestID,
Status: r.StatusCode,
Message: r.Status,
}
Expand All @@ -139,6 +145,12 @@ func ErrorFromResponse(r *http.Response) error {
if detailedError.Status == 0 {
detailedError.Status = r.StatusCode
}

// ensure we have the requestID set
if detailedError.ID == "" {
detailedError.ID = requestID
}

return detailedError
}
}
Expand Down
1 change: 1 addition & 0 deletions client/marker_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,7 @@ func TestMarkers(t *testing.T) {
_, err := c.Markers.Get(ctx, dataset, m.ID)

var de client.DetailedError
require.NoError(t, err)
require.Error(t, err)
require.ErrorAs(t, err, &de)
assert.True(t, de.IsNotFound())
Expand Down
4 changes: 1 addition & 3 deletions client/v2/client.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,11 +104,9 @@ func NewClientWithConfig(config *Config) (*Client, error) {
}

if config.Debug {
// if enabled we log all requests and responses to sterr
client.http.Logger = log.New(os.Stderr, "", log.LstdFlags)
client.http.ResponseLogHook = func(l retryablehttp.Logger, resp *http.Response) {
l.Printf("[DEBUG] Request: %s %s", resp.Request.Method, resp.Request.URL.String())
// TODO: Log request body
l.Printf("[DEBUG] %s \"%s\" %s %s", resp.Request.Method, resp.Request.URL.String(), resp.Status, resp.Header.Get("Request-Id"))
}
}

Expand Down
1 change: 1 addition & 0 deletions honeycombio/type_helpers.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -251,6 +251,7 @@ func diagFromDetailedErr(err honeycombio.DetailedError) diag.Diagnostics {
detail += d.Field + " "
}
detail += d.Description
detail += fmt.Sprintf(" - ID: %s", err.ID)

diags = append(diags, diag.Diagnostic{
Severity: diag.Error,
Expand Down
4 changes: 2 additions & 2 deletions internal/helper/diag.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,8 +53,8 @@ func AddDiagnosticOnError(diag *diag.Diagnostics, summary string, err error) boo
}

func (d DetailedErrorDiagnostic) Detail() string {
response := fmt.Sprintf("ID: %s\n", d.e.ID)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We're doing some confusing things with looping over the Details of the error to generate the message, and it's otherwise not super consistent as far as I can tell. Super open to other ideas of how to format this.

if len(d.e.Details) > 0 {
var response string
for i, dt := range d.e.Details {
response += dt.Code + " - " + dt.Description

Expand All @@ -65,7 +65,7 @@ func (d DetailedErrorDiagnostic) Detail() string {
return response
}

return d.e.Message
return response + d.e.Message
}

func (d DetailedErrorDiagnostic) Summary() string {
Expand Down
Loading