From 6ae626a5d5ac839357273df1a449ecf2beb9a316 Mon Sep 17 00:00:00 2001 From: Nathan Sprenkle Date: Fri, 11 Feb 2022 13:56:25 -0500 Subject: [PATCH] feat: ESG logging updates (#29895) * feat: log exceptions for fall-through exceptions * chore: remove unused XBlock handler * refactor: log errors for XBlock handlers * refactor: log bad ORA locations * feat: log lock/grade contest errors * feat: log output data for ESG endpoints * feat: unpack extra error info from more endpoints --- lms/djangoapps/ora_staff_grader/errors.py | 3 + lms/djangoapps/ora_staff_grader/ora_api.py | 37 ++++------ lms/djangoapps/ora_staff_grader/utils.py | 11 ++- lms/djangoapps/ora_staff_grader/views.py | 78 +++++++++++++++------- 4 files changed, 80 insertions(+), 49 deletions(-) diff --git a/lms/djangoapps/ora_staff_grader/errors.py b/lms/djangoapps/ora_staff_grader/errors.py index 801ecc32da..de43121ced 100644 --- a/lms/djangoapps/ora_staff_grader/errors.py +++ b/lms/djangoapps/ora_staff_grader/errors.py @@ -23,6 +23,9 @@ class ExceptionWithContext(Exception): class XBlockInternalError(ExceptionWithContext): """Errors from XBlock handlers""" + def __str__(self): + return str(self.context) + class LockContestedError(ExceptionWithContext): """Signal for trying to operate on a lock owned by someone else""" diff --git a/lms/djangoapps/ora_staff_grader/ora_api.py b/lms/djangoapps/ora_staff_grader/ora_api.py index b245f06524..73c0865e6c 100644 --- a/lms/djangoapps/ora_staff_grader/ora_api.py +++ b/lms/djangoapps/ora_staff_grader/ora_api.py @@ -17,7 +17,7 @@ from lms.djangoapps.ora_staff_grader.errors import ( XBlockInternalError, ) -from lms.djangoapps.ora_staff_grader.utils import call_xblock_json_handler +from lms.djangoapps.ora_staff_grader.utils import call_xblock_json_handler, is_json def get_submissions(request, usage_id): @@ -33,29 +33,6 @@ def get_submissions(request, usage_id): return json.loads(response.content) -def get_rubric_config(request, usage_id): - """ - Get rubric data from the ORA's 'get_rubric' XBlock.json_handler - """ - handler_name = "get_rubric" - data = {"target_rubric_block_id": usage_id} - response = call_xblock_json_handler(request, usage_id, handler_name, data) - - # Unhandled errors might not be JSON, catch before loading - if response.status_code != 200: - raise XBlockInternalError(context={"handler": handler_name}) - - response_data = json.loads(response.content) - - # Handled faillure still returns HTTP 200 but with 'success': False and supplied error message "msg" - if not response_data.get("success", False): - raise XBlockInternalError( - context={"handler": handler_name, "msg": response_data.get("msg", "")} - ) - - return response_data["rubric"] - - def get_submission_info(request, usage_id, submission_uuid): """ Get submission content from ORA 'get_submission_info' XBlock.json_handler @@ -65,6 +42,11 @@ def get_submission_info(request, usage_id, submission_uuid): response = call_xblock_json_handler(request, usage_id, handler_name, data) if response.status_code != 200: + details = ( + json.loads(response.content).get("error", "") + if is_json(response.content) + else "" + ) raise XBlockInternalError(context={"handler": handler_name}) return json.loads(response.content) @@ -79,7 +61,12 @@ def get_assessment_info(request, usage_id, submission_uuid): response = call_xblock_json_handler(request, usage_id, handler_name, data) if response.status_code != 200: - raise XBlockInternalError(context={"handler": handler_name}) + details = ( + json.loads(response.content).get("error", "") + if is_json(response.content) + else "" + ) + raise XBlockInternalError(context={"handler": handler_name, "details": details}) return json.loads(response.content) diff --git a/lms/djangoapps/ora_staff_grader/utils.py b/lms/djangoapps/ora_staff_grader/utils.py index c307f77cf3..3552923808 100644 --- a/lms/djangoapps/ora_staff_grader/utils.py +++ b/lms/djangoapps/ora_staff_grader/utils.py @@ -43,7 +43,16 @@ def require_params(param_names): return decorator -def call_xblock_json_handler(request, usage_id, handler_name, data): +def is_json(input_string): + """Quick True/False check to see if a value is JSON""" + try: + json.loads(input_string) + except ValueError: + return False + return True + + +def call_xblock_json_handler(request, usage_id, handler_name, data, auth=False): """ WARN: Tested only for use in ESG. Consult before use outside of ESG. diff --git a/lms/djangoapps/ora_staff_grader/views.py b/lms/djangoapps/ora_staff_grader/views.py index 655dd9b995..bdad314699 100644 --- a/lms/djangoapps/ora_staff_grader/views.py +++ b/lms/djangoapps/ora_staff_grader/views.py @@ -6,6 +6,7 @@ Views for Enhanced Staff Grader # NOTE: we intentionally add extra args using @require_params # pylint: disable=arguments-differ +import logging from edx_rest_framework_extensions.auth.jwt.authentication import JwtAuthentication from edx_rest_framework_extensions.auth.session.authentication import ( @@ -54,6 +55,8 @@ from openedx.core.djangoapps.content.course_overviews.api import ( ) from openedx.core.lib.api.authentication import BearerAuthenticationAllowInactiveUser +log = logging.getLogger(__name__) + class StaffGraderBaseView(RetrieveAPIView): """ @@ -102,18 +105,23 @@ class InitializeView(StaffGraderBaseView): # Get list of submissions for this ORA init_data["submissions"] = get_submissions(request, ora_location) - return Response(InitializeSerializer(init_data).data) + response_data = InitializeSerializer(init_data).data + log.info(response_data) + return Response(response_data) # Catch bad ORA location except (InvalidKeyError, ItemNotFoundError): + log.error(f"Bad ORA location provided: {ora_location}") return BadOraLocationResponse() # Issues with the XBlock handlers except XBlockInternalError as ex: + log.error(ex) return InternalErrorResponse(context=ex.context) - # Blanket exception handling - except Exception: + # Blanket exception handling in case something blows up + except Exception as ex: + log.exception(ex) return UnknownErrorResponse() @@ -162,22 +170,25 @@ class SubmissionFetchView(StaffGraderBaseView): ) lock_info = check_submission_lock(request, ora_location, submission_uuid) - serializer = SubmissionFetchSerializer( + response_data = SubmissionFetchSerializer( { "submission_info": submission_info, "assessment_info": assessment_info, "lock_info": lock_info, } - ) + ).data - return Response(serializer.data) + log.info(response_data) + return Response(response_data) # Issues with the XBlock handlers except XBlockInternalError as ex: + log.error(ex) return InternalErrorResponse(context=ex.context) - # Blanket exception handling - except Exception: + # Blanket exception handling in case something blows up + except Exception as ex: + log.exception(ex) return UnknownErrorResponse() @@ -217,21 +228,24 @@ class SubmissionStatusFetchView(StaffGraderBaseView): ) lock_info = check_submission_lock(request, ora_location, submission_uuid) - serializer = SubmissionStatusFetchSerializer( + response_data = SubmissionStatusFetchSerializer( { "assessment_info": assessment_info, "lock_info": lock_info, } - ) + ).data - return Response(serializer.data) + log.info(response_data) + return Response(response_data) # Issues with the XBlock handlers except XBlockInternalError as ex: + log.error(ex) return InternalErrorResponse(context=ex.context) - # Blanket exception handling - except Exception: + # Blanket exception handling in case something blows up + except Exception as ex: + log.exception(ex) return UnknownErrorResponse() @@ -291,6 +305,7 @@ class UpdateGradeView(StaffGraderBaseView): "lock_info": lock_info, } ).data + log.error(f"Grade contested for submission: {submission_uuid}") return GradeContestedResponse(context=submission_status) # Transform grade data and submit assessment, rasies on failure @@ -306,20 +321,24 @@ class UpdateGradeView(StaffGraderBaseView): request, ora_location, submission_uuid ) lock_info = check_submission_lock(request, ora_location, submission_uuid) - serializer = SubmissionStatusFetchSerializer( + response_data = SubmissionStatusFetchSerializer( { "assessment_info": assessment_info, "lock_info": lock_info, } - ) - return Response(serializer.data) + ).data + + log.info(response_data) + return Response(response_data) # Issues with the XBlock handlers except XBlockInternalError as ex: + log.error(ex) return InternalErrorResponse(context=ex.context) - # Blanket exception handling - except Exception: + # Blanket exception handling in case something blows up + except Exception as ex: + log.exception(ex) return UnknownErrorResponse() @@ -350,24 +369,31 @@ class SubmissionLockView(StaffGraderBaseView): # Validate ORA location UsageKey.from_string(ora_location) lock_info = claim_submission_lock(request, ora_location, submission_uuid) - return Response(LockStatusSerializer(lock_info).data) + + response_data = LockStatusSerializer(lock_info).data + log.info(response_data) + return Response(response_data) # Catch bad ORA location except (InvalidKeyError, ItemNotFoundError): + log.error(f"Bad ORA location provided: {ora_location}") return BadOraLocationResponse() # Return updated lock info on error except LockContestedError: lock_info = check_submission_lock(request, ora_location, submission_uuid) lock_status = LockStatusSerializer(lock_info).data + log.error(f"Lock contested for submission: {submission_uuid}") return LockContestedResponse(context=lock_status) # Issues with the XBlock handlers except XBlockInternalError as ex: + log.error(ex) return InternalErrorResponse(context=ex.context) # Blanket exception handling - except Exception: + except Exception as ex: + log.exception(ex) return UnknownErrorResponse() @require_params([PARAM_ORA_LOCATION, PARAM_SUBMISSION_ID]) @@ -377,10 +403,14 @@ class SubmissionLockView(StaffGraderBaseView): # Validate ORA location UsageKey.from_string(ora_location) lock_info = delete_submission_lock(request, ora_location, submission_uuid) - return Response(LockStatusSerializer(lock_info).data) + + response_data = LockStatusSerializer(lock_info).data + log.info(response_data) + return Response(response_data) # Catch bad ORA location except (InvalidKeyError, ItemNotFoundError): + log.error(f"Bad ORA location provided: {ora_location}") return BadOraLocationResponse() # Return updated lock info on error @@ -391,8 +421,10 @@ class SubmissionLockView(StaffGraderBaseView): # Issues with the XBlock handlers except XBlockInternalError as ex: + log.error(ex) return InternalErrorResponse(context=ex.context) - # Blanket exception handling - except Exception: + # Blanket exception handling in case something blows up + except Exception as ex: + log.exception(ex) return UnknownErrorResponse()