-
Notifications
You must be signed in to change notification settings - Fork 88
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
*: improve logging and error handling for exits #3347
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3347 +/- ##
==========================================
+ Coverage 58.69% 59.65% +0.96%
==========================================
Files 210 210
Lines 30290 30289 -1
==========================================
+ Hits 17778 18069 +291
+ Misses 10614 10325 -289
+ Partials 1898 1895 -3 ☔ View full report in Codecov by Sentry. |
app/obolapi/api.go
Outdated
if res.StatusCode/100 != 2 { | ||
data, err := io.ReadAll(res.Body) | ||
if err != nil { | ||
return errors.Wrap(err, "read POST response") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be great to catch the status code as well as headers when an error occurs, or maybe log them as debug. Also, verbose logging may be printing the body as base64 or json (depending on content-type). I remember I needed this a few times and patched this code locally to do so.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be great to catch the status code as well as headers when an error occurs, or maybe log them as debug.
Indeed! Added it.
Also, verbose logging may be printing the body as base64 or json (depending on content-type).
What do you mean here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I realized we don't have verbose logging level, debug is the minimum.
I meant the ability to intercept HTTP requests and responses (bodies) when we need to intercept traffic between stack components.. in addition to HTTP status code, URL and Headers. To see the full picture.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I see now. Yes having some sort of a "trace" logging would help in a lot of scenarios I believe. We can probably add it on the roadmap to add it throughout the project?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe if your Wireshark experience was successful then we don't need to add anything here. Anyway this out of scope for the current work.
Quality Gate passedIssues Measures |
Add more logging and more detailed errors for exits. Given that the exits are fire and forget operation and not a long running process, we can afford more details without the issue of polluting with too much.
category: feature
ticket: #3136