From 2a083faf75b94169efe10a60487f4aa990037a92 Mon Sep 17 00:00:00 2001 From: Dannon Baker Date: Mon, 22 Apr 2024 17:32:26 -0400 Subject: [PATCH 1/7] Adds logging of messageExceptions in the fastapi exception handler. We're intentionally not doing a full log.exception with the traceback here, though we could. This exception will also be dispatched to Sentry if configured, this just makes logs less opaque when one sees a 500. --- lib/galaxy/webapps/base/api.py | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/lib/galaxy/webapps/base/api.py b/lib/galaxy/webapps/base/api.py index df779127f0c3..725d30000b80 100644 --- a/lib/galaxy/webapps/base/api.py +++ b/lib/galaxy/webapps/base/api.py @@ -198,6 +198,10 @@ async def validate_exception_middleware(request: Request, exc: RequestValidation @app.exception_handler(MessageException) async def message_exception_middleware(request: Request, exc: MessageException) -> Response: + # Intentionally not logging traceback here as the full context will be + # dispatched to Sentry if configured. This just makes logs less opaque + # when one sees a 500. + log.error(f"MessageException: {exc}") return get_error_response_for_request(request, exc) From 114803bc9d8ca0947ed4e15b3da05f4fd846b5c0 Mon Sep 17 00:00:00 2001 From: Dannon Baker Date: Mon, 22 Apr 2024 21:32:31 -0400 Subject: [PATCH 2/7] Force MessageExceptions used in workflow modules to have a string error message. --- lib/galaxy/workflow/modules.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/lib/galaxy/workflow/modules.py b/lib/galaxy/workflow/modules.py index ba0692c77d52..6113473c95f4 100644 --- a/lib/galaxy/workflow/modules.py +++ b/lib/galaxy/workflow/modules.py @@ -2557,11 +2557,13 @@ def populate_module_and_state( step_args = param_map.get(step.id, {}) step_errors = module_injector.compute_runtime_state(step, step_args=step_args) if step_errors: - raise exceptions.MessageException(step_errors, err_data={step.order_index: step_errors}) + raise exceptions.MessageException( + "Error computing workflow step runtime state", err_data={step.order_index: step_errors} + ) if step.upgrade_messages: if allow_tool_state_corrections: log.debug('Workflow step "%i" had upgrade messages: %s', step.id, step.upgrade_messages) else: raise exceptions.MessageException( - step.upgrade_messages, err_data={step.order_index: step.upgrade_messages} + "Workflow step has upgrade messages", err_data={step.order_index: step.upgrade_messages} ) From 28f0fc843f1d6773085ac4c1494b5bbbcf7a460e Mon Sep 17 00:00:00 2001 From: Dannon Baker Date: Tue, 23 Apr 2024 08:52:16 -0400 Subject: [PATCH 3/7] Chage MessageException typing to only accept str as err_msg. --- lib/galaxy/exceptions/__init__.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/lib/galaxy/exceptions/__init__.py b/lib/galaxy/exceptions/__init__.py index 5806ed157c21..88513f318a8e 100644 --- a/lib/galaxy/exceptions/__init__.py +++ b/lib/galaxy/exceptions/__init__.py @@ -30,7 +30,7 @@ class MessageException(Exception): # Error code information embedded into API json responses. err_code: ErrorCode = error_codes_by_name["UNKNOWN"] - def __init__(self, err_msg=None, type="info", **extra_error_info): + def __init__(self, err_msg: str = "", type="info", **extra_error_info): self.err_msg = err_msg or self.err_code.default_error_message self.type = type self.extra_error_info = extra_error_info @@ -64,7 +64,7 @@ class AcceptedRetryLater(MessageException): err_code = error_codes_by_name["ACCEPTED_RETRY_LATER"] retry_after: int - def __init__(self, msg, retry_after=60): + def __init__(self, msg: str, retry_after=60): super().__init__(msg) self.retry_after = retry_after @@ -136,7 +136,7 @@ class ToolMissingException(MessageException): status_code = 400 err_code = error_codes_by_name["USER_TOOL_MISSING_PROBLEM"] - def __init__(self, err_msg=None, type="info", tool_id=None, **extra_error_info): + def __init__(self, err_msg: str = "", type="info", tool_id=None, **extra_error_info): super().__init__(err_msg, type, **extra_error_info) self.tool_id = tool_id @@ -152,7 +152,7 @@ class ToolInputsNotReadyException(MessageException): class ToolInputsNotOKException(MessageException): - def __init__(self, err_msg=None, type="info", *, src: str, id: int, **extra_error_info): + def __init__(self, err_msg: str = "", type="info", *, src: str, id: int, **extra_error_info): super().__init__(err_msg, type, **extra_error_info) self.src = src self.id = id From 52a7a144809efed79ec8f32bf100865f640a5dac Mon Sep 17 00:00:00 2001 From: Dannon Baker Date: Tue, 23 Apr 2024 13:39:15 -0400 Subject: [PATCH 4/7] Downgrade message to log.info to avoid sending redundantly to sentry --- lib/galaxy/webapps/base/api.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/galaxy/webapps/base/api.py b/lib/galaxy/webapps/base/api.py index 725d30000b80..22793c85550b 100644 --- a/lib/galaxy/webapps/base/api.py +++ b/lib/galaxy/webapps/base/api.py @@ -201,7 +201,7 @@ async def message_exception_middleware(request: Request, exc: MessageException) # Intentionally not logging traceback here as the full context will be # dispatched to Sentry if configured. This just makes logs less opaque # when one sees a 500. - log.error(f"MessageException: {exc}") + log.info(f"MessageException: {exc}") return get_error_response_for_request(request, exc) From 1c1317be24ec24c2395986820592c186b2467bd9 Mon Sep 17 00:00:00 2001 From: Dannon Baker Date: Tue, 23 Apr 2024 13:41:43 -0400 Subject: [PATCH 5/7] Only log if it's a 500 --- lib/galaxy/webapps/base/api.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/lib/galaxy/webapps/base/api.py b/lib/galaxy/webapps/base/api.py index 22793c85550b..3da3ba3d3900 100644 --- a/lib/galaxy/webapps/base/api.py +++ b/lib/galaxy/webapps/base/api.py @@ -201,7 +201,8 @@ async def message_exception_middleware(request: Request, exc: MessageException) # Intentionally not logging traceback here as the full context will be # dispatched to Sentry if configured. This just makes logs less opaque # when one sees a 500. - log.info(f"MessageException: {exc}") + if exc.status_code >= 500: + log.info(f"MessageException: {exc}") return get_error_response_for_request(request, exc) From e2798c9ca8c435850e47b773fb9aa90e7827635e Mon Sep 17 00:00:00 2001 From: Dannon Date: Wed, 24 Apr 2024 11:33:31 -0400 Subject: [PATCH 6/7] Apply suggestions from code review Co-authored-by: Marius van den Beek --- lib/galaxy/exceptions/__init__.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/lib/galaxy/exceptions/__init__.py b/lib/galaxy/exceptions/__init__.py index 88513f318a8e..d167a2e1fd00 100644 --- a/lib/galaxy/exceptions/__init__.py +++ b/lib/galaxy/exceptions/__init__.py @@ -30,7 +30,7 @@ class MessageException(Exception): # Error code information embedded into API json responses. err_code: ErrorCode = error_codes_by_name["UNKNOWN"] - def __init__(self, err_msg: str = "", type="info", **extra_error_info): + def __init__(self, err_msg: Optional[str] = None, type="info", **extra_error_info): self.err_msg = err_msg or self.err_code.default_error_message self.type = type self.extra_error_info = extra_error_info @@ -64,7 +64,7 @@ class AcceptedRetryLater(MessageException): err_code = error_codes_by_name["ACCEPTED_RETRY_LATER"] retry_after: int - def __init__(self, msg: str, retry_after=60): + def __init__(self, msg: Optional[str] = None, retry_after=60): super().__init__(msg) self.retry_after = retry_after @@ -136,7 +136,7 @@ class ToolMissingException(MessageException): status_code = 400 err_code = error_codes_by_name["USER_TOOL_MISSING_PROBLEM"] - def __init__(self, err_msg: str = "", type="info", tool_id=None, **extra_error_info): + def __init__(self, err_msg: Optional[str] = None, type="info", tool_id=None, **extra_error_info): super().__init__(err_msg, type, **extra_error_info) self.tool_id = tool_id @@ -152,7 +152,7 @@ class ToolInputsNotReadyException(MessageException): class ToolInputsNotOKException(MessageException): - def __init__(self, err_msg: str = "", type="info", *, src: str, id: int, **extra_error_info): + def __init__(self, err_msg: Optional[str] = None, type="info", *, src: str, id: int, **extra_error_info): super().__init__(err_msg, type, **extra_error_info) self.src = src self.id = id From e5c59e1e5eb6d656e25a1f072a5fc48d7f5507e2 Mon Sep 17 00:00:00 2001 From: Dannon Baker Date: Wed, 24 Apr 2024 11:34:52 -0400 Subject: [PATCH 7/7] Fix imports after suggestions from code review. --- lib/galaxy/exceptions/__init__.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/lib/galaxy/exceptions/__init__.py b/lib/galaxy/exceptions/__init__.py index d167a2e1fd00..8deab77bc17b 100644 --- a/lib/galaxy/exceptions/__init__.py +++ b/lib/galaxy/exceptions/__init__.py @@ -16,6 +16,8 @@ and messages. """ +from typing import Optional + from .error_codes import ( error_codes_by_name, ErrorCode,