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

Don't log BuildEnvironmentWarning as error #6112

Merged
merged 2 commits into from Sep 5, 2019

Conversation

@stsewd
Copy link
Member

@stsewd stsewd commented Aug 28, 2019

Turns out we are logging this manually

except Exception:
# Catch unhandled errors when syncing
log.exception(
'An unhandled exception was raised during VCS syncing',
extra={
'stack': True,
'tags': {
'project': self.project.slug,
'version': self.version.slug,
},
},
)

Closes #4590

@stsewd stsewd requested a review from Aug 28, 2019
build_cmd = self.environment.run(*cmd, **kwargs)
try:
build_cmd = self.environment.run(*cmd, **kwargs)
except BuildEnvironmentWarning as e:
Copy link
Member

@humitos humitos Sep 4, 2019

Choose a reason for hiding this comment

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

It is supposed that we are not logging BuildEnvironmentWarning already (

BuildEnvironmentWarning,
). I'm not sure to follow why catching this exception and re-raising it as RepositoyError will avoid the log in Sentry.

I feel confused here 😕

Loading

Copy link
Member Author

@stsewd stsewd Sep 4, 2019

Choose a reason for hiding this comment

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

Yes, but that isn't the cause. We are logging it manually here

except Exception:
# Catch unhandled errors when syncing
log.exception(
'An unhandled exception was raised during VCS syncing',
extra={
'stack': True,
'tags': {
'project': self.project.slug,
'version': self.version.slug,
},
},
)

That try...except block only captures RepositoryError and LockTimeout, everything else is logged

Loading

Copy link
Member

@humitos humitos Sep 5, 2019

Choose a reason for hiding this comment

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

Gotcha! Do you know why the sync_repo is not ran inside the context manager that manages all the exceptions in the __exit__ as we do run all the rest of the commands?

Loading

Copy link
Member Author

@stsewd stsewd Sep 5, 2019

Choose a reason for hiding this comment

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

It runs inside one

with self.setup_env:
try:
before_vcs.send(sender=self.version)
if self.project.skip:
raise ProjectBuildsSkippedError
try:
with self.project.repo_nonblockinglock(version=self.version):
self.setup_vcs()

But again, that's not the problem. We still log the exception to sentry manually.

Loading

Copy link
Member

@humitos humitos Sep 5, 2019

Choose a reason for hiding this comment

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

I'm talking about this one which does not seem to be ran inside a context manager:

Loading

Copy link
Member Author

@stsewd stsewd Sep 5, 2019

Choose a reason for hiding this comment

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

Not really sure, we don't record that action as a build, so we don't need the all the stuff from the context manager. It doesn't seen related to this anyway. Both calls use the default env

self.environment = environment or LocalEnvironment(project)
and don't use the context manager.

Loading

Copy link
Member

@humitos humitos Sep 5, 2019

Choose a reason for hiding this comment

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

Let's merge and see what happen after the next deploy :)

Loading

humitos
humitos approved these changes Sep 5, 2019
@stsewd stsewd merged commit 85161d7 into readthedocs:master Sep 5, 2019
2 checks passed
Loading
@stsewd stsewd deleted the dont-log-buildenv-warning branch Sep 5, 2019
@stsewd
Copy link
Member Author

@stsewd stsewd commented Sep 6, 2019

@humitos errors from that place disappeared, but now it's logging from another place (where we log manually too)

https://sentry.io/organizations/read-the-docs/issues/1209161216/?project=148442&query=is%3Aunresolved+logger%3Areadthedocs.doc_builder.environments&statsPeriod=14d

def handle_exception(self, exc_type, exc_value, _):
"""
Exception handling for __enter__ and __exit__.
This reports on the exception we're handling and special cases
subclasses of BuildEnvironmentException. For
:py:class:`BuildEnvironmentWarning`, exit this context gracefully, but
don't mark the build as a failure. For all other exception classes,
including :py:class:`BuildEnvironmentError`, the build will be marked as
a failure and the context will be gracefully exited.
If the exception's type is :py:class:`BuildEnvironmentWarning` or it's
an exception marked as ``WARNING_EXCEPTIONS`` we log the problem as a
WARNING, otherwise we log it as an ERROR.
"""
if exc_type is not None:
log_level_function = None
if issubclass(exc_type, BuildEnvironmentWarning):
log_level_function = log.warning
elif exc_type in self.WARNING_EXCEPTIONS:
log_level_function = log.warning
self.failure = exc_value
else:
log_level_function = log.error
self.failure = exc_value
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,
},
},
)
return True

Looks it can be fixed by adding the exception in this list

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

Loading

@humitos
Copy link
Member

@humitos humitos commented Sep 6, 2019

Good!

It is supposed the WARNING_EXCEPTIONS is not needed anymore after we added the throws argument in the celery task. Actually, there should be an issue saying that we have to remove it.

The right place to add the exception class is

# Exceptions under ``throws`` argument are considered ERROR from a Build
# perspective (the build failed and can continue) but as a WARNING for the
# application itself (RTD code didn't failed). These exception are logged as
# ``INFO`` and they are not sent to Sentry.
@app.task(
bind=True,
max_retries=5,
default_retry_delay=7 * 60,
throws=(
VersionLockedError,
ProjectBuildsSkippedError,
YAMLParseError,
BuildTimeoutError,
BuildEnvironmentWarning,
RepositoryError,
ProjectConfigurationError,
ProjectBuildsSkippedError,
MkDocsYAMLParseError,
),
)

Loading

@stsewd
Copy link
Member Author

@stsewd stsewd commented Sep 6, 2019

@humitos the class is already on the throws arg. We should remove the code that manually logs to sentry.

Loading

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

Successfully merging this pull request may close these issues.

2 participants