CCMRG-1903: Centralize lock logging in BaseCodecovTask #588
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 26, 2025
drazisil-codecov
commented
- Add with_logged_lock() context manager to BaseCodecovTask for consistent lock logging
- Refactor all tasks to use centralized lock logging method
- Remove duplicate lock logging code from:
- upload.py
- preprocess_upload.py
- sync_repos.py
- sync_pull.py
- manual_trigger.py
- crontasks.py
- upload_finisher.py
- All locks now log: Acquiring lock, Acquired lock, Releasing lock (with duration)
- No changes to actual lock logic, only added observability
- Add with_logged_lock() context manager to BaseCodecovTask for consistent lock logging - Refactor all tasks to use centralized lock logging method - Remove duplicate lock logging code from: - upload.py - preprocess_upload.py - sync_repos.py - sync_pull.py - manual_trigger.py - crontasks.py - upload_finisher.py - All locks now log: Acquiring lock, Acquired lock, Releasing lock (with duration) - No changes to actual lock logic, only added observability
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #588 +/- ##
=======================================
Coverage 93.86% 93.86%
=======================================
Files 1284 1284
Lines 46478 46492 +14
Branches 1522 1522
=======================================
+ Hits 43628 43642 +14
Misses 2540 2540
Partials 310 310 ☔ 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! |
- Add comprehensive test suite for with_logged_lock() context manager - Test that 'Acquiring lock' is logged before lock acquisition - Test that 'Acquired lock' is logged after successful acquisition - Test that 'Releasing lock' is logged with duration after release - Test that extra context is included in all log messages - Test that code executes correctly within lock context - Test that LockError propagates correctly (not caught) - Test that 'Releasing lock' is logged even on exceptions
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Use approximate comparison instead of exact equality for lock_duration_seconds to handle floating point precision issues
Updated assertions in test_base.py to use multi-line format for better clarity in floating point precision checks for lock_duration_seconds.
Contributor
Author
|
@sentry review |
![sentry[bot]](https://avatars.githubusercontent.com/u/1396951?s=60&v=4){kind=small}
Modify get_report_lock to return both lock and lock_name as a tuple, eliminating duplication and ensuring the lock_name passed to with_logged_lock always matches the actual lock being used. This prevents potential bugs if get_report_lock's internal lock naming logic changes in the future.
adrianviquez
approved these changes
Nov 26, 2025
calvin-codecov
approved these changes
Nov 26, 2025
Removed comments verifying log messages in unit tests.
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.