| Message ID | 20180920205836.13163-1-stephen@that.guru |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [v2,1/2] parsearchive: Fix logging | expand |
On Thu, 2018-09-20 at 21:58 +0100, Stephen Finucane wrote: > We should use a counter normally, avoid using the counter and emit > logs > when more detailed output is requested, and emit nothing when no > output > is requested. > > In addition, the default logging level for the parser module is set > to > 'WARNING' to make it less chatty. > > Signed-off-by: Stephen Finucane <stephen@that.guru> > --- > v2: > - Set 'WARNING' level logging by default for all parsing activities > - Make more use of loggers rather than stdout I've rebased a couple of things onto these two patches and am starting to rely on it locally, so I'm just going to apply both. It's easy to revert either of the if necessary. Stephen
Hi Stephen, > I've rebased a couple of things onto these two patches and am starting > to rely on it locally, so I'm just going to apply both. It's easy to > revert either of the if necessary. They're breaking the build: https://travis-ci.org/getpatchwork/patchwork/jobs/431231311#L486 I don't think it's worth reverting them because of this, but please could you do a fixup? Regards, Daniel > > Stephen > > > _______________________________________________ > Patchwork mailing list > Patchwork@lists.ozlabs.org > https://lists.ozlabs.org/listinfo/patchwork
On Sat, 2018-09-22 at 00:22 +1000, Daniel Axtens wrote: > Hi Stephen, > > > I've rebased a couple of things onto these two patches and am starting > > to rely on it locally, so I'm just going to apply both. It's easy to > > revert either of the if necessary. > > They're breaking the build: > > https://travis-ci.org/getpatchwork/patchwork/jobs/431231311#L486 > > I don't think it's worth reverting them because of this, but please > could you do a fixup? Yup, will take a look at this this evening. I saw it last night but it was too late to investigate it. This didn't happen locally so I'm guessing the issue is something to do with how Travis runs things. Worst case scenario, I will revert and can retry later. Stephen > Regards, > Daniel > > > > Stephen
Stephen Finucane <stephen@that.guru> writes: > On Sat, 2018-09-22 at 00:22 +1000, Daniel Axtens wrote: >> Hi Stephen, >> >> > I've rebased a couple of things onto these two patches and am starting >> > to rely on it locally, so I'm just going to apply both. It's easy to >> > revert either of the if necessary. >> >> They're breaking the build: >> >> https://travis-ci.org/getpatchwork/patchwork/jobs/431231311#L486 >> >> I don't think it's worth reverting them because of this, but please >> could you do a fixup? > > Yup, will take a look at this this evening. I saw it last night but it > was too late to investigate it. This didn't happen locally so I'm > guessing the issue is something to do with how Travis runs things. > Worst case scenario, I will revert and can retry later. I can repro at least the pep8 stuff with docker-compose run web --tox -e pep8 That seems to highlight the issue that ends up erroring the test, so fixing the pep8 warnings should hopefully be sufficient. Regards, Daniel > > Stephen > >> Regards, >> Daniel >> > >> > Stephen
On Sat, 2018-09-22 at 01:18 +1000, Daniel Axtens wrote: > Stephen Finucane <stephen@that.guru> writes: > > > On Sat, 2018-09-22 at 00:22 +1000, Daniel Axtens wrote: > > > Hi Stephen, > > > > > > > I've rebased a couple of things onto these two patches and am starting > > > > to rely on it locally, so I'm just going to apply both. It's easy to > > > > revert either of the if necessary. > > > > > > They're breaking the build: > > > > > > https://travis-ci.org/getpatchwork/patchwork/jobs/431231311#L486 > > > > > > I don't think it's worth reverting them because of this, but please > > > could you do a fixup? > > > > Yup, will take a look at this this evening. I saw it last night but it > > was too late to investigate it. This didn't happen locally so I'm > > guessing the issue is something to do with how Travis runs things. > > Worst case scenario, I will revert and can retry later. > > I can repro at least the pep8 stuff with > docker-compose run web --tox -e pep8 > > That seems to highlight the issue that ends up erroring the test, so > fixing the pep8 warnings should hopefully be sufficient. Yeah, these were modifications I squeezed in at the last minute and didn't test thoroughly. Patch submitted. Stephen
diff --git a/patchwork/management/commands/parsearchive.py b/patchwork/management/commands/parsearchive.py index 96f64fa7..b4d8bcce 100644 --- a/patchwork/management/commands/parsearchive.py +++ b/patchwork/management/commands/parsearchive.py @@ -37,10 +37,24 @@ class Command(BaseCommand): dropped = 0 errors = 0 + verbosity = int(options['verbosity']) + if not verbosity: + level = logging.CRITICAL + elif verbosity == 1: + level = logging.ERROR + elif verbosity == 2: + level = logging.INFO + else: # verbosity == 3 + level = logging.DEBUG + + if level: + logger.setLevel(level) + logging.getLogger('patchwork.parser').setLevel(level) + # TODO(stephenfin): Support passing via stdin path = args and args[0] or options['infile'] if not os.path.exists(path): - self.stdout.write('Invalid path: %s' % path) + logger.error('Invalid path: %s', path) sys.exit(1) # assume if <infile> is a directory, then we're passing a maildir @@ -65,7 +79,7 @@ class Command(BaseCommand): for m in mbox: pass except AttributeError: - logger.warning('Broken mbox/Maildir, aborting') + logger.error('Broken mbox/Maildir, aborting') return logger.info('Parsing %d mails', count) @@ -81,10 +95,15 @@ class Command(BaseCommand): # somewhere for future reference? errors += 1 - if (i % 10) == 0: + if verbosity == 1 and (i % 10) == 0: self.stdout.write('%06d/%06d\r' % (i, count), ending='') self.stdout.flush() + mbox.close() + + if not verbosity: + return + self.stdout.write( 'Processed %(total)d messages -->\n' ' %(covers)4d cover letters\n' @@ -101,4 +120,3 @@ class Command(BaseCommand): 'errors': errors, 'new': count - dropped - errors, }) - mbox.close() diff --git a/patchwork/settings/base.py b/patchwork/settings/base.py index 16ca712a..3eb1f0e2 100644 --- a/patchwork/settings/base.py +++ b/patchwork/settings/base.py @@ -177,17 +177,17 @@ LOGGING = { 'loggers': { 'django': { 'handlers': ['console'], - 'level': 'INFO', + 'level': 'WARNING', 'propagate': True, }, 'patchwork.parser': { 'handlers': ['console'], - 'level': 'DEBUG', + 'level': 'WARNING', 'propagate': False, }, 'patchwork.management.commands': { 'handlers': ['console', 'mail_admins'], - 'level': 'INFO', + 'level': 'WARNING', 'propagate': True, }, },
We should use a counter normally, avoid using the counter and emit logs when more detailed output is requested, and emit nothing when no output is requested. In addition, the default logging level for the parser module is set to 'WARNING' to make it less chatty. Signed-off-by: Stephen Finucane <stephen@that.guru> --- v2: - Set 'WARNING' level logging by default for all parsing activities - Make more use of loggers rather than stdout --- patchwork/management/commands/parsearchive.py | 26 ++++++++++++++++--- patchwork/settings/base.py | 6 ++--- 2 files changed, 25 insertions(+), 7 deletions(-)