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

Allow using upper-case levels in ConsoleRenderer #139

Merged
merged 8 commits into from Jan 20, 2018

Conversation

Projects
None yet
2 participants
@quodlibetor
Contributor

quodlibetor commented Aug 14, 2017

Loggers that come from the standard library will have SCREAMING levels. Using the dev renderer for those causes errors like the following:

Traceback (most recent call last):
  File "...lib/python2.7/logging/__init__.py", line 861, in emit
    msg = self.format(record)
  File "...lib/python2.7/logging/__init__.py", line 734, in format
    return fmt.format(record)
  File ".../structlog/stdlib.py", line 459, in format
    record.msg = self.processor(logger, meth_name, ed)
  File ".../structlog/dev.py", line 176, in __call__
    self._styles.reset + "] "
KeyError: 'INFO'

I considered adding a more general "set all styles" API, but it seems like this patch should cover 99% of cases.

@quodlibetor quodlibetor force-pushed the quodlibetor:uppercase-levels-in-dev-renderer branch from ae8475b to 7a53dd1 Aug 14, 2017

@codecov

This comment has been minimized.

codecov bot commented Aug 14, 2017

Codecov Report

Merging #139 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff          @@
##           master   #139   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          13     13           
  Lines         794    802    +8     
  Branches      102    104    +2     
=====================================
+ Hits          794    802    +8
Impacted Files Coverage Δ
src/structlog/dev.py 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d22eaec...afbd69e. Read the comment docs.

@quodlibetor

This comment has been minimized.

Contributor

quodlibetor commented Aug 14, 2017

That codecov report doesn't make any sense to me. Although it does point out that I didn't actually add any tests for this, I'm happy to if they're desired.

@quodlibetor quodlibetor force-pushed the quodlibetor:uppercase-levels-in-dev-renderer branch from 7a53dd1 to 209b925 Aug 15, 2017

@quodlibetor

This comment has been minimized.

Contributor

quodlibetor commented Aug 15, 2017

I added a test.

@quodlibetor quodlibetor force-pushed the quodlibetor:uppercase-levels-in-dev-renderer branch from 209b925 to 8fec602 Aug 15, 2017

Repository owner deleted a comment from codecov bot Aug 17, 2017

@hynek

This comment has been minimized.

Owner

hynek commented Aug 17, 2017

Thanks! This needs an CHANGELOG entry.

@quodlibetor quodlibetor force-pushed the quodlibetor:uppercase-levels-in-dev-renderer branch from 8fec602 to b52e8dd Aug 17, 2017

@quodlibetor

This comment has been minimized.

Contributor

quodlibetor commented Aug 17, 2017

Cool! How's that?

@quodlibetor

This comment has been minimized.

Contributor

quodlibetor commented Aug 17, 2017

I'm not sure why the travis build is failing.

@quodlibetor quodlibetor force-pushed the quodlibetor:uppercase-levels-in-dev-renderer branch 2 times, most recently from 21b1712 to fdb5bfb Aug 17, 2017

@@ -26,7 +26,7 @@ Changes:
- Empty strings are valid events now.
`#110 <https://github.com/hynek/structlog/issues/110>`_
- ``structlog.dev.ConsoleRenderer`` now handles log events with all-caps log-levels, in addition to the default lowercase levels generated by ``structlog.stdlib.add_log_level``

This comment has been minimized.

@hynek

hynek Aug 17, 2017

Owner

Please

  • add () behind add_log_level (it’s nicer to read if you can identify functions/callables on one blink)
  • end the sentence with a period
  • add a link to this pr in a separate line

This comment has been minimized.

@quodlibetor

quodlibetor Aug 17, 2017

Contributor

done.

@hynek

This comment has been minimized.

Owner

hynek commented Aug 17, 2017

Eh that seems to be my fault, don’t worry about it. Could you give me a quick reproducer on how to get uppercase log levels tho? I can’t remember to ever having seen them.

@quodlibetor

This comment has been minimized.

Contributor

quodlibetor commented Aug 17, 2017

Trying to reproduce, I think I was wrong about this occurring if you just use the standard add_log_level. I created the following function based on the existing one because (a) I was confused about how the two pieces fit together and (b) my monitoring infrastructure is case-sensitive, searching for ERROR and WARNING.

def add_log_level(_logger, method_name, event_dict):
    # type: (str, str, Dict[str, Any]) -> Dict[str, Any]
    """
    Add the log level to the event dict.
    """
    if method_name == 'warn':
        # The stdlib has an alias
        method_name = 'warning'

    event_dict['level'] = method_name.upper()  # <-- the magic bug
    return event_dict

So you need to actually define your own add_log_level function to trigger this problem, making it... maybe not a problem? I would be happy to either change this PR to take an arbitrary formatting dict or, slightly less happily, drop this PR and just extend ProcessorFormatter in my own code.

The reason extending it locally would make me less happy is that the format dict is an implementation detail, so I expect anything I did to modify it in my own code would would be likely to break down the line. Ultimately I'd like to be able to safely specify some colors for the dev renderer.

@quodlibetor quodlibetor force-pushed the quodlibetor:uppercase-levels-in-dev-renderer branch from fdb5bfb to 7d26b80 Aug 17, 2017

@hynek

This comment has been minimized.

Owner

hynek commented Aug 18, 2017

Yeah that seems very much your problem. :)

Why don't we just add a .lower() tolevel and a comment to

"[" + self._level_to_color[level] +
and call it a day?

@quodlibetor

This comment has been minimized.

Contributor

quodlibetor commented Aug 18, 2017

Well the issue with putting a .lower() in there is that it's about 3x slower:

>>> from structlog import dev
>>> import timeit
>>> styles = dev._ColorfulStyles

>>> levels = {
...             "critical": styles.level_critical,
...             "exception": styles.level_exception,
...             "error": styles.level_error,
...             "warn": styles.level_warn,
...             "warning": styles.level_warn,
...             "info": styles.level_info,
...             "debug": styles.level_debug,
...             "notset": styles.level_notset,
...         }
>>> levels_with_upper = levels.copy()
>>> for level in list(levels_with_upper.keys()):
...     levels_with_upper[level.upper()] = levels_with_upper[level]

>>> timeit.repeat("levels['info']", globals=globals())
[0.04941119905561209, 0.048801488941535354, 0.04878102499060333]

>>> timeit.repeat("levels_with_upper['info']", globals=globals())
[0.04272375698201358, 0.04602321516722441, 0.04284745710901916]

# and now the same thing with a `.lower()` call
>>> timeit.repeat("levels_with_upper['info'.lower()]", globals=globals())
[0.17659165197983384, 0.17641176492907107, 0.17436388204805553]

I guess that's not a huge deal for a renderer for in development, but if people are using the ConsoleRenderer without colors for logging to a file (as is recommend by the docs) in production then that seems like an unfortunate regression?

I'd rather add a default_level_styles method that allows something like:

my_styles = ConsoleRenderer.default_level_styles(color=True)
fixup_styles(my_styles)
renderer = ConsoleRenderer(..., level_styles=my_styles)

This is flexible enough to allow users to add custom colors (e.g. a trace if they wanted to go through the other implementation work) or to override individual colors if they wanted to for some reason. It also allows me to do what I want.

ConsoleRenderer: A more configurable level_styles approach
This works for me in particular because I would like to be able to use
capitalized levels, but in general this allows people to override color
settings and to add new levels without requiring patching the ConsoleRenderer.
@quodlibetor

This comment has been minimized.

Contributor

quodlibetor commented Aug 18, 2017

I've added an implementation of my idea so you can see what it looks like. It's more code and adds to the API surface, but hopefully most people wouldn't need to think about it, the default is correct for most cases.

hynek added some commits Jan 20, 2018

@hynek hynek merged commit 5a3d3e5 into hynek:master Jan 20, 2018

3 checks passed

codecov/patch 100% of diff hit (target 100%)
Details
codecov/project 100% (+0%) compared to d22eaec
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@hynek

This comment has been minimized.

Owner

hynek commented Jan 20, 2018

Sorry it took so long. I’ll rename the method to get_… but otherwise it looks fine.

@quodlibetor

This comment has been minimized.

Contributor

quodlibetor commented Jan 21, 2018

Oh, neat! Thanks!

@quodlibetor quodlibetor deleted the quodlibetor:uppercase-levels-in-dev-renderer branch Jan 21, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment