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

Backwards incompatible change in 17.1.0 event now required #110

Closed
keis opened this Issue May 4, 2017 · 4 comments

Comments

Projects
None yet
3 participants
@keis

keis commented May 4, 2017

This minimal example shows the problem. Not passing an event is perhaps a bit silly and bad pratice but a regress none the less.

import structlog
l = structlog.get_logger()
l.info(foo='bar')

With 16.1.0 this outputs

foo='bar'

on 17.1.0 I get this instead

Traceback (most recent call last):
  File "test.py", line 4, in <module>
    l.info(foo='bar')
  File "stuctlog/_base.py", line 176, in _proxy_to_logger
    args, kw = self._process_event(method_name, event, event_kw)
  File "stuctlog/_base.py", line 136, in _process_event
    event_dict = proc(self._logger, method_name, event_dict)
  File "stuctlog/dev.py", line 179, in __call__
    event = event_dict.pop("event")
KeyError: 'event'
@hynek

This comment has been minimized.

Owner

hynek commented May 4, 2017

This change is explicitly covered by our backward compatibility policy: all that changed is the default configuration. I don’t want to guarantee the same defaults forever because that makes it impossible to improve the first impression.

All you have to do is to configure structlog explicitly. The former default still works fine:

>>> structlog.processors.KeyValueRenderer()(None, None, {"foo": "bar"})
"foo='bar'"

Is there a way we could document the policy and its effects better? I’ve put it to the top of the changelog but I’m open to suggestions.

@keis

This comment has been minimized.

keis commented May 4, 2017

Ok, makes sense and also explains why this works in other parts where we explicitly configure the formatter.

My confusion here comes from when reading the changelog it registered as "the default rendering format has changed", expecting that maybe the exact string produced would be different. While this to me looks a change in the api of logger.info to require the event message. That this was because of a new renderer was not immediately obvious to me.

It should perhaps also be noted what requirements the default components put on the log record e.g json needs the values to be json encodable and console requires the event key to be present.

@iwanb

This comment has been minimized.

iwanb commented Jun 22, 2017

The same issue appears if the event is the empty string:

logger.info('')

Is that also expected?

hynek added a commit that referenced this issue Jun 22, 2017

@hynek

This comment has been minimized.

Owner

hynek commented Jun 22, 2017

Nope, that’s a bug. And a pretty embarrassing one given how much I preach to never do naked if checks like if event:. 🙈

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