Adds some WebClient options #278

aoberoi · 2019-10-04T21:28:31Z

Summary

This PR adds the following options to the App constructor:

agent: Allows users to set a custom HTTP agent, which is most often used for setting up proxy support. Will affect both the client HTTP requests made from the WebClient and the ExpressReceiver (if the default receiver is used).
clientTls: Allows users to set a custom TLS configuration for HTTP client requests. This is most often used to send requests to a specific Slack environment (internal usage). Will affect both the client HTTP requests made from the WebClient and the ExpressReceiver (if the default receiver is used).
clientOptions.slackApiUrl: Allows users to set a custom endpoint for the Slack API. This is most often used for testing as well as sending requests to a specific Slack environment (internal usage). This only affects the WebClient.

It also changes the behavior of the logger option to also affect the WebClient.

Fixes #221

Requirements (place an `x` in each `[ ]`)

I've read and understood the Contributing Guidelines and have done my best effort to follow them.
I've read and agree to the Code of Conduct.

codecov · 2019-10-04T21:30:28Z

Codecov Report

Merging #278 into master will increase coverage by 0.16%.
The diff coverage is 91.66%.

@@            Coverage Diff             @@
##           master     #278      +/-   ##
==========================================
+ Coverage   72.48%   72.65%   +0.16%     
==========================================
  Files           7        7              
  Lines         498      501       +3     
  Branches      138      142       +4     
==========================================
+ Hits          361      364       +3     
  Misses        105      105              
  Partials       32       32

Impacted Files	Coverage Δ
src/App.ts	`81.48% <100%> (ø)`	⬆️
src/ExpressReceiver.ts	`52.77% <85.71%> (+1.34%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 54bf827...7654091. Read the comment docs.

seratch · 2019-10-05T00:14:02Z

src/ExpressReceiver.ts

  }: ExpressReceiverOptions) {
    super();

    this.app = express();
    this.app.use(this.errorHandler.bind(this));
    // TODO: what about starting an https server instead of http? what about other options to create the server?
    this.server = createServer(this.app);
+    this.axios = axios.create(Object.assign(


Ah, I overlooked the necessity to support the respond function as well. Great catch 🙇

seblaz · 2020-04-06T00:22:21Z

Hi, is the custom logger passed to the WebClient? I see the pull request has been merged but it doesn't seem to be in the package

seratch · 2020-04-06T00:27:23Z

@seblaz 👋 Can you try version ^1.4.0? (the latest versions are 1.8.0 or 2.0.0). https://github.com/slackapi/bolt/releases/tag/%40slack%2Fbolt%401.4.0

seblaz · 2020-04-06T00:36:49Z

Hi @seratch, I'm using the latest version (2.0.0). I dig a little and found that this feature (using a custom logger in the WebClient) was removed in commit 1428ec3. I feel that this has been discused before, but why did you chose to remove it? I would find it useful to use a custom logger in the WebClient.

seratch · 2020-04-06T01:22:29Z

@seblaz
The main reason was this issue on the @slack/web-api side. slackapi/node-slack-sdk#916 However, I realized that now it's safe to share a customer logger with WebClient as I fixed the issue by slackapi/node-slack-sdk#917. So, it's safe to share a logger given in App's constructor with underlying WebClients now.

Another minor reason is to have different names among a Bolt app itself and WebClient instances. The reason why "instances" is plural is that Bolt internally has a pool of WebClient instances (per token) for better consideration of rate limiter.

I've changed the behavior before but I'm positive to revisit this. There are two solutions.

Solution 1) Get it back to the previous behavior @seblaz mentioned
- This means sharing a single logger among a Bolt app and WebClients (all the logging will be using the default name bolt-app or custom name)
Solution 2) Introduce webClientLogger option
- logger option won't be shared with WebClients in any case
- If both options are missing, Bolt initializes loggers for Bolt itself and WebClients respectively

Both look fine to me but I prefer Solution 2) as it enables developers to take control of logging much more. What do you think? @aoberoi @stevengill

aoberoi · 2020-04-13T22:28:01Z

I also prefer Solution 2. I think the name separation is a meaningful advantage. Similarly, I think its a bit odd that the name of the ExpressReceivers logger is also bolt-app since the change mentioned above. @seratch, how would you feel about not passing the logger option to ExpressReceiver, only the logLevel option. If the user wants to control the specific logger used by the receiver, they would have to pass a receiver into the App constructor.

seratch · 2020-04-14T01:04:10Z

@seratch, how would you feel about not passing the logger option to ExpressReceiver, only the logLevel option. If the user wants to control the specific logger used by the receiver, they would have to pass a receiver into the App constructor.

This sounds nice to me!

@aoberoi
What do you think about the case the ExpressReceiver is initialized in App?
1️⃣ Should the name bolt-app be shared with the receiver? Or 2️⃣ should the receiver have a different default name like bolt-default-receiver or bolt-receiver? I prefer 2️⃣ if we enhance it this time.

aoberoi · 2020-04-14T21:06:14Z

@seratch i prefer a different name. in my opinion the point of the logger is for tracing and understand which parts of the code are run. so putting a name that matches with the filename is actually what i would prefer. for example, i would change bolt-app to @slack/bolt:App and the logger for ExpressReceiver would look like @slack/bolt:ExpressReceiver. I realize this is verbose so I'm okay with some other format - I just want to preserve idea that you know which package and which file in the package is emitting the log line.

Also, if we provide a new option for specifying the WebClient instances' logger, instead of webClientLogger, i think it would make sense to reuse the clientOptions option by adding clientOptions.logger.

seratch · 2020-04-14T22:18:43Z

@aoberoi
I like the whole idea except for the prefix @slack/bolt:.
@slack/web-api outputs just WebClient:{instanceId}. What do you think about going with just App and ExpressReceiver? I would like the logging to be consistent among the tools we provide.

stevengill · 2020-04-16T18:42:47Z

why not bolt-app and ExpressReceiver. App seems a little generic on its own

aoberoi · 2020-04-16T19:03:15Z

Because `bolt-app` isn’t a clear reference to the class name or file name `App`. I agree that App on its own feels generic. How about a shorter prefix? `bolt:App:{instanceId}` As for consistency, maybe in the future we could add a prefix (such as `web-api`) for the node-slack-sdk packages. This should also be a little less verbose. @seratch: what do you think?

On Thu, Apr 16, 2020 at 11:43 AM Steve Gill ***@***.***> wrote: why not bolt-app and ExpressReceiver. App seems a little generic on its own — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#278 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AADYZS4MDIZSQAUPJWUR2QTRM5GTPANCNFSM4I5UTFAQ> .

-- Ankur Oberoi Developer Relations Manager, Developer Tools Lead | Slack @aoberoi (github <https://github.com/aoberoi>, facebook <https://facebook.com/aoberoi>, twitter <https://twitter.com/aoberoi>)

seratch · 2020-04-16T22:24:58Z

@aoberoi I like the idea. I've created a task on the node-slack-sdk side: slackapi/node-slack-sdk#995

seratch · 2020-04-23T23:46:37Z

On the node-slack-sdk side, @stevengill created a fix slackapi/node-slack-sdk#999 for slackapi/node-slack-sdk#995
It looks good to me. Thanks!

adds agent, clientTls, and clientOptions.slackApiUrl App options

4c6ba6c

WebClient allowing custom logger

7654091

aoberoi requested a review from shaydewael October 4, 2019 21:49

seratch reviewed Oct 5, 2019

View reviewed changes

shaydewael approved these changes Oct 7, 2019

View reviewed changes

shaydewael merged commit 2e11041 into slackapi:master Oct 7, 2019

seratch mentioned this pull request Apr 16, 2020

Having a prefix to the logger name slackapi/node-slack-sdk#995

Closed

9 tasks

aoberoi mentioned this pull request May 13, 2020

Add clientOptions.logger option #496

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds some WebClient options #278

Adds some WebClient options #278

aoberoi commented Oct 4, 2019 •

edited

Loading

codecov bot commented Oct 4, 2019 •

edited

Loading

seratch Oct 5, 2019

seblaz commented Apr 6, 2020

seratch commented Apr 6, 2020

seblaz commented Apr 6, 2020

seratch commented Apr 6, 2020 •

edited

Loading

aoberoi commented Apr 13, 2020

seratch commented Apr 14, 2020

aoberoi commented Apr 14, 2020 •

edited

Loading

seratch commented Apr 14, 2020

stevengill commented Apr 16, 2020

aoberoi commented Apr 16, 2020 via email

seratch commented Apr 16, 2020

seratch commented Apr 23, 2020

Adds some WebClient options #278

Adds some WebClient options #278

Conversation

aoberoi commented Oct 4, 2019 • edited Loading

Summary

Requirements (place an x in each [ ])

codecov bot commented Oct 4, 2019 • edited Loading

Codecov Report

seratch Oct 5, 2019

Choose a reason for hiding this comment

seblaz commented Apr 6, 2020

seratch commented Apr 6, 2020

seblaz commented Apr 6, 2020

seratch commented Apr 6, 2020 • edited Loading

aoberoi commented Apr 13, 2020

seratch commented Apr 14, 2020

aoberoi commented Apr 14, 2020 • edited Loading

seratch commented Apr 14, 2020

stevengill commented Apr 16, 2020

aoberoi commented Apr 16, 2020 via email

seratch commented Apr 16, 2020

seratch commented Apr 23, 2020

aoberoi commented Oct 4, 2019 •

edited

Loading

Requirements (place an `x` in each `[ ]`)

codecov bot commented Oct 4, 2019 •

edited

Loading

seratch commented Apr 6, 2020 •

edited

Loading

aoberoi commented Apr 14, 2020 •

edited

Loading