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

Clear args on LogRecords after rendering them in ProcessorFormatter #117

Merged
merged 3 commits into from May 15, 2017

Conversation

Projects
None yet
3 participants
@hynek
Owner

hynek commented May 13, 2017

This fixes #116 and probably http://stackoverflow.com/questions/43855507/configuring-and-using-structlog-with-django .

@if-fi did I miss something obvious that will break everything for everyone? also cc @gilbsgilbs who seems to know a lot more about stdlib logging than I. :)

@hynek hynek added this to the 17.2 milestone May 13, 2017

@codecov

This comment has been minimized.

codecov bot commented May 13, 2017

Codecov Report

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

Impacted file tree graph

@@          Coverage Diff          @@
##           master   #117   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          13     13           
  Lines         774    775    +1     
  Branches       96     96           
=====================================
+ Hits          774    775    +1
Impacted Files Coverage Δ
src/structlog/stdlib.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 5636314...161ede8. Read the comment docs.

@codecov

This comment has been minimized.

codecov bot commented May 13, 2017

Codecov Report

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

Impacted file tree graph

@@          Coverage Diff          @@
##           master   #117   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          13     13           
  Lines         778    779    +1     
  Branches       97     97           
=====================================
+ Hits          778    779    +1
Impacted Files Coverage Δ
src/structlog/stdlib.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 ff7cf4c...e618223. Read the comment docs.

@hynek hynek force-pushed the clear-args branch from 32dcc50 to 25fe993 May 13, 2017

@if-fi

This comment has been minimized.

if-fi commented May 14, 2017

Hi @hynek I just found the same problem on Thursday, and I was looking for a non-hackish solution to send as a pull request.
Yours is pretty smooth. I just tried it locally and it works fine in all scenarios I came up with.

P.S. I also found a problem when using ProcessorFormatter with RotatingFileHandler. I will send you a pull request that I hope will be included in the same bugfix release as this one

@gilbsgilbs

This comment has been minimized.

Contributor

gilbsgilbs commented May 14, 2017

Hi @if-fi, hi @hynek.

As far as I can see, this won't break anything and will fix the issue. Still, I have some remarks:

  • You're clearing record.args too often. Clearing after calling record.getMessage() (which is the one actually consuming the args) should be sufficient I guess (untested),
  • Not really fund of mutating record.msg and now record.args. You really shouldn't ever have to mutate any record's attribute, apart from record.message (≠ from record.msg, yeah, ik, logging module is real crap, but record.msg should be the unformatted message and record.message the formatted one -_-"),
  • You're actually formatting the message twice. First time in record.getMessage(), and another time calling super function, which formats the message and puts it into record.message . The second time should be near-free since record.args is empty (provided that format operator is a bit smart, haven't checked that out). Yet, it still requires two format when one is most likely possible (calling super function sooner, and getting the formatted message from record.message).

Don't have much time to dig into this. I think the fix is ok.

@hynek

This comment has been minimized.

Owner

hynek commented May 15, 2017

  1. Good catch, I’ll move it up.
  2. I sadly don’t know enough about this part. But I guess I’d be hoping for too much if I thought I could format into record.message and it solves #119 too, right? :)
  3. The reason is #105 (comment)

@hynek hynek merged commit 117ad7c into master May 15, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@hynek hynek deleted the clear-args branch May 15, 2017

@gilbsgilbs

This comment has been minimized.

Contributor

gilbsgilbs commented May 15, 2017

  1. Not hoping too much at all. That's the right way to fix #119.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment