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.

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

Draft
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

cewkrupa
Copy link
Contributor

@cewkrupa cewkrupa commented Nov 20, 2024

Which problem is this PR solving?

When calls to the Honeycomb API fail in tests and in real operation, we don't get much detail about what's wrong - just an error message if we're lucky. We've recently added a Request-Id header to our API responses to aid in debugging.

Short description of the changes

This PR pulls the Request-Id header of failed requests into the error messages, so they'll show up in test failures, in debug mode, and in regular operation.

How to verify that this has the expected result

@codecov-commenter
Copy link

Codecov Report

Attention: Patch coverage is 44.44444% with 5 lines in your changes missing coverage. Please review.

Project coverage is 75.61%. Comparing base (fdc4317) to head (65ea57b).

Files with missing lines Patch % Lines
internal/helper/diag.go 0.00% 2 Missing ⚠️
client/client.go 0.00% 1 Missing ⚠️
client/errors.go 80.00% 1 Missing ⚠️
honeycombio/type_helpers.go 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #581      +/-   ##
==========================================
- Coverage   75.62%   75.61%   -0.01%     
==========================================
  Files          87       87              
  Lines        7277     7283       +6     
==========================================
+ Hits         5503     5507       +4     
- Misses       1423     1425       +2     
  Partials      351      351              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.


🚨 Try these New Features:

@@ -141,7 +141,7 @@ func NewClientWithConfig(config *Config) (*Client, error) {
// 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.

@@ -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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants