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

Logging exceptions rework #5118

Merged
merged 7 commits into from
Feb 19, 2019
Merged

Logging exceptions rework #5118

merged 7 commits into from
Feb 19, 2019

Conversation

humitos
Copy link
Member

@humitos humitos commented Jan 16, 2019

This PR,

  • fixes the issue when calling setup_vcs and it raises an exception without stopping the build process
  • removes BuildEnvironmentWarning from Task.throws argument in tasks that is not needed
  • adds more "expected" exceptions for our update_docs tasks to not log them in Sentry as ERROR
  • fixes a docstring
  • applies pre-commit

Closes #5114
Closes #3856
Closes #4978
Related #5073
Related #5115

@ericholscher
Copy link
Member

applies pre-commit

Ugh. This makes it much harder to review. Let's not do that until the entire codebase is linted.

ericholscher
ericholscher previously approved these changes Jan 16, 2019
Copy link
Member

@ericholscher ericholscher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, with a bit of comment cleanup

readthedocs/projects/tasks.py Outdated Show resolved Hide resolved
)
return False


# Exceptions under ``throws`` argument are considered ERROR from a Build
# perspective but as a WARNING for the application itself. These exception are
# logged as ``INFO`` and they are not sent to Sentry.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand this comment. "considered ERROR from a Build
perspective but as a WARNING for the application itself" -- what does that mean in practice? Does the build stop?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A known error "by RTD's code" like "we didn't find the requirements.txt you configured in the YAML file" is an ERROR that will make the build to stop. It's an error from the user's perspective.

Although, it's just a WARNING from the developer's perspective or the application (RTD's code) itself.

The build stops and the exception is logged as INFO/WARNING not as an ERROR.

If you have a better way to explain this we can adapt the comment. I'd appreciate.

@agjohnson agjohnson added this to the Refactoring milestone Jan 22, 2019
@agjohnson agjohnson added the Improvement Minor improvement to code label Jan 22, 2019
These exceptions won't be logged into Sentry as ERROR but they will be
logged as INFO since they are errors from a user's perspective (build
failed) but not from an application perspective (our code didn't fail).
Depending on the exception, we want to log at different levels and in
all the cases stop the build (execute `raise` again).
@humitos humitos force-pushed the humitos/logging-exceptions-rework branch from 30885b3 to 1176c71 Compare February 18, 2019 16:03
@humitos humitos requested a review from a team February 18, 2019 17:59
Copy link
Member

@ericholscher ericholscher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense. 👍

@humitos humitos merged commit d901982 into master Feb 19, 2019
@delete-merged-branch delete-merged-branch bot deleted the humitos/logging-exceptions-rework branch February 19, 2019 21:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Improvement Minor improvement to code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Manage setup_vcs problems better BuildEnvironmentWarning logged as error Reduce logging to Sentry
3 participants