-
Notifications
You must be signed in to change notification settings - Fork 612
Revamp debug logs #816
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
Revamp debug logs #816
Conversation
|
cc @ahmetb |
b12fbd3 to
5774bd5
Compare
Codecov Report
@@ Coverage Diff @@
## master #816 +/- ##
==========================================
- Coverage 74.77% 74.69% -0.08%
==========================================
Files 103 103
Lines 4313 4339 +26
==========================================
+ Hits 3225 3241 +16
- Misses 611 616 +5
- Partials 477 482 +5
Continue to review full report at Codecov.
|
5774bd5 to
025662c
Compare
|
I found req/resp dumps very useful. so I was doing something like this to download a layer into a file.
and |
|
Trying to figure out a good way to actually filter out stuff you don't care about. We don't want to log the token response and we don't want to log binary blobs, but it's impossible to determine those from just the request or response in this context. We could do something janky like modify the context to tell the logger not to log certain requests... |
025662c to
8a8877e
Compare
|
I've implemented my janky suggestion to thread this through Note some things:
Nicely, config blob is not omitted: |
|
cc @mattmoor this also includes timing info from the transport, which you may find useful |
1. Drop places that can log creds. May bring this back with build tags? 2. Introduce pkg/internal/redact to hold a context that signals we should omit the body when dumping logs. Maybe this should live in the logs package, but internal for now should be fine. 3. Stop logging the request and response body for blobs and token responses.
8a8877e to
c690162
Compare
should omit the body when dumping logs. Maybe this should live in
the logs package, but internal for now should be fine.
responses.