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

Flush async worker queue immediately on exit #384

Merged

Conversation

c24t
Copy link
Member

@c24t c24t commented Nov 6, 2018

This diff changes the behavior of AsyncTransport such that its worker thread starts exporting items in its queue immediately when the process exits.

Currently the worker thread exports a batch of items from the queue at an interval of _WAIT_PERIOD, and waits for up to grace_period on exit to finish draining the queue. Since we don't trigger an export on exit this means that the thread can block for up to _WAIT_PERIOD before doing any work, while preventing the process from exiting.

#354 changed _WAIT_PERIOD from 1s to 60s. The default grace_period is 5s. Before #354, any process that spawned a worker thread to be killed on exit would block for up to 1s (plus the time to export pending items), after the change these processes would block for up to 5s. As a result, tests that created a lot of AsyncTransports -- like TestStackdriverStatsExporter -- would block for a long time on exit.

I think this is the behavior we want, but it is a significant change. On exit, the transport will now export data in a tight loop for up to grace_period, when before it would never export items at a rate faster than batch_size / _WAIT_PERIOD.

@c24t
Copy link
Member Author

c24t commented Nov 6, 2018

Compare some offending tests on this branch and master:

python -m pytest -s 'tests/unit/stats/exporter/test_stackdriver_stats.py::TestStackdriverStatsExporter'

On my machine this hangs for over a minute after the tests complete on master and finishes in under a second on this branch.

@c24t c24t self-assigned this Nov 7, 2018
Copy link
Contributor

@liyanhui1228 liyanhui1228 left a comment

Choose a reason for hiding this comment

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

LGTM

@c24t
Copy link
Member Author

c24t commented Nov 7, 2018

Thanks @liyanhui1228. I'll make the same changes in the trace package with #386.

@c24t c24t merged commit 32ee2c8 into census-instrumentation:master Nov 7, 2018
@c24t c24t deleted the fix-async-transport-blocking branch November 7, 2018 18:01
@ocervell
Copy link
Contributor

Thanks for this. Have we released a OC version with this change in ?

@c24t
Copy link
Member Author

c24t commented Nov 16, 2018

@ocervell not yet, and we don't have an ETA for the next release.

Obviously not a great solution, but if you want to use bleeding-edge unreleased changes in the meantime you can do an editable install, e.g.:

pip install -e '[email protected]:census-instrumentation/opencensus-python.git@32ee2c8#egg=opencensus-python'

c24t added a commit to c24t/opencensus-python that referenced this pull request Nov 21, 2018
c24t added a commit to c24t/opencensus-python that referenced this pull request Nov 21, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants