-
Notifications
You must be signed in to change notification settings - Fork 276
fix: honor passing bolt logger in default web client installation #1272
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
fix: honor passing bolt logger in default web client installation #1272
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1272 +/- ##
=======================================
Coverage 90.96% 90.96%
=======================================
Files 222 222
Lines 7501 7501
=======================================
Hits 6823 6823
Misses 678 678 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
zimeg
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤔 QQ: Do we want to change the slack_sdk logger if no logger is provided to slack_bolt?
Before these changes I found API requests logged as so:
DEBUG:slack_sdk.web.base_client:Sending a request - url: https://slack.com/api/views.publish
But with the default setup this is changed to:
DEBUG:slack_bolt.App:Sending a request - url: https://slack.com/api/views.publish
I think both cases make sense, but I might be used to having the default slack_sdk logs shown here 📣
| assert response.body == "" | ||
| assert_auth_test_count(self, 1) | ||
|
|
||
| def test_custom_web_client_logger_is_used_instead_of_bolt_app_logger(self): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👏 Awesome checks!
zimeg
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A bit more testing is showing that a custom logger setup with the following outputs as expected 🚀
bolt = logging.getLogger("zaps")
logs.setLevel(logging.DEBUG)
app = App(logger=logs, token=os.environ.get("SLACK_BOT_TOKEN"))DEBUG:zaps:Sending a request - url: https://slack.com/api/views.publish
Even fun combinations of logger setups are logging to the right outputs:
bolt = logging.getLogger("zaps")
bolt.setLevel(logging.DEBUG)
webapi = logging.getLogger("zips")
webapi.setLevel(logging.DEBUG)
app = App(logger=bolt, client=WebClient(token=os.environ.get("SLACK_BOT_TOKEN"), logger=webapi))LGTM if the comment beforehand seems right to you, but I'm open to more discussion whenever!
|
🏷️ Small nit to the milestone used, we could add this PR to |
|
🤔 QQ: Do we want to change the slack_sdk logger if no logger is provided to slack_bolt? This is a good point to bring up, from my investigation before #714 the default slack_bolt logger was being injected as the slack_sdk logger, the default logger of the parent client in the app is the bolt framework logger. bolt-python/slack_bolt/app/app.py Line 242 in 4c237fe
|
Summary
This aims to resolve #1255
#246 defined passing the Bolt logger as the default logger for WebClient instantiation. Since #714 Bolt creates a new WebClient for each request handling execution, this is to prevent using the singleton pattern.
These changes aim to honor both previous decisions by passing the
app client loggerinto each WebClient created for request handling.Testing
Default WebClient logger is the Bolt App logger
Custom logger can be set on WebClient
Category
slack_bolt.Appand/or its core componentsslack_bolt.async_app.AsyncAppand/or its core componentsslack_bolt.adapter/docsRequirements
Please read the Contributing guidelines and Code of Conduct before creating this issue or pull request. By submitting, you are agreeing to those rules.
./scripts/install_all_and_run_tests.shafter making the changes.