-
Notifications
You must be signed in to change notification settings - Fork 0
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
Update API entrypoint and add logger configuration #62
Conversation
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.
Look good. Just a couple questions from me.
app/api/main/app.py
Outdated
@@ -5,10 +5,13 @@ | |||
|
|||
from fastapi import Depends, FastAPI, HTTPException | |||
from fastapi.middleware.cors import CORSMiddleware | |||
import logging |
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.
Is there a reason we need these lines in app.py if we're not logging anything from the file?
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.
maybe it is needed because this is the entry point of the app? Did you test without it and it failed?
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 removed the entry point logger, which works with the customer format elsewhere. Since this log config is passed to uvicorn at the start the logger inherits the config in different modules in the API.
app/api/models/patient_encounter.py
Outdated
@@ -102,8 +104,7 @@ def get_latest_patient_encounter_by_patient_rfid( | |||
standalone_argon2.exceptions.VerificationError, | |||
standalone_argon2.exceptions.InvalidHash, | |||
) as e: | |||
# TODO: Should do something intelligent like logging this. | |||
continue | |||
logger.error(e) |
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.
do we want to add exc_info=True? I am not sure if we do but something to consider.
reference: https://www.loggly.com/blog/exceptional-logging-of-exceptions-in-python/#:~:text=Setting%20exc_info%20to%20True%20will,than%20error%3A%20Just%20replace%20logger.
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 looked into that flag and I think it is worth putting in. I added it in the latest commit.
Problem
The API has no logging for developers to use.
Solution
api/logger_config.yaml
with configurations for formatters and loggersdocker-compose.yaml
servicesapi
andapitarget
commands to useapi/logger_config.yaml
api/models/patient_encounter.py
and completed TODOTicket URL
https://mediform.atlassian.net/browse/MEDI-15
Documentation
N/A
Tests Run
api
image and spun up container.patient_encounter.py
and line was represented in logs. Line removed for production.