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

Cleanup exception that are not logged as error #4590

Closed
humitos opened this issue Aug 30, 2018 · 7 comments · Fixed by #6112
Closed

Cleanup exception that are not logged as error #4590

humitos opened this issue Aug 30, 2018 · 7 comments · Fixed by #6112
Labels
Accepted Accepted issue on our roadmap Improvement Minor improvement to code
Milestone

Comments

@humitos
Copy link
Member

humitos commented Aug 30, 2018

After #4495 got merged @agjohnson suggested to have an attribute in the Exception class and check for that attribute before log the exception, instead of defining a list for the warning exceptions as I did at:

https://github.com/rtfd/readthedocs.org/pull/4495/files#diff-ca52b098301dd315a834b3556ab9a7d5R424

Also, there are more exceptions that have to treat in the same way: ProjectConfigurationError for example.

https://sentry.io/read-the-docs/readthedocs-org/issues/668248681/

@humitos humitos added this to the Cleanup milestone Aug 30, 2018
@humitos humitos self-assigned this Aug 30, 2018
@agjohnson
Copy link
Contributor

This is cleanup milestone, deprioritizing for now.

@humitos
Copy link
Member Author

humitos commented Oct 31, 2018

I think this is easier to do by defining Task.throws and using the proper Sentry setting SENTRY_CELERY_IGNORE_EXPECTED for this.

@stale
Copy link

stale bot commented Jan 10, 2019

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the Status: stale Issue will be considered inactive soon label Jan 10, 2019
@stsewd stsewd added Improvement Minor improvement to code Accepted Accepted issue on our roadmap labels Jan 10, 2019
@stale stale bot removed the Status: stale Issue will be considered inactive soon label Jan 10, 2019
@dojutsu-user
Copy link
Member

@humitos
I think this can be closed now.

@stsewd
Copy link
Member

stsewd commented Feb 22, 2019

I think we still need #5073 to close this

@stsewd
Copy link
Member

stsewd commented Sep 10, 2019

This is still valid. We are logging manually those exceptions to sentry

log_level_function(
LOG_TEMPLATE,
{
'project': self.project.slug,
'version': self.version.slug,
'msg': exc_value,
},
exc_info=True,
extra={
'stack': True,
'tags': {
'build': self.build.get('id'),
'project': self.project.slug,
'version': self.version.slug,
},
},
)

From projects/tasks.py we are re-raising some exceptions to don't fall into the genereal Exception block and don't log those (#6112).

And we haven't removed the old code :)

WARNING_EXCEPTIONS = (
VersionLockedError,
ProjectBuildsSkippedError,
YAMLParseError,
BuildTimeoutError,
MkDocsYAMLParseError,
)

There is no way to fix this using throws on the task. And we can't re-raise the exception because that will break many places.

And using the BuildEnviroment should know nothing about the exception on WARNING_EXCEPTIONS. So, we could pass on the BuildEnvironment constructor an arg like ignore_exceptions=() with all classes that we want to be ignored. Or Just remove the code that manually logs those errors and each part that uses a buildenviroment should be in charge of catching those, buildenviroment will only handle BuildEnvironmentWanings.

@stsewd stsewd reopened this Sep 10, 2019
@ericholscher
Copy link
Member

I feel like this is now solved. Re-open it if not 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Accepted Accepted issue on our roadmap Improvement Minor improvement to code
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants