CV2-4790 fix broken error log due to missing file, surface s3 errors to sentry #429
Conversation
![codeclimate[bot]](https://avatars.githubusercontent.com/in/9403?s=60&v=4){kind=small}
| @@ -32,6 +34,7 @@ def download_file_from_s3(bucket: str, filename: str, local_path: str): | |||
| s3_client.download_file(bucket, tmk_file, local_path) | |||
| app.logger.info(f'Successfully downloaded file {tmk_file} from S3 bucket.') | |||
| except Exception as e: | |||
| ErrorLog.notify(e, {"bucket": bucket, "filename": filename, "local_path": local_path, "tmk_file": tmk_file}) | |||
|
Code Climate has analyzed commit 496c68f and detected 1 issue on this pull request. Here's the issue category breakdown:
The test coverage on the diff in this pull request is 33.3% (50% is the threshold). This pull request will bring the total coverage in the repository to 80.8% (0.0% change). View more on Code Climate. |
| @@ -3,6 +3,8 @@ | |||
|
|
|||
| from flask import current_app as app | |||
|
|
|||
| from app.main.lib.error_log import ErrorLog | |||
There was a problem hiding this comment.
I'm surprised the ErrorLog is needed to get errors to sentry, normally it already picks up on anything sent to python error output, so I'd think app.logger.error() would work?
There was a problem hiding this comment.
Maybe its a difference in configuration but I believe Sentry only surfaces raised actual errors and not log messages?
There was a problem hiding this comment.
Maybe. Originally I added a bunch of explicit sentry error dispatch, then removed most of it because I was seeing dupes. https://stackoverflow.com/questions/73456044/why-does-sentry-log-errors-caught-in-except-block
There was a problem hiding this comment.
Hrm - well, I'll do the same then if I see the problem manifesting here? lol
Description
Right now we hit an error where
filesisn't defined when we try to log the needle error to sentry, and we don't log failures to download the files from S3 directly to sentry. Let's resolve that to make easier the job of surfacing whatever the needle error is.Reference: CV2-4790
How has this been tested?
No tests on this yet - may need to change a unit test or two.
Have you considered secure coding practices when writing this code?
None