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

Suppressing the KeyError for an unbind() call #171

Closed
anirvan-majumdar opened this Issue Jun 28, 2018 · 6 comments

Comments

Projects
None yet
2 participants
@anirvan-majumdar

anirvan-majumdar commented Jun 28, 2018

Hi,

This is a suggestion for possible improvement in the logic of invoking the unbind() method on the BoundLogger. We recently deployed structlog in our Django project. Everything seemed to work fine until we started getting a bunch of KeyErrors while unbinding parameters.

We realised that we had a recursive logic where on a child stack the context variable would get unbound and when the control came back to the parent stack, another call to unbind crashed the flow.

For now, we added a new wrapper class over the BoundLogger which simply suppresses the exception:

from structlog.stdlib import BoundLogger


class BoundLoggerWrapper(BoundLogger):
    def unbind(self, *keys):
        try:
            return super(BoundLoggerWrapper, self).unbind(*keys)
        except KeyError:
            return self.bind()

Just wanted to check whether doing this might have any unwanted consequences.

Thanks for this wonderful library!

@hynek

This comment has been minimized.

Owner

hynek commented Jun 28, 2018

I don’t think silencing errors is a good plan, but I may be open to add something like try_unbind?


Unrelatedly you mentioned you deployed structlog in a Django app…would you mind to contribute some wisdom how you did it? I haven’t used Django in years but people keep mentioning, that they’d like examples of that.

@anirvan-majumdar

This comment has been minimized.

anirvan-majumdar commented Jun 28, 2018

try_unbind would be perfect and clear in intent. The KeyError issue can be quite pervasive in a big application where binding/unbinding on the same key might happen within the context of a request/response cycle. Esp more so when a web application might also rely on an async library like celery 😕

would you mind to contribute some wisdom how you did it?

Sure thing. Would you like me to jot down some notes in the comments here or in a separate doc?

@hynek

This comment has been minimized.

Owner

hynek commented Jun 30, 2018

Sure thing. Would you like me to jot down some notes in the comments here or in a separate doc?

I think it would be the easiest for everybody if you just opened a bug “Add integration information for Django" and dump whatever you‘re comfortable to share there. Then we can start thinking how to structure it and where to put it!

@anirvan-majumdar

This comment has been minimized.

anirvan-majumdar commented Jul 4, 2018

Cool. Will do so by end of this week.

hynek added a commit that referenced this issue Jul 5, 2018

@hynek

This comment has been minimized.

Owner

hynek commented Jul 5, 2018

try_unbind() has been added in fd01af7 and will be released with 18.2.

@anirvan-majumdar

This comment has been minimized.

anirvan-majumdar commented Jul 12, 2018

Added a document here: #175

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