Scc 4840/json logging WIP #46
Conversation
I think you should use the real thing. In my opinion it only needs to be mocked when testing other methods that happen to use logging, not when logging is the point of the test. |
|
The tests that are failing are from a completely different part of the code, and are also broken in main. I'm going to make a ticket for fixing them and not have them block this work. |
I wanted to see if I could fix the failing test but the failure isn't reproducing for me on the main branch- or your branch checked out locally, for that matter. I re-ran the test in GH and an additional test failed. Mysterious. I agree with your assessment that this failure isn't caused by your change- Tentatively approving this so we can unblock experimenting with structured logging in our python apps |
yossariano
left a comment
yossariano
left a comment
There was a problem hiding this comment.
As I mentioned in our 1:1- I'm interested in seeing how well this plays with cloudwatch logging. But I don't think if there's a problem there that the fix will be needed in this package.
| processors=[ | ||
| structlog.processors.add_log_level, | ||
| structlog.processors.TimeStamper(fmt="iso"), | ||
| structlog.processors.EventRenamer("message"), |
There was a problem hiding this comment.
I don't fully recall why I changed this from the default event to message in eReading- I think the change is fine but we should keep it in mind in case there's anything that expects event as a field in structured logs.
There was a problem hiding this comment.
i think that the winston json logging sets the first param as message in the json object, so this would ensure parity with that.
Add optional json logger using structlog.