Skip to content

add login logs #76

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

Merged
merged 10 commits into from
May 27, 2023
Merged

add login logs #76

merged 10 commits into from
May 27, 2023

Conversation

wu-clan
Copy link
Member

@wu-clan wu-clan commented May 25, 2023

This is a pre-built version

Please note: This method will cause unnecessary delays in the login api

@wu-clan wu-clan requested a review from downdawn May 25, 2023 15:00
@wu-clan wu-clan marked this pull request as ready for review May 25, 2023 15:01
@wu-clan
Copy link
Member Author

wu-clan commented May 25, 2023

Hi, @downdawn

Routine log entry may lead to unnecessary time loss? What do you think?

@wu-clan
Copy link
Member Author

wu-clan commented May 26, 2023

please review.

@downdawn
Copy link
Collaborator

Hi, @downdawn

Routine log entry may lead to unnecessary time loss? What do you think?

The main delay is caused by the request url, can increase the timeout parameter

Log storage includes operations such as parsing ua and obtaining ip information, which can be extracted into tasks separately.

Consider whether to use BackgroundTasks

@wu-clan
Copy link
Member Author

wu-clan commented May 26, 2023

Okay.

The default timeout is 5s.

wu-clan added 2 commits May 26, 2023 16:17
# Conflicts:
#	backend/app/api/v1/user.py
#	backend/app/crud/crud_user.py
#	backend/app/services/user_service.py
@wu-clan
Copy link
Member Author

wu-clan commented May 26, 2023

Hi, @downdawn

Now it's elegant, please review.

@downdawn
Copy link
Collaborator

LGTM

@wu-clan wu-clan merged commit 4e4c6fb into fastapi-practices:master May 27, 2023
@wu-clan wu-clan deleted the login-log branch May 27, 2023 14:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants