-
Notifications
You must be signed in to change notification settings - Fork 0
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: handle Sentry issue for CSV downloading errors #212
base: main
Are you sure you want to change the base?
Conversation
03efd31
to
bcd85d2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think if we raise an error, we won't return the tmp_file
and maybe won't remove it in the finally
close?
udata_hydra/utils/file.py
Outdated
except aiohttp.ClientResponseError as e: | ||
raise IOError(f"Error downloading CSV: {e}") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this makes the error more explicit than previously?
I think the issue is more about marking these as "expected" in sentry
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the error is more explicit that way, at least it does describe it in the title. For now in Sentry it's not easy to identify what is the issue is when listing the errors, and that way we can see immediately in list mode that it's an "expected" error.
We could also create a specific exception for this, which would include attributes to identify the ressource, as we did for the custom ParseException
. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could also create a specific exception for this, which would include attributes to identify the ressource, as we did for the custom ParseException. What do you think?
Update: it has been done in this PR, as a proposal. Please review again!
e55a3ab
to
32d7f95
Compare
Good point, thanks. Logic has been modified since then, try/catch block now has a finally block to close and return the temp file. |
Attempt to handle numerous Sentry errors similar to #145755 in a more explicit and generic way:
IOException
using a new parent classExceptionWithSentryDetails
which sends details to Sentry as tags, also inherited by the recent customParseException
errors.py
file from/analysis
to/utils
, since it's more broadly used throughout the codesentry_sdk.Hub.current.client
instead ofconfig.SENTRY_DSN
to check if Sentry is correctly working, as it is the recommended way