-
-
Notifications
You must be signed in to change notification settings - Fork 327
[16.0] [ADD] fastapi_log #501
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
base: 16.0
Are you sure you want to change the base?
Conversation
Thanks for working on this. I wanted to do it since a while 😅 Early feedback... The way I was planning to handle this was to have a base module to collect any kind of "request/response log". Would you consider splitting this part to a module like |
@simahawk for now I don't want to overengineer this module. But feel free to split it when done. As a side note about that, is there any advantage to have all heterogeneous logs in the same model/views? Because aside of that, the amount of factored code would be low. |
Hi @paradoxxxzero, thanks for this PR! |
Hello, no we have it deployed, I just forgot to remove the draft status, thanks. |
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.
Code review
fastapi_endpoint = ( | ||
self.request.env["fastapi.endpoint"] | ||
.sudo() | ||
.search([("root_path", "=", root_path)]) | ||
) |
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.
question: what do you think about calling the get_uid
method of fastapi.endpoint
?
This way, if we need to override the method, the same endpoint will be matched
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.
get_uid
would return the user ID, could you please clarify see how that could be used in this piece of code?
In akretion#8 I have included your PR #515 and used the new get_endpoint
, is that what you meant?
|
||
def _headers_to_dict(self, headers): | ||
try: | ||
return {key.lower(): value for key, value in headers.items()} |
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.
issue: we should redact sentitive headers
rest-framework/rest_log/components/service.py
Lines 130 to 132 in e0e48e3
@property | |
def _log_call_header_strip(self): | |
return ("Cookie", "Api-Key") |
rest-framework/base_rest/http.py
Lines 109 to 111 in e0e48e3
for k in params.keys(): | |
if k in BLACKLISTED_LOG_PARAMS: | |
params[k] = "<redacted>" |
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.
Right, added this too in akretion#8.
This is not a migration of rest_log, this module aims to be simpler, at least in the first iterations.
This PR depends on #500