-
Notifications
You must be signed in to change notification settings - Fork 682
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
v2:interceptors/logging: allow to separate request response payload logging #442
v2:interceptors/logging: allow to separate request response payload logging #442
Conversation
We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google. ℹ️ Googlers: Go here for more info. |
@googlebot I fixed it. |
We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google. ℹ️ Googlers: Go here for more info. |
1c3e861
to
5bcaa5b
Compare
@googlebot I fixed it. |
5bcaa5b
to
ebc4206
Compare
Codecov Report
@@ Coverage Diff @@
## v2 #442 +/- ##
===========================================
- Coverage 84.01% 58.49% -25.52%
===========================================
Files 30 38 +8
Lines 932 1706 +774
===========================================
+ Hits 783 998 +215
- Misses 110 636 +526
- Partials 39 72 +33
Continue to review full report at Codecov.
|
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 quite like this, but it was @bwplotka who refactored these, so I would prefer to have his input.
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.
Functionally - I think using a switch case would make this code easier to read and maintainable. Otherwise looks good to me!
ebc4206
to
e0ec88c
Compare
Hi @michaljemala ! This PR looks good to me, can you run |
cc'ing @bwplotka for the inputs 😄 |
685607e
to
e75d159
Compare
Seems like the CI build needs approval for first time contributors - https://github.blog/changelog/2021-04-22-github-actions-maintainers-must-approve-first-time-contributor-workflow-runs/ |
Hi @yashrsharma44 is there anything else on my side to be done? |
We are blocked on @bwplotka's review. Let me get back to him when he is free. |
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.
LGTM, only minor nit and lint error 🤗
Thanks!
interceptors/logging/logging.go
Outdated
// NoPayloadLogging - Payload logging is disabled. | ||
NoPayloadLogging PayloadDecision = iota | ||
// LogRequest - Only logging of requests is enabled. | ||
LogRequest |
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.
LogRequest | |
LogPayloadRequest |
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.
Done.
interceptors/logging/logging.go
Outdated
// LogRequest - Only logging of requests is enabled. | ||
LogRequest | ||
// LogResponse - Only logging of responses is enabled. | ||
LogResponse |
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.
LogResponse | |
LogPayloadResponse |
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.
Done.
interceptors/logging/logging.go
Outdated
// LogResponse - Only logging of responses is enabled. | ||
LogResponse | ||
// LogRequestAndResponse - Logging of both requests and responses is enabled. | ||
LogRequestAndResponse |
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.
LogRequestAndResponse | |
LogPayloadRequestAndResponse |
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.
Done.
…ponse payload logging
e75d159
to
9f260f9
Compare
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.
Thanks!
Logging payloads of requests on the server-side and similarly of responses on the client-side is a pretty common and desired use-case. However, logging by default payloads of both requests as well as responses in both scenarios is IMO not a very common requirement.
Consider the following PR to enable more granular control of what to log and when. This is achieved by using a special
PayloadDecison
type having the following values:NoPayloadLogging
- Payload logging is disabled.LogRequest
- Only logging of requests is enabled.LogResponse
- Only logging of responses is enabled.LogRequestAndResponse
- Logging of both requests and responses is enabled.The
PayloadDecison
is to be used instead of the originalbool
by thelogging.ServerPayloadLoggingDecider
andlogging.ClientPayloadLoggingDecider
.NOTE: I quickly put this together to see whether it could be considered a valid approach, thus please excuse the lack of additional unit tests. I will be glad to add those once the approach is approved.