-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
API request logging improvements #3180
Conversation
/retest |
Codecov Report
@@ Coverage Diff @@
## master #3180 +/- ##
==========================================
+ Coverage 54.52% 54.58% +0.06%
==========================================
Files 565 565
Lines 12397 12415 +18
==========================================
+ Hits 6759 6777 +18
- Misses 5376 5377 +1
+ Partials 262 261 -1
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.
Few minor comments to the code, I will test it later. Thanks for taking care of it :)
src/app/backend/handler/filter.go
Outdated
@@ -76,6 +83,19 @@ func formatRequestLog(request *restful.Request) string { | |||
} | |||
} | |||
|
|||
// Is DEBUG level logging enabled? | |||
if args.Holder.GetAPILogLevel() != "DEBUG" { | |||
// Great now let's filter out any content from sensitive URLs |
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 would suggest is to export it to another method. You can also use map instead of slice to get rid of the loop:
_, isSensitive := sensitiveUrls[key]
You can put struct{}
as a value to save space.
Please remember to use camelCase and switch likes of sensitive_urls
to sensitiveUrls
.
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.
Addressed and pushed to c820847 -- Thoughts?
If that looks good, I can squash before the merge.
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.
Updated in commit c820847
src/app/backend/handler/filter.go
Outdated
@@ -76,6 +83,19 @@ func formatRequestLog(request *restful.Request) string { | |||
} | |||
} | |||
|
|||
// Is DEBUG level logging enabled? | |||
if args.Holder.GetAPILogLevel() != "DEBUG" { | |||
// Great now let's filter out any content from sensitive URLs |
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.
Addressed and pushed to c820847 -- Thoughts?
If that looks good, I can squash before the merge.
/lgtm It seems to work, but I think it should be possible to use this flag also when running |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jeefy, maciaszczykm The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Hey all!
This directly addresses #3174, #3012, and potentially others.
I created a list of addresses that may potentially have sensitive information that should not be logged. Instead it will only log
{ contents hidden }
.However, in some cases (like debugging and development) you still want to see what's being passed along. So I added a new CLI argument to the dashboard called
--api-log-level
with three options: INFO,DEBUG,NONEThe default setting is INFO, which will log everything but sensitive URLs.
NONE will turn off all request/response logging.
DEBUG will log everything including the contents of sensitive URLs.
I added tests for all this, and also updated
TestFormatRequestLog
to handle more diverse test cases.Thanks!