Skip to content
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

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

bolinocroustibat
Copy link
Contributor

@bolinocroustibat bolinocroustibat commented Oct 30, 2024

Attempt to handle numerous Sentry errors similar to #145755 in a more explicit and generic way:

  • Catch the exception when the CSV URL is not reachable and rename it more clearly, so that it can be more instantaneously understood as non major when browsing Sentry
  • New custom IOException using a new parent class ExceptionWithSentryDetails which sends details to Sentry as tags, also inherited by the recent custom ParseException
  • Move the errors.py file from /analysis to /utils, since it's more broadly used throughout the code
  • Use sentry_sdk.Hub.current.client instead of config.SENTRY_DSN to check if Sentry is correctly working, as it is the recommended way

Copy link
Contributor

@maudetes maudetes left a 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?

Comment on lines 65 to 66
except aiohttp.ClientResponseError as e:
raise IOError(f"Error downloading CSV: {e}")
Copy link
Contributor

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

Copy link
Contributor Author

@bolinocroustibat bolinocroustibat Nov 15, 2024

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?

Copy link
Contributor Author

@bolinocroustibat bolinocroustibat Nov 15, 2024

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!

@bolinocroustibat bolinocroustibat changed the title fix: handle Sentry issue #145755 fix: handle Sentry issue for CSV downloading errors Nov 15, 2024
@bolinocroustibat
Copy link
Contributor Author

bolinocroustibat commented Nov 15, 2024

I think if we raise an error, we won't return the tmp_file and maybe won't remove it in the finally close?

Good point, thanks. Logic has been modified since then, try/catch block now has a finally block to close and return the temp file.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants