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

Added new thread_pool handler #416

Merged
merged 4 commits into from
Nov 26, 2022
Merged

Added new thread_pool handler #416

merged 4 commits into from
Nov 26, 2022

Conversation

danielmorell
Copy link
Collaborator

Description of the change

Here are the high-level changes...

  1. This adds a new HTTP request handler called thread_pool that utilizes a pool of worker threads to send message to the Rollbar services asynchronously off the main thread.
  2. A new setting has been added thread_pool_workers to allow users control of the number of worker threads.
  3. It also updates the version to 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 is None. This is passed to the max_workers argument for ThreadPoolExecutor.__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 versions None 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

  • Bug fix (non-breaking change that fixes an issue)
  • New feature (non-breaking change that adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Maintenance
  • New release

Related issues

Checklists

Development

  • Lint rules pass locally
  • The code changed/added as part of this pull request has been covered with tests
  • All tests related to the changed code pass in development

Code review

  • This pull request has a descriptive title and information useful to a reviewer. There may be a screenshot or screencast attached
  • "Ready for review" label attached to the PR and reviewers assigned
  • Issue from task tracker has a link to this pull request
  • Changes have been reviewed by at least one other engineer

Copy link
Contributor

@waltjones waltjones left a comment

Choose a reason for hiding this comment

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

@danielmorell Thank you for the thoughtful analysis. It's been a pleasure to review this PR.

Regarding version compatibility, I think the difference in max workers for 3.5 and higher is fine since it is based on the python version the user chose, and is consistent with other uses of ThreadPoolExecutor in the host application. They can always override if they have a better idea.

For python < 3.5, maybe we should set a reasonable default value. It's OK for this to be a different default than other versions as long as it's documented in the SDK docs. If we don't, we still need to suggest a reasonable value in the doc, so this requires some additional documentation regardless.

Good call on the beta version and keeping the current default handler for now.

This is approved as is, but feel free to update with the version dependent default if you agree.

👍

@danielmorell
Copy link
Collaborator Author

@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 e14af00 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.

@waltjones
Copy link
Contributor

@danielmorell Looks good, and thank you for the doc improvements!

@danielmorell danielmorell merged commit 55d08a6 into master Nov 26, 2022
@danielmorell danielmorell deleted the added-thread-pool branch November 26, 2022 20:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants