feat: logging status code and error on the sdk #26
Conversation
WalkthroughThe pull request introduces enhancements to the logging and error handling capabilities across several modules in the Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (4)
- athina_logger/athina_meta.py (2 hunks)
- athina_logger/inference_logger.py (3 hunks)
- athina_logger/openai_wrapper.py (5 hunks)
- athina_logger/request_helper.py (1 hunks)
Additional context used
Ruff
athina_logger/request_helper.py
26-26: Local variable
error_messageis assigned to but never usedRemove assignment to unused variable
error_message(F841)
27-27: Local variable
details_messageis assigned to but never usedRemove assignment to unused variable
details_message(F841)
athina_logger/inference_logger.py
45-46: SyntaxError: Expected an indented block after
exceptclause
Additional comments not posted (15)
athina_logger/athina_meta.py (2)
17-17: LGTM!The addition of the
custom_eval_metricsattribute with the correct type hint looks good.
18-18: LGTM!The addition of the
status_codeattribute with the correct type hint looks good.athina_logger/request_helper.py (2)
20-23: LGTM!The added print statements improve logging capabilities by providing visibility into the response data from the OpenAI API. This can be helpful for debugging purposes.
32-33: Verify the impact of the change in error handling behavior.The change in error handling for
requests.exceptions.RequestExceptionalters the control flow and how errors are communicated to the caller. ReturningNoneinstead of raising the exception could lead to unexpected behavior in the calling code if it is not updated to handle the new behavior.Consider reverting the change to maintain the expected behavior of raising the exception:
except requests.exceptions.RequestException as e: - print(f"Request exception: {e}") - return None + raise eAlternatively, ensure that the calling code is updated to handle the new behavior of returning
Nonein case of arequests.exceptions.RequestException.To verify the impact of this change, run the following script:
athina_logger/inference_logger.py (6)
39-40: LGTM!The addition of
status_codeanderrorparameters to thelog_inferencefunction aligns with the PR objective and allows capturing more details about the inference request. The parameter types are correctly annotated.
43-43: LGTM!The
argstuple is correctly updated to include the newstatus_codeanderrorparameters, which are then passed to the_log_inference_asynchronouslymethod.
49-49: LGTM!The
_log_inference_asynchronouslymethod signature is correctly updated to include the newstatus_codeanderrorparameters, which are then used in the method body to include additional details in the payload.
79-80: LGTM!The payload dictionary is correctly updated to include the new
status_codeand a serialized version of theerrorusing the newly added_serialize_errormethod. This allows for more detailed logging of inference events, capturing the status code and any associated error information.
84-91: LGTM!The inclusion of
traceback.print_exc()in the error handling of the_log_inference_asynchronouslymethod provides more detailed error output in case of failures during the logging operation. This change enhances the debugging experience by providing more context about the errors.
93-117: LGTM!The new
_serialize_errormethod handles various types of error inputs, including dictionaries, strings, and exceptions, to extract and format the error message appropriately. It provides a consistent way to serialize errors for logging.The method correctly handles different scenarios:
- For dictionary inputs, it attempts to extract the inner error message by parsing the string as a dictionary.
- For string and exception inputs, it returns the error message as is.
This change enhances the error handling in the logging process and ensures that the error information is captured in a standardized format.
athina_logger/openai_wrapper.py (5)
Line range hint
33-56: LGTM!The changes to include
status_codeanderrorin the logging payload are implemented correctly. This ensures that error details are consistently captured and logged to Athina.
100-106: LGTM!The exception handling and error logging in the
wrappermethod are implemented correctly. This ensures that any exceptions during the OpenAI API call are captured and logged to Athina before re-raising the exception.
138-145: LGTM!The exception handling and error logging in the
_response_interceptormethod for streaming responses are implemented correctly. This ensures that any exceptions during the processing of streaming responses are captured and logged to Athina before re-raising the exception.
149-162: LGTM!The exception handling and error logging in the
_response_interceptormethod for non-streaming responses are implemented correctly. This ensures that any exceptions during the logging of non-streaming responses are captured and logged to Athina before re-raising the exception.
163-190: LGTM!The
_log_exception_to_athinamethod is implemented correctly. It centralizes the error logging logic and ensures consistent error reporting to Athina. The method constructs an error dictionary, updates theathina_metaobject with the error details, and logs the error using thelog_to_athinafunction.
| custom_eval_metrics: Optional[Dict] = None | ||
| status_code: Optional[int] = None | ||
| error: Optional[Any] = None, |
There was a problem hiding this comment.
Consider using a more specific type hint for the error attribute.
Using Any as the type hint for the error attribute is very broad and could lead to inconsistencies in the stored error data. This could make it difficult to handle and process the stored error data consistently.
Consider using a more specific type hint, such as a custom Error class or a Union of specific error types, to ensure consistency in the stored error data.
| error_message = response_json.get('error', 'Unknown Error') | ||
| details_message = response_json.get( | ||
| 'details', {}).get('message', 'No Details') | ||
| raise CustomException( | ||
| response.status_code, f'{error_message}: {details_message}') | ||
| response.status_code, f'{response_json}') |
There was a problem hiding this comment.
Fix the error handling behavior and remove unused variables.
The changes in this code segment have the following issues:
- The expected behavior of raising a
CustomExceptionwith a detailed error message has been changed to include the entire response JSON. This could affect how the calling code handles failures. - The local variables
error_messageanddetails_messageare assigned but never used due to the changes in error handling.
Consider applying the following fixes:
- error_message = response_json.get('error', 'Unknown Error')
- details_message = response_json.get(
- 'details', {}).get('message', 'No Details')
raise CustomException(
response.status_code, f'{response_json}')If the new error handling behavior is intended, ensure that the calling code is updated to handle the change. Otherwise, consider reverting the change to maintain the expected behavior of raising a CustomException with a detailed error message:
error_message = response_json.get('error', 'Unknown Error')
details_message = response_json.get(
'details', {}).get('message', 'No Details')
- raise CustomException(
- response.status_code, f'{response_json}')
+ raise CustomException(
+ response.status_code, f'{error_message}: {details_message}')Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| error_message = response_json.get('error', 'Unknown Error') | |
| details_message = response_json.get( | |
| 'details', {}).get('message', 'No Details') | |
| raise CustomException( | |
| response.status_code, f'{error_message}: {details_message}') | |
| response.status_code, f'{response_json}') | |
| raise CustomException( | |
| response.status_code, f'{response_json}') |
Tools
Ruff
26-26: Local variable
error_messageis assigned to but never usedRemove assignment to unused variable
error_message(F841)
27-27: Local variable
details_messageis assigned to but never usedRemove assignment to unused variable
details_message(F841)
| print(f"Unexpected error: {e}") | ||
| return None |
There was a problem hiding this comment.
Tip
Codebase Verification
Potential issues identified with the error handling change
The change in error handling for make_post_request function could lead to unexpected behavior in some parts of the codebase. Here are the specific concerns:
-
In
athina_logger/log_stream_inference/openai_chat_completion_stream.pyandathina_logger/log_stream_inference/openai_completion_stream.py, exceptions are currently re-raised. ReturningNoneinstead of raising an exception could lead to aTypeErrorif the code assumes a response object. -
In
athina_logger/tracing/trace.py, themake_post_requestfunction is called without a try-except block. ReturningNonecould lead to unexpected behavior if the code assumes a response object.
To address these issues:
- Consider reverting the change in
make_post_requestto maintain the expected behavior of raising exceptions. - If keeping the new behavior, update the calling code in the mentioned files to handle the possibility of receiving
Noneas a return value.
Analysis chain
Verify the impact of the change in error handling behavior.
Similar to the previous comment, the change in error handling for unexpected exceptions alters the control flow and how errors are communicated to the caller. Returning None instead of raising the exception could lead to unexpected behavior in the calling code if it is not updated to handle the new behavior.
Consider reverting the change to maintain the expected behavior of raising the exception:
except Exception as e:
- print(f"Unexpected error: {e}")
- return None
+ raise eAlternatively, ensure that the calling code is updated to handle the new behavior of returning None in case of an unexpected exception.
To verify the impact of this change, run the following script:
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the usage of the `make_post_request` function.
# Test: Search for the function usage. Expect: Calling code handles the new behavior of returning `None`.
rg --type python -A 5 $'make_post_request'
Length of output: 72
Script:
#!/bin/bash
# Description: Find Python files and search for make_post_request function usage
# Find all Python files
python_files=$(fd -e py)
# Search for make_post_request in Python files
for file in $python_files; do
echo "Searching in $file:"
grep -n "make_post_request" "$file" -A 5
echo "---"
done
Length of output: 8357
Summary by CodeRabbit
New Features
Bug Fixes
Chores