fix: handle Sentry issue for CSV downloading errors #212
Conversation
bolinocroustibat
force-pushed
the
fix-sentry-issue-145755
branch
from
03efd31
to
bcd85d2
Compare
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.
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.
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.
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!
bolinocroustibat
changed the title
bolinocroustibat
force-pushed
the
fix-sentry-issue-145755
branch
from
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. |
@magopian Thanks! Should we do something similar for udata? What do you think? (I can do the PR if needed) |
|
I'm all up for it! |
A proposal: opendatateam/udata#3201 |
|
|
||
|
|
||
| async def handle_parse_exception(e: ParseException, table_name: str, check: Record | None) -> None: | ||
| """Specific ParseException handling. Store error if in a check context. Also cleanup :table_name: if needed.""" | ||
| db = await context.pool("csv") | ||
| await db.execute(f'DROP TABLE IF EXISTS "{table_name}"') | ||
| if check: | ||
| if config.SENTRY_DSN: | ||
| if sentry_sdk.Hub.current.client: |
There was a problem hiding this comment.
Why the change from config.SENTRY_DSN to sentry_sdk.Hub.current.client?
There was a problem hiding this comment.
It seems to be the recommend way to test if the Sentry client is properly started in several example like that one and it covers a larger number of cases of Sentry not being properly initialized compared to config.SENTRY_DSN.
But I agree it weakens the readability. Should we revert that?
Attempt to handle numerous Sentry errors similar to #145755 in a more explicit and generic way:
IOExceptionusing a new parent classExceptionWithSentryDetailswhich sends details to Sentry as tags, also inherited by the recent customParseExceptionerrors.pyfile from/analysisto/utils, since it's more broadly used throughout the codesentry_sdk.Hub.current.clientinstead ofconfig.SENTRY_DSNto check if Sentry is correctly working, as it is the recommended way