add Identify errors in plugin#20
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR adds logic to uniquely identify errors in the plugin and prevent duplicate logging and notifications through a new error_logger module.
- Updates the plugin to call log_error with error deduplication and silencing.
- Introduces error_logger.py to handle error logging and notification management.
- Updates the README to document the new error identification and notification features.
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| ckanext/push_errors/plugin.py | Updates error_handler to use log_error with try/except wrapping |
| ckanext/push_errors/error_logger.py | Adds error_logger module for deduplicated error logging |
| README.md | Adds documentation for the new error identification and management |
Comments suppressed due to low confidence (2)
ckanext/push_errors/error_logger.py:11
- Consider verifying that 'error.traceback' is not None before using it to generate error_id, to avoid always defaulting to 'unknown' when no traceback is available.
def log_error(error, disable_notification=False):
ckanext/push_errors/plugin.py:45
- [nitpick] The variable name 'log_err' is somewhat terse; consider renaming it to something clearer like 'logging_exception' for improved readability.
except Exception as log_err:
2a95dbc to
660cdbd
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR adds error identification enhancements to the plugin by deduplicating error logs and enabling notification suppression, as well as corresponding tests and documentation updates.
- New tests have been added to verify error de-duplication and notification behavior.
- The plugin now uses log_error with error identification and fallback logging.
- The error_logger module implements error deduplication using file path and line number.
- README has been updated to document the new error handling approach.
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| ckanext/push_errors/tests/test_error_login.py | Adds tests to validate deduplication and optional notification suppression behavior. |
| ckanext/push_errors/plugin.py | Integrates log_error in the error_handler with fallback logging for log_error failures. |
| ckanext/push_errors/error_logger.py | Implements error deduplication by tracking errors based on file path and line number. |
| README.md | Updates documentation for the new error identification and notification management features. |
avdata99
left a comment
There was a problem hiding this comment.
See comments but not for change, just to analyze. A new PR is required in the latest comment.
| # Some code that raises an exception | ||
| raise ValueError("An example error") | ||
| except Exception as e: | ||
| log_error(e, disable_notification=True) |
There was a problem hiding this comment.
Disable notification is not expected to do like this.
Disable will be dynamic, this PR is to start uniquely identifying error.
File path + line looks good but disable_notification is not required, see other comments
|
|
||
| # Registrar usando log_error (deduplicación y silenciado) | ||
| try: | ||
| log_error(exception) |
There was a problem hiding this comment.
This will send the push message and after that, we'll sent it again later in this function
| tb = traceback.extract_tb(error.__traceback__) | ||
| if tb: | ||
| file_path, line_number, _, _ = tb[-1] |
There was a problem hiding this comment.
It look interesting this way to get the file_path and the line_number ensure it works, document what else we get (the 3º and 4º params) and use it in the new requested PR
| file_path, line_number, _, _ = tb[-1] | ||
| error_id = f"{file_path}:{line_number}" | ||
| else: | ||
| error_id = "unknown" |
There was a problem hiding this comment.
This will skip all non identified erros. Always a unique ID is required
|
|
||
| log = logging.getLogger(__name__) | ||
|
|
||
| logged_errors = set() # To track already logged errors |
There was a problem hiding this comment.
This will be always empty.
For the future PR, we need to use redis or the main CKAN DB to preserve a list of logged errors
|
We have two type of push-error notifications:
So the first goal of this PR is to get a unique ID for all messages before sent them. It could be the filepath+line number for error objects and the whole text for log.critical messages. You can restart a new PR, this code do not capture the general idea. Let split this into multiple PRs:
Use both function and add the unique ID to the push message |
fixes #18