Skip to content
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

A newer, better thread-local API #225

Merged
merged 14 commits into from
Sep 23, 2019
Merged

A newer, better thread-local API #225

merged 14 commits into from
Sep 23, 2019

Conversation

radix
Copy link
Contributor

@radix radix commented Sep 21, 2019

Fixes #222

caveat: I didn't duplicate the greenlet support because it turns out greenlet.getcurrent() doesn't seem to return separate objects for different OS-level Python threads. That means that if you somehow have greenlet in your dependency tree, the existing thread-local stuff will just straight up not work.

If someone cares I'm sure it'd be possible to implement something that uses greenlets and threadlocal in a way that ensures that you get distinct values in different greenlets AND distinct values in different python threads.

… NOT return distinct values if you call it from separate python threads.

Also reformat.
@radix
Copy link
Contributor Author

radix commented Sep 21, 2019

I'm not sure why coverage isn't working. My tests should be covering everything 100% and I don't understand how to read the output of codecov.io

@hynek
Copy link
Owner

hynek commented Sep 22, 2019

This should be helpful: https://codecov.io/gh/hynek/structlog/src/222-thread-local/src/structlog/threadlocal.py#L178 Maybe you call clear before or something?


That said, could you make them both a try/except block? I'm not a fan of hasattr in my codebases.

Other that that, it looks super promising. When people asked me about support contextvars, something exactly like this came to my mind. Since the code is rather trivial, I suppose it doesn't make any sense to try to make something generic?

It's definitely the better approach than having everything global by default and offer temporary binds.

@hynek
Copy link
Owner

hynek commented Sep 22, 2019

OK cool. Do you need anything from me to finish this up? If you want, you can skip narrative docs for now. I can write them in a separate PR and you'll review it (timely! 🤪).

But please refer to CONTRIBUTING.rst, there's a bunch of transgressions. :)

@radix
Copy link
Contributor Author

radix commented Sep 22, 2019

@hynek I already started on the docs when I saw your message, so I figured I'd finish up what I have and put it up here.

Please let me know which contribution guidelines I have missed.

@radix radix marked this pull request as ready for review September 22, 2019 17:37
Copy link
Owner

@hynek hynek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bit of bikeshedding but also please add the .. autofunctions to api.rst. Please put it at the top of the threadlocal module docs so people see it first.

tests/test_threadlocal.py Show resolved Hide resolved

def merge_threadlocal_context(logger, method_name, event_dict):
"""
A structlog processor that merges in a global (thread-local) context.
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can drop the structlog here.

src/structlog/threadlocal.py Show resolved Hide resolved
CHANGELOG.rst Outdated
@@ -51,7 +51,9 @@ Changes:
So far, the configuration proxy, ``structlog.processor.TimeStamper``, ``structlog.BoundLogger``, ``structlog.PrintLogger`` and ``structlog.dev.ConsoleLogger`` have been made pickelable.
Please report if you need any another class ported.
`#126 <https://github.com/hynek/structlog/issues/126>`_

- Added a new thread-local API that allows binding values to a thread-local context explicitly without affecting the default behavior of bind.
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

``bind()``

please

@radix radix requested a review from hynek September 23, 2019 15:41
@radix
Copy link
Contributor Author

radix commented Sep 23, 2019

Thanks for the review @hynek, I've addressed your requests.

@hynek hynek merged commit 895257b into hynek:master Sep 23, 2019
@hynek
Copy link
Owner

hynek commented Sep 23, 2019

Thanks Chris, please come back soon! ;)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Is there a way to do threadlocal context for only certain binds?
2 participants