Conversation
app/endpoints/task.py
Outdated
| count=accounting_info.parameters.count, | ||
| project_id=user_context.project_id, | ||
| ) | ||
| accounting_session = None |
There was a problem hiding this comment.
accounting should be handling the null case with the null session. We shouldn't have a None default.
app/endpoints/task.py
Outdated
| activity_id=activity_id, | ||
| task_definition=TASK_DEFINITIONS[task_type], | ||
| ) | ||
| L.info(f"Task failure callback received: task_type={task_type}, activity_id={activity_id}") |
There was a problem hiding this comment.
When logging a service it would be better to only log warnings or unexpected code paths that would help with debugging, otherwise the server can be drown in trivial logs generated with each request.
There was a problem hiding this comment.
Ok, I see! I removed all trivial info logging.
app/endpoints/task.py
Outdated
| try: | ||
| task_service.handle_task_failure_callback( | ||
| db_client=db_client, | ||
| activity_id=activity_id, | ||
| task_definition=TASK_DEFINITIONS[task_type], | ||
| ) | ||
| except Exception as exc: | ||
| L.error( | ||
| f"Failed to process task failure callback for activity {activity_id}: {exc}", | ||
| exc_info=True, | ||
| ) | ||
| raise ApiError( | ||
| message=f"Failed to process task failure callback: {exc}", | ||
| http_status_code=HTTPStatus.INTERNAL_SERVER_ERROR, | ||
| error_code=ApiErrorCode.INTERNAL_ERROR, | ||
| ) from exc |
There was a problem hiding this comment.
Is this necessary? That part of the code has only entitysdk calls which map automatically to ApiError in app/application.py
async def entity_sdk_error_handler(request: Request, exc: EntitySDKError) -> None:
"""Handle database client errors globally.
Allows using the sdk anywhere in the service without having to handle EntitySDKError that are
emitted explicitly if not needed.
"""
L.exception(
"EntitySDKError in %s %s",
request.method,
request.url,
)
raise ApiError(
message=str(exc),
error_code=ApiErrorCode.DATABASE_CLIENT_ERROR,
http_status_code=HTTPStatus.INTERNAL_SERVER_ERROR,
) from exc
app = FastAPI(
title=settings.APP_NAME,
version=settings.APP_VERSION or "0.0.0",
debug=settings.APP_DEBUG,
lifespan=lifespan,
exception_handlers={
ApiError: api_error_handler,
RequestValidationError: validation_exception_handler,
EntitySDKError: entity_sdk_error_handler,
},
root_path=settings.ROOT_PATH,
)
There was a problem hiding this comment.
Ok, reverted this part
| project_id=user_context.project_id, | ||
| ) | ||
| accounting_session = None | ||
| try: |
There was a problem hiding this comment.
Instead of trying to capture everything, each service func should have its try/excepts, which is the case for make_task_reservation for example.
There was a problem hiding this comment.
Added separate try/except blocks for all service calls
app/endpoints/task.py
Outdated
| L.info( | ||
| f"Task launch request: task_type={json_model.task_type}, config_id={json_model.config_id}" | ||
| ) |
There was a problem hiding this comment.
Same as below. No need for trivial logging that add noise to the server logs.
There was a problem hiding this comment.
Ok, I removed all trivial info logging.
2 participants
Removed default URLs from settings