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 support for tracing asynchronous code #87

Merged
merged 9 commits into from
Nov 18, 2019

Conversation

niligulmohar
Copy link
Contributor

This change adds AsyncioTracer, which is chosen instead of
SynchronousTracer as the tracer implementation if beeline is
initialized from within an asyncio event loop.

AsyncioTracer stores its trace state in a context variable instead
of in thread local storage. This requires the contextvars module,
which was added in Python 3.7. Otherwise the implementation is shared
with SynchronousTracer. The trace state implementation is moved into
the Trace class to support both implementations.

Context variable assignments are unique per asyncio task, allowing
separate trace state per task. New tasks automatically receive the
same context variable bindings when they are created, which is not
what you want for the trace state, since they need to be able to
diverge between tasks. To fix this, AsyncioTracer sets up a task
factory on the event loop that creates a copy of the trace state for
each task. The task factory chain-calls to the previous one, if one
happens to already be configured.

A new beeline.untraced function decorator is added, to avoid
automatically sharing trace state for asyncio tasks that are meant
to be unrelated.

The async code requires syntax that is incompatible with Python 2.7,
and modules of which all aren't available until Python 3.7. The code
itself checks for the necessary features by handling ImportError.
The test suite/CI jobs now have separate configurations depending on
the Python version.

@niligulmohar
Copy link
Contributor Author

Hi,

Sorry for the big code dump. I wanted to be sure that this approach to adding async support actually worked to begin with.

Please let me know if there is anything i can do to make this acceptable for merging.

@tredman
Copy link
Contributor

tredman commented Nov 12, 2019

Thanks for the PR! Taking a look this week.

@tredman tredman self-requested a review November 12, 2019 18:28
Copy link
Contributor

@tredman tredman left a comment

Choose a reason for hiding this comment

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

Thanks for wading in and implementing this, we've wanted it for a while. :)

Couple issues below and one optional thing but otherwise looking good from my perspective. Note I'm not super experienced with asyncio patterns so it's possible I've missed something, but the synchronous stuff passes my smoke test script and as long as we don't break any existing functionality I'm happy to iterate and work out any asyncio bugs that do show up.

beeline/__init__.py Outdated Show resolved Hide resolved
beeline/__init__.py Outdated Show resolved Hide resolved
beeline/__init__.py Outdated Show resolved Hide resolved
In TestBeeline.test_treaded_trace there are some assertions run in
threads. Assertion failures from these would only appear in the log,
but not fail the test. This change re-raises any exceptions in the
main thread so that they will cause test failures.
This moves the trace state from attributes on the threading.local
object to a single Trace object. The new trace object is made
available as the _trace attribute on SynchronousTracer objects.

Behavior is unmodified except in some corner cases: some additional
operations will log a warning if they are performed while no trace is
started, like updating trace fields.
This change moves all code except the Trace object access
implementation up to the generic Tracer class.
This change adds AsyncioTracer, which is chosen instead of
SynchronousTracer as the tracer implementation if beeline is
initialized from within an asyncio event loop.

AsyncioTracer stores its trace state in a context variable. This
requires the contextvars module, which was added in Python 3.7.

AsyncioTracer also sets up the task factory for the event loop to hook
in a copying of the trace state for each new task. This ensures that
trace state behaves as expected for parallel running tasks.

The CI jobs have been updated to handle that some of the new code uses
async/await syntax that wasn't available in Python 2.7.
@niligulmohar
Copy link
Contributor Author

Rebased on top of v2.10.1 .

@eanakashima eanakashima requested a review from tredman November 15, 2019 19:55
@tredman
Copy link
Contributor

tredman commented Nov 18, 2019

awesome!

@tredman tredman merged commit 5f532c8 into honeycombio:master Nov 18, 2019
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.

2 participants