Add reset_contextvars and update bind_contextvars #339

jab · 2021-07-26T02:15:30Z

Update structlog.contextvars.bind_contextvars() to return a Mapping[str, contextvar.Token] corresponding to the results
of setting the requested contextvars.

This return value is suitable for use with the newly-added structlog.contextvars.reset_contextvars() function, which resets the requested contextvars to their previous values using the given Tokens.

Pull Request Check List

This is just a friendly reminder about the most common mistakes. Please make sure that you tick all boxes. But please read our contribution guide at least once, it will save you unnecessary review cycles!

If an item doesn't apply to your pull request, check it anyway to make it apparent that there's nothing left to do.

Added tests for changed code.
Updated documentation for changed code.
- New functions/classes have to be added to docs/api.rst by hand.
- Changed/added classes/methods/functions have appropriate versionadded, versionchanged, or deprecated directives. Find the appropriate next version in our __init__.py file.
Documentation in .rst files is written using semantic newlines.
Changes (and possible deprecations) are documented in the changelog.

If you have any questions to any of the points above, just submit and ask! This checklist is here to help you, not to deter you from contributing!

hynek · 2021-07-27T07:03:19Z

I’m gonna have to ask you to write some narrative docs for contextvars.rst so I have a clue what this is about. 😅

i have to admit that my understanding of contextvars is rather superficial.

jab · 2021-08-02T00:30:39Z

Done in the latest revision. Is it any clearer now?

jab · 2021-08-02T00:34:17Z

src/structlog/contextvars.py

@@ -98,16 +98,22 @@ def clear_contextvars() -> None:
            k.set(Ellipsis)


-def bind_contextvars(**kw: Any) -> None:
-    """
+def bind_contextvars(**kw: Any) -> Mapping[str, contextvars.Token]:


I think for some reason, Sphinx is getting confused trying to resolve the occurrences of contextvars.Token in these typehints and docstrings, causing a spurious warning (which is getting turned into a spurious error).

I noticed the old-(pre-Sphinx-1.0)-format intersphinx_mapping was being used, so I tried updating it to the up-to-date format, but that wasn't enough to appease Sphinx. 🤷

Meant to say, I also tried several other things (e.g. adding from contextvars import Token and changing the typehint to just Token, and when that didn't work, tried spelling these references a bunch of other ways), but nothing I tried worked, and I gave up after the better part of an hour. I wonder if it's confusing Sphinx that both structlog and the standard library have modules named contextvars?

Unfortunately Sphinx and types is hit and miss. :(

If you can't make it work, add it to

structlog/docs/conf.py

Lines 69 to 81 in b39a720

nitpick_ignore = [

("py:class", "BinaryIO"),

("py:class", "ILogObserver"),

("py:class", "PlainFileObserver"),

("py:class", "TLLogger"),

("py:class", "TextIO"),

("py:class", "traceback"),

("py:class", "structlog._base.BoundLoggerBase"),

("py:class", "structlog.dev._Styles"),

("py:class", "structlog.types.EventDict"),

("py:obj", "sync_bl"),

("py:obj", "entries"),

]

Thanks! Done in the latest revision. (After rebasing on top of latest main, I also had to add an entry for bare "TLLogger", to ignore a new warning I was getting that is unrelated to my changes.)

hynek

Just a few stylistic changes.

Other than that, it needs a changelog entry. I would suggest some high-level description telling why ppl should be excited?

Maybe something like:

``structlog.contextvars.bind_contextvars()`` now returns a mapping of keys to ``contextvars.Token``\ s that allow to reset values using the new ``structlog.contextvars.reset_contextvars()``.

hynek · 2021-08-28T14:02:10Z

docs/contextvars.rst

+If e.g. your request handler calls a helper function that needs to
+temporarily override some contextvars before restoring them back to
+their original values, you can use the :class:`~contextvars.contextvars.Token`\s
+returned by :func:`~structlog.contextvars.bind_contextvars` along with
+:func:`~structlog.contextvars.reset_contextvars` to accomplish this
+(much like how :meth:`contextvars.ContextVar.reset` works):


Suggested change

If e.g. your request handler calls a helper function that needs to

temporarily override some contextvars before restoring them back to

their original values, you can use the :class:`~contextvars.contextvars.Token`\s

returned by :func:`~structlog.contextvars.bind_contextvars` along with

:func:`~structlog.contextvars.reset_contextvars` to accomplish this

(much like how :meth:`contextvars.ContextVar.reset` works):

If e.g. your request handler calls a helper function that needs to temporarily override some contextvars before restoring them back to their original values, you can use the :class:`~contextvars.contextvars.Token`\s returned by :func:`~structlog.contextvars.bind_contextvars` along with :func:`~structlog.contextvars.reset_contextvars` to accomplish this (much like how :meth:`contextvars.ContextVar.reset` works):

We use semantic newlines.

Done in the latest revision.

(Though, as a fan of semantic line breaks myself, I was surprised to get this suggestion. I think adding additional line breaks at appropriate positions within a single sentence is actually encouraged, not forbidden. See the example at https://sembr.org starting with "We can further clarify the source text by adding a line break after the clause". This makes reading and editing docs (and diffs) easier in editors that are not set to soft-wrap long lines, as well as pagers and other tools.)

hynek · 2021-08-28T14:03:14Z

src/structlog/contextvars.py

@@ -116,7 +122,20 @@ def bind_contextvars(**kw: Any) -> None:
            var = contextvars.ContextVar(structlog_k, default=Ellipsis)
            _CONTEXT_VARS[structlog_k] = var

-        var.set(v)
+        rv[k] = var.set(v)
+    return rv


Suggested change

return rv

return rv

Done in the latest revision.

hynek · 2021-08-28T14:04:37Z

tests/test_contextvars.py

+            bind_contextvars(a=1)
+            assert {"a": 1} == get_contextvars()
+            await event_loop.create_task(nested_coro())


Suggested change

bind_contextvars(a=1)

assert {"a": 1} == get_contextvars()

await event_loop.create_task(nested_coro())

bind_contextvars(a=1)

assert {"a": 1} == get_contextvars()

await event_loop.create_task(nested_coro())

Done in the latest revision.

hynek · 2021-08-28T14:05:10Z

tests/test_contextvars.py

+            assert {"a": 2, "b": 3} == get_contextvars()
+            reset_contextvars(**tokens)
+            assert {"a": 1} == get_contextvars()


Suggested change

assert {"a": 2, "b": 3} == get_contextvars()

reset_contextvars(**tokens)

assert {"a": 1} == get_contextvars()

assert {"a": 2, "b": 3} == get_contextvars()

reset_contextvars(**tokens)

assert {"a": 1} == get_contextvars()

Done in the latest revision.

jab

Thanks for reviewing, @hynek! I believe I've addressed all your requested changes.

jab · 2021-09-11T21:42:56Z

docs/contextvars.rst

+If e.g. your request handler calls a helper function that needs to
+temporarily override some contextvars before restoring them back to
+their original values, you can use the :class:`~contextvars.contextvars.Token`\s
+returned by :func:`~structlog.contextvars.bind_contextvars` along with
+:func:`~structlog.contextvars.reset_contextvars` to accomplish this
+(much like how :meth:`contextvars.ContextVar.reset` works):


Done in the latest revision.

(Though, as a fan of semantic line breaks myself, I was surprised to get this suggestion. I think adding additional line breaks at appropriate positions within a single sentence is actually encouraged, not forbidden. See the example at https://sembr.org starting with "We can further clarify the source text by adding a line break after the clause". This makes reading and editing docs (and diffs) easier in editors that are not set to soft-wrap long lines, as well as pagers and other tools.)

jab · 2021-09-11T21:50:33Z

src/structlog/contextvars.py

@@ -98,16 +98,22 @@ def clear_contextvars() -> None:
            k.set(Ellipsis)


-def bind_contextvars(**kw: Any) -> None:
-    """
+def bind_contextvars(**kw: Any) -> Mapping[str, contextvars.Token]:


Thanks! Done in the latest revision. (After rebasing on top of latest main, I also had to add an entry for bare "TLLogger", to ignore a new warning I was getting that is unrelated to my changes.)

jab · 2021-09-11T21:50:47Z

src/structlog/contextvars.py

@@ -116,7 +122,20 @@ def bind_contextvars(**kw: Any) -> None:
            var = contextvars.ContextVar(structlog_k, default=Ellipsis)
            _CONTEXT_VARS[structlog_k] = var

-        var.set(v)
+        rv[k] = var.set(v)
+    return rv


Done in the latest revision.

jab · 2021-09-11T21:50:55Z

tests/test_contextvars.py

+            bind_contextvars(a=1)
+            assert {"a": 1} == get_contextvars()
+            await event_loop.create_task(nested_coro())


Done in the latest revision.

jab · 2021-09-11T21:50:59Z

tests/test_contextvars.py

+            assert {"a": 2, "b": 3} == get_contextvars()
+            reset_contextvars(**tokens)
+            assert {"a": 1} == get_contextvars()


Done in the latest revision.

hynek

mypy is failing:

src/structlog/contextvars.py:101: error: Missing type parameters for generic type "Token"
src/structlog/contextvars.py:130: error: Missing type parameters for generic type "Token"

I suspect (vacationing rn) you have to set it to Token[Any]?

jab · 2021-09-19T16:06:46Z

@hynek should be fixed now! Thanks for catching that, and sorry I missed that when testing locally. (Each time I update this PR, the CI checks don't run until you approve running them, since this is my first structlog PR. If there were some way for you to pre-approve running the checks for future updates of this PR, I would have noticed and fixed that by the time you took your next look. Not sure if that's possible?)

Hope you've been having a great vacation!

hynek · 2021-09-19T16:12:44Z

Sorry terse because phone:

any reason why running tox Locally doesn’t work for you?
I think we’ll have to go for a string annotation :(
I’m not aware of a way to pre-approve users. If you find anything lmk. But it’ll be moot once a single PR by you is merged.

structlog.contextvars.bind_contextvars() now returns a Mapping[str, contextvar.Token] corresponding to the results of setting the requested contextvars. This return value is suitable for use with the newly-added structlog.contextvars.reset_contextvars() function, which resets the requested contextvars to their previous values using the given Tokens.

jab · 2021-09-19T16:34:07Z

tox is now succeeding for me locally as of the latest revision. I think 3.6, 3.7, 3.8, 3.9, and docs should all succeed in CI once they run next.

Not sure what's going on with the required codecov status checks. Please let me know if there's anything else you need me to do about those. Thanks for reviewing!

hynek · 2021-09-19T16:36:35Z

Quickly approved before leaving. As long as builders are failing, you can ignore codecov, because of fast-failing alone (you can see that some builders don’t even start).

jab · 2021-09-19T16:37:34Z

All checks have passed

🎉

hynek · 2021-09-19T16:46:48Z

Thanks for your perseverance!

jab · 2021-09-19T17:10:03Z

Thanks for merging! And for structlog and your other great work!

hynek · 2021-11-12T14:25:12Z

JFTR, I've run into contextvars.Token problems myself and it's actually a bug in CPython's docs!

So I've fixed it today: python/cpython#29533

jab force-pushed the reset branch 3 times, most recently from 70fa295 to 4aa209b Compare August 2, 2021 00:27

jab commented Aug 2, 2021

View reviewed changes

hynek requested changes Aug 28, 2021

View reviewed changes

jab force-pushed the reset branch from ceaca1e to 8a7e829 Compare September 11, 2021 21:40

jab commented Sep 11, 2021

View reviewed changes

hynek requested changes Sep 16, 2021

View reviewed changes

jab force-pushed the reset branch from e266269 to f66b4e5 Compare September 19, 2021 16:01

jab force-pushed the reset branch from f66b4e5 to fc2f159 Compare September 19, 2021 16:17

jab force-pushed the reset branch from fc2f159 to ed3c6e2 Compare September 19, 2021 16:28

hynek merged commit 3a08abd into hynek:main Sep 19, 2021

jab deleted the reset branch September 19, 2021 17:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add reset_contextvars and update bind_contextvars #339

Add reset_contextvars and update bind_contextvars #339

jab commented Jul 26, 2021

hynek commented Jul 27, 2021

jab commented Aug 2, 2021

jab Aug 2, 2021 •

edited

Loading

jab Aug 2, 2021

hynek Aug 28, 2021

jab Sep 11, 2021

hynek left a comment

hynek Aug 28, 2021

jab Sep 11, 2021

hynek Aug 28, 2021

jab Sep 11, 2021

hynek Aug 28, 2021

jab Sep 11, 2021

hynek Aug 28, 2021

jab Sep 11, 2021

jab left a comment

jab Sep 11, 2021

jab Sep 11, 2021

jab Sep 11, 2021

jab Sep 11, 2021

jab Sep 11, 2021

hynek left a comment •

edited

Loading

jab commented Sep 19, 2021

hynek commented Sep 19, 2021

jab commented Sep 19, 2021

hynek commented Sep 19, 2021

jab commented Sep 19, 2021

hynek commented Sep 19, 2021

jab commented Sep 19, 2021

hynek commented Nov 12, 2021

	nitpick_ignore = [
	("py:class", "BinaryIO"),
	("py:class", "ILogObserver"),
	("py:class", "PlainFileObserver"),
	("py:class", "TLLogger"),
	("py:class", "TextIO"),
	("py:class", "traceback"),
	("py:class", "structlog._base.BoundLoggerBase"),
	("py:class", "structlog.dev._Styles"),
	("py:class", "structlog.types.EventDict"),
	("py:obj", "sync_bl"),
	("py:obj", "entries"),
	]

Add reset_contextvars and update bind_contextvars #339

Add reset_contextvars and update bind_contextvars #339

Conversation

jab commented Jul 26, 2021

Pull Request Check List

hynek commented Jul 27, 2021

jab commented Aug 2, 2021

jab Aug 2, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hynek left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jab left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hynek left a comment • edited Loading

Choose a reason for hiding this comment

jab commented Sep 19, 2021

hynek commented Sep 19, 2021

jab commented Sep 19, 2021

hynek commented Sep 19, 2021

jab commented Sep 19, 2021

hynek commented Sep 19, 2021

jab commented Sep 19, 2021

hynek commented Nov 12, 2021

jab Aug 2, 2021 •

edited

Loading

hynek left a comment •

edited

Loading