fix: Improve logging messages #298
Conversation
attiyaIshaque
force-pushed
the
attiya/SONIC-745-improve-logging
branch
3 times, most recently
from
0b926dc to
375eabe
Compare
Coverage report
Click to see where and how coverage changed
This report was generated by python-coverage-comment-action |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
attiyaIshaque
force-pushed
the
attiya/SONIC-745-improve-logging
branch
from
375eabe to
cd07f5d
Compare
attiyaIshaque
force-pushed
the
attiya/SONIC-745-improve-logging
branch
3 times, most recently
from
96bc7d1 to
2598127
Compare
There was a problem hiding this comment.
I see in the ticket ACs that we had to add appropriate errors such as CommerceToolsError to retries where needed. Have we verified that we don't need to change anything related to this?
Note for merging: please ensure on staging that we get appropriate error badges on DataDog.
There was a problem hiding this comment.
I don't think we need log messages at the start and end of every operation. In my opinion this will just pollute the logging information. Mostly we use this approach for debugging some issues but in normal cases I wouldn't suggest adding this many logs
There was a problem hiding this comment.
Please also make appropriate changes elsewhere in the PR where needed.
There was a problem hiding this comment.
Yes, I have added the CommerceToolsError in the appropriate places where retries are needed. As for the error badges, I've ensured that the log messages are structured correctly, first showing info and then debug. I will also verify on staging to ensure that the appropriate error badges are showing up on DataDog as expected before merging.
There was a problem hiding this comment.
This comment still needs resolution: Reference
There was a problem hiding this comment.
Also i am not sure if I see any new error being added to retry cases. You might need to look into that as well.
There was a problem hiding this comment.
I have checked, and we don't need any new errors.
| logger.error(f"Unable to update line item [ {line_item_id} ] state on fulfillment " | ||
| f"result with error {err.errors} and correlation id {err.correlation_id}") | ||
| logger.error(f"[update_line_item_state] Failed to update line item {line_item_id} for order {order_id}. " | ||
| f"Error: {err.errors}, Correlation ID: {err.correlation_id}") |
There was a problem hiding this comment.
Can we make this a continuous line instead.
| f"Error: {err.errors}, Correlation ID: {err.correlation_id}") | |
| f"with error: {err.errors}, Correlation ID: {err.correlation_id}") |
There was a problem hiding this comment.
Please use continuous lines for logs elsewhere in the PR too
There was a problem hiding this comment.
This still needs resolution elsewhere. Please use the same approach in the whole PR.
attiyaIshaque
force-pushed
the
attiya/SONIC-745-improve-logging
branch
from
2598127 to
a35b631
Compare
| ext_logger.debug('Request headers: %s', response.request.headers) | ||
| ext_logger.debug('Response status: %s %s', response.status_code, response.reason) | ||
| ext_logger.debug('Response text: %s', response.text) | ||
| # Extract request details and response information |
There was a problem hiding this comment.
Please remove these comments where added. We don't add this much comments in the codebase
| ext_logger.debug('Response status: %s %s', response.status_code, response.reason) | ||
| ext_logger.debug('Response text: %s', response.text) | ||
| # Extract request details and response information | ||
| request_url = response.request.url |
There was a problem hiding this comment.
Do we need to make separate variables for all these?
These can simply be used directly in the log message.
Same comment for the below changes as well
| f"course id: {logging_obj['course_id']}, message_id: {logging_obj['message_id']}, " | ||
| f"celery_task_id: {logging_obj['celery_task_id']}]" | ||
| f"[LMSAPIClient] Successful fulfillment for user '{logging_obj['user']}'. " | ||
| f"Details: [LMS User ID: {logging_obj['lms_user_id']}, Order ID: {logging_obj['order_id']}, " |
There was a problem hiding this comment.
Please use the field names itself in the log messages. This is an improvement to the already existing log messages since they are also not using the field names for logging which makes it difficult to follow the logs.
Example:
If we are logging lms_user_id in logs then it's log should also log the same field name lms_user_id and not lms user id or LMS User ID
| f"Details: [LMS User ID: {logging_obj['lms_user_id']}, Order ID: {logging_obj['order_id']}, " | |
| f"Details: [lms_user_id: {logging_obj['lms_user_id']}, Order ID: {logging_obj['order_id']}, " |
|
Please take a look into this recommendation from @shafqatfarhan as well: https://twou.slack.com/archives/C07UZC8DDSR/p1733148401861559 |
attiyaIshaque
force-pushed
the
attiya/SONIC-745-improve-logging
branch
from
a35b631 to
5a68365
Compare
I have addressed this in this PR. |
Merge checklist:
Check off if complete or not applicable:
Post-merge: