CCMRG-1862 Enhance logging in ReportService and UploadTask for better traceability #556
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
drazisil-codecov
commented
Nov 10, 2025
drazisil-codecov
commented
- Added detailed logging in the initialize_and_save_report method of ReportService to track the report creation and backfilling process.
- Improved error handling in UploadTask to log unexpected errors during report initialization, ensuring better visibility into issues.
- Included success messages for report initialization and saving, enhancing overall logging clarity.
… traceability - Added detailed logging in the initialize_and_save_report method of ReportService to track the report creation and backfilling process. - Improved error handling in UploadTask to log unexpected errors during report initialization, ensuring better visibility into issues. - Included success messages for report initialization and saving, enhancing overall logging clarity.
Contributor
Author
|
@sentry review |
jason-ford-codecov
approved these changes
Nov 10, 2025
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #556 +/- ##
=======================================
Coverage 93.84% 93.84%
=======================================
Files 1284 1284
Lines 46318 46325 +7
Branches 1524 1524
=======================================
+ Hits 43469 43476 +7
Misses 2540 2540
Partials 309 309
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
- Removed redundant info logs in the initialize_and_save_report method of ReportService to streamline the logging process. - Changed some info logs to debug level for less verbosity during normal operations. - Updated UploadTask to remove success log after report initialization, focusing on error handling instead.
- Added a new test to verify logging of unexpected errors during report initialization in UploadTask. - Refactored logging in ReportService to improve clarity and structure, ensuring consistent formatting in log messages.
Contributor
Author
|
@sentry review |
- Cleaned up unnecessary whitespace in the test_upload_task.py file to enhance code clarity and maintain consistent formatting. - Ensured that the test structure remains intact while improving overall readability.
- Changed the logging of the files count in the ReportService to use the total files count from the report instead of the length of the files list, improving accuracy in the report details.
- Added mocks for functions that run before `initialize_and_save_report` in the `TestUploadTaskUnit` class to isolate tests and improve reliability. - This change ensures that tests can focus on error handling without external dependencies affecting outcomes.
Contributor
Author
|
@sentry review |
- Updated the ReportService to include a fallback mechanism for the files count in the logging process, ensuring accurate reporting even if totals computation fails. - Improved log details by including the repository ID alongside the commit ID for better traceability.
- Implemented a call to log breadcrumbs when unexpected errors occur during the upload process, enhancing error tracking and visibility. - Updated unit tests to verify that the breadcrumb logging is correctly triggered with the appropriate parameters, ensuring robust error handling in UploadTask.
Contributor
Author
|
@sentry review |
- Modified the ReportService to log the total files count directly from the report object, removing the fallback mechanism for improved accuracy in logging. - This change enhances the clarity of the log messages by ensuring that the correct files count is always reported.
- Updated the unit test for UploadTask to use assert_called_once_with for verifying error logging, ensuring both call count and arguments are checked. - This change enhances the accuracy of error context in logs, improving overall test reliability and clarity.
Contributor
Author
|
@sentry review |
Comment on lines
555
to
+575
| error=Errors.INTERNAL_RETRYING, | ||
| ) | ||
| self.retry(countdown=60, kwargs=upload_context.kwargs_for_retry(kwargs)) | ||
| except Exception as e: | ||
| log.error( | ||
| "Unexpected error during initialize_and_save_report", | ||
| extra={ | ||
| **upload_context.log_extra(), | ||
| "error_type": type(e).__name__, | ||
| "error_message": str(e), | ||
| }, | ||
| exc_info=True, | ||
| ) | ||
| self._call_upload_breadcrumb_task( | ||
| commit_sha=commit.commitid, | ||
| repo_id=repository.repoid, | ||
| milestone=Milestones.COMPILING_UPLOADS, | ||
| error=Errors.UNKNOWN, | ||
| error_text=repr(e), | ||
| ) | ||
| raise |
There was a problem hiding this comment.
The error handling for unexpected exceptions during initialize_and_save_report calls _call_upload_breadcrumb_task with error=Errors.UNKNOWN, which is appropriate for catching unexpected errors. However, make sure that other report service implementations (BundleAnalysisReportService, TestResultsReportService) don't override this method in a way that would bypass this error handling. If they do, the exception handling should be applied consistently across all report service implementations.
Severity: MEDIUM
🤖 Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location: apps/worker/tasks/upload.py#L555-L575
Potential issue: The error handling for unexpected exceptions during
`initialize_and_save_report` calls `_call_upload_breadcrumb_task` with
`error=Errors.UNKNOWN`, which is appropriate for catching unexpected errors. However,
make sure that other report service implementations (BundleAnalysisReportService,
TestResultsReportService) don't override this method in a way that would bypass this
error handling. If they do, the exception handling should be applied consistently across
all report service implementations.
Did we get this right? 👍 / 👎 to inform future reviews.
…ed clarity - Updated the unit test for UploadTask to replace assert_called_once_with with explicit assertions for call count and arguments, improving test readability and reliability. - This change ensures that the error logging behavior is thoroughly verified, including the message and additional context details.
drazisil-codecov
deleted the
CCMRG-1862_add-extra-logging-to-upload-task
branch
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.