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

add unbind_threadlocal and try_unbind_threadlocal methods #239

Merged
merged 4 commits into from Jan 25, 2020

Conversation

c-wilson
Copy link
Contributor

@c-wilson c-wilson commented Jan 13, 2020

I really like the new processor-style interface for thread local context, but I was hoping we could extend it with unbind methods that mirror the BoundLoggerBase class. I frequently have the need to enter and exit a logging context, but I'm a bit worried about unintended side-effects using the old interface and threadlocal.tmp_bind.

adds:

  • structlog.threadlocal.unbind_threadlocal
  • tests
import structlog

log= structlog.get_logger()

def myfunction():
  structlog.threadlocal.bind_threadlocal(x=12, y=2)
  log.msg('my message')
  # enter another function, module, etc.
  structlog.threadlocal.unbind_threadlocal('x', 'y')

If I'm violating the spirit of threadlocal context, just let me know. I would also be happy to add an actual logging context manager here or in another PR if it would be helpful!

with LogContext(a=12, b=32):
  log.msg('mymsg')

@MarkusH
Copy link
Contributor

@MarkusH MarkusH commented Jan 14, 2020

I'm not sure we need the strict and "lenient" implementations. Logging interactions should IMO not cause an application to fail. I'd thus go with a "fail-silent" approach as I've done in the contextvars implementation:

ctx = _get_context()
for key in args:
ctx.pop(key, None)

@hynek
Copy link
Owner

@hynek hynek commented Jan 14, 2020

hmmm the problem is that it's not consistent with structlog's bind/unbind. I guess best way forward would be to add an option and deprecate try_unbind?

@c-wilson
Copy link
Contributor Author

@c-wilson c-wilson commented Jan 14, 2020

Ha, now that I know about the contextvars implementation, I'm second-guessing whether to use threadlocal at all... still happy to see this through though!

While I do tend to agree that "logging should not make your app fail", there are definitely contexts where production of accurate logs are an important side-effect for guiding decisions (ie how much infra do I need, what is my ETL task failure rate, etc.).

Bringing to light double-removals of context could be valuable ie:

def main(var)
  bind_threadlocal(var=var)
  fun1(var)
  fun2()
  log.msg("I finished something with 'a' in my context")
  unbind_threadlocal("var")

def fun1(var):
  bind_threadlocal(var=var)
  # do something
  unbind_threadlocal("var")

def fun2():
  pass

but maybe this highlights that we should use a context manager to roll-back context instead of this interface.

Anyways, let me know what you think, and I can remove try_unbind_threadlocal if that makes sense to you.

@hynek
Copy link
Owner

@hynek hynek commented Jan 18, 2020

Yeah let's just mirror the approach in contextvars please -- thanks!

@c-wilson c-wilson requested a review from hynek Jan 21, 2020
@hynek
Copy link
Owner

@hynek hynek commented Jan 22, 2020

Thanks!

Could you add

  • a change log entry
  • and a .. versionadded:: tag?

Thanks!

@c-wilson
Copy link
Contributor Author

@c-wilson c-wilson commented Jan 22, 2020

Sure thing! Maybe this is a dumb question, but the versionadded will be 20.1.0?

hynek
hynek approved these changes Jan 25, 2020
@hynek hynek merged commit ce4e51a into hynek:master Jan 25, 2020
21 checks passed
@hynek
Copy link
Owner

@hynek hynek commented Jan 25, 2020

Yeah but I'll do it myself real quick – thanks!

hynek added a commit that referenced this issue Jan 25, 2020
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.

None yet

3 participants