Skip to content

logger: output footer once for multithreaded push/pull errors #2674

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.

Sign up for GitHub

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

Merged
merged 8 commits into from
Nov 3, 2019
Merged

logger: output footer once for multithreaded push/pull errors #2674

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
a768fd1
logger: output footer once for multithreaded push/pull errors
Oct 26, 2019
2e12af0
Fix improper variable name
Oct 26, 2019
98bafa9
Change f-string to old format
Oct 26, 2019
3bf810a
move footer from logger to main
Nov 1, 2019
8ae1165
fix typo upload -> download
Nov 1, 2019
89502be
fix tests
Nov 1, 2019
2893c9a
move footer to better place and change logging to stderr
Nov 3, 2019
d25282b
revert print to logger
Nov 3, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 12 additions & 15 deletions dvc/logger.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,17 @@
import colorama


FOOTER = (
"\n{yellow}Having any troubles?{nc}"
" Hit us up at {blue}https://dvc.org/support{nc},"
" we are always happy to help!"
).format(
blue=colorama.Fore.BLUE,
nc=colorama.Fore.RESET,
yellow=colorama.Fore.YELLOW,
)


class LoggingException(Exception):
def __init__(self, record):
msg = "failed to log {}".format(str(record))
Expand Down Expand Up @@ -48,16 +59,6 @@ class ColorFormatter(logging.Formatter):
"CRITICAL": colorama.Fore.RED,
}

footer = (
"{yellow}Having any troubles?{nc}"
" Hit us up at {blue}https://dvc.org/support{nc},"
" we are always happy to help!"
).format(
blue=colorama.Fore.BLUE,
nc=colorama.Fore.RESET,
yellow=colorama.Fore.YELLOW,
)

def format(self, record):
if record.levelname == "INFO":
return record.msg
Expand All @@ -66,18 +67,14 @@ def format(self, record):
exception, stack_trace = self._parse_exc(record.exc_info)

return (
"{color}{levelname}{nc}: {description}"
"{stack_trace}\n"
"\n"
"{footer}"
"{color}{levelname}{nc}: {description}" "{stack_trace}\n"
).format(
color=self.color_code.get(record.levelname, ""),
nc=colorama.Fore.RESET,
levelname=record.levelname,
description=self._description(record.msg, exception),
msg=record.msg,
Copy link
Author

@n3hrox n3hrox Nov 3, 2019

Choose a reason for hiding this comment

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

I see that deepsource detected that msg is not used here although I did not delete it
I think adding it back to message or removing it entirely should be scope of another PR to be honest, as it affects multiple test cases.
What do you think @efiop?

Copy link
Contributor

@efiop efiop Nov 3, 2019

Choose a reason for hiding this comment

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

You are right, it is out of scope for this PR.

stack_trace=stack_trace,
footer=self.footer,
)

return "{color}{levelname}{nc}: {msg}".format(
Expand Down
4 changes: 4 additions & 0 deletions dvc/main.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@

from dvc.cli import parse_args
from dvc.lock import LockError
from dvc.logger import FOOTER
from dvc.config import ConfigError
from dvc.analytics import Analytics
from dvc.exceptions import NotDvcRepoError, DvcParserError
Expand Down Expand Up @@ -75,6 +76,9 @@ def main(argv=None):
# so won't be reused by any other subsequent run anyway.
clean_repos()

if ret != 0:
Copy link
Author

@n3hrox n3hrox Nov 1, 2019

Choose a reason for hiding this comment

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

not sure if it should be here or rather in finally block

Copy link
Contributor

@Suor Suor Nov 1, 2019

Choose a reason for hiding this comment

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

This place is fine.

logger.info(FOOTER)

Analytics().send_cmd(cmd, args, ret)

return ret
2 changes: 1 addition & 1 deletion tests/func/test_remote.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -245,5 +245,5 @@ def unreliable_upload(self, from_file, to_info, name=None, **kwargs):


def get_last_exc(caplog):
_, exc, _ = caplog.records[-1].exc_info
_, exc, _ = caplog.records[-2].exc_info
return exc
38 changes: 8 additions & 30 deletions tests/unit/test_logger.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,11 +45,7 @@ def test_error(self, caplog):
with caplog.at_level(logging.INFO, logger="dvc"):
logger.error("message")

expected = (
"{red}ERROR{nc}: message\n"
"\n"
"{footer}".format(footer=formatter.footer, **colors)
)
expected = "{red}ERROR{nc}: message\n".format(**colors)

assert expected == formatter.format(caplog.records[0])

Expand All @@ -60,11 +56,7 @@ def test_exception(self, caplog):
except Exception:
logger.exception("message")

expected = (
"{red}ERROR{nc}: message\n"
"\n"
"{footer}".format(footer=formatter.footer, **colors)
)
expected = "{red}ERROR{nc}: message\n".format(**colors)

assert expected == formatter.format(caplog.records[0])

Expand All @@ -75,11 +67,7 @@ def test_exception_with_description_and_without_message(self, caplog):
except Exception:
logger.exception("")

expected = (
"{red}ERROR{nc}: description\n"
"\n"
"{footer}".format(footer=formatter.footer, **colors)
)
expected = "{red}ERROR{nc}: description\n".format(**colors)

assert expected == formatter.format(caplog.records[0])

Expand All @@ -90,10 +78,8 @@ def test_exception_with_description_and_message(self, caplog):
except Exception:
logger.exception("message")

expected = (
"{red}ERROR{nc}: message - description\n"
"\n"
"{footer}".format(footer=formatter.footer, **colors)
expected = "{red}ERROR{nc}: message - description\n".format(
**colors
)

assert expected == formatter.format(caplog.records[0])
Expand All @@ -110,13 +96,8 @@ def test_exception_under_verbose(self, caplog):
"{red}ERROR{nc}: description\n"
"{red}{line}{nc}\n"
"{stack_trace}"
"{red}{line}{nc}\n"
"\n"
"{footer}".format(
footer=formatter.footer,
line="-" * 60,
stack_trace=stack_trace,
**colors
"{red}{line}{nc}\n".format(
line="-" * 60, stack_trace=stack_trace, **colors
)
)

Expand All @@ -138,10 +119,7 @@ def test_nested_exceptions(self, caplog):
"{red}ERROR{nc}: message - second: first\n"
"{red}{line}{nc}\n"
"{stack_trace}"
"{red}{line}{nc}\n"
"\n"
"{footer}".format(
footer=formatter.footer,
"{red}{line}{nc}\n".format(
line="-" * 60,
stack_trace="\n".join([first_traceback, second_traceback]),
**colors
Expand Down