Closed danielmorell closed 1 year ago
@waltjones I appreciate the feedback. Your input has been very helpful! I agree that adding a default for Python < 3.5 would be helpful. I added a new commit https://github.com/rollbar/pyrollbar/pull/416/commits/e14af00922ade40beac9cf578ad42f113eac5370 with the same default as Python 3.5.
I will make sure the details are added to the documentation once we publish the release. I will also probably update the explanation of the "thread" handler in the documentation, so it is clear.
Let me know if you have any further thoughts.
@danielmorell Looks good, and thank you for the doc improvements!
Description of the change
Here are the high-level changes...
thread_pool
that utilizes a pool of worker threads to send message to the Rollbar services asynchronously off the main thread.thread_pool_workers
to allow users control of the number of worker threads.0.16.4beta
. As this may need some tweaks/modifications before we go stable.Now all the considerations...
First, Python version compatibility. Using this requires a minimum of Python version of 3.2. However, depending on the version the behavior may be slightly different. This is because of how Python calculates the maximum number of workers (if the user did not specify it with
thread_pool_workers
). The calculation changed in 3.5 and 3.8 (see the docs on ThreadPoolExecutor for details).Second, stability. I had thought about updating the default handler to use a tread pool instead of creating a new thread for every message (that is what it currently does). However, this seems too early to make such a change. Any new handler should really be put through its paces under real world conditions, and ideally be used in production codebases for some time before we even consider changing the default.
Third, max workers. The default for
thread_pool_workers
isNone
. This is passed to themax_workers
argument forThreadPoolExecutor.__init__()
. I don't believe a default was added until Python 3.5. This means that our default may not work on versions 3.2 to 3.4. Users will likely need to explicitly set the number of workers. On more recent versionsNone
will cause Python to calculate it based on the number of CPUs (see the link to the docs above).Are we okay with the default not working on every version, or should we provide a different default on lower versions of Python?
Both the implementation and the tests are pretty minimal. If anyone thinks of anything that should also be tested let me know and I will add it. Feedback is very much appreciated!
Type of change
Related issues
Checklists
Development
Code review