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

Enable the logging of the request body #374

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ts-3156
Copy link

@ts-3156 ts-3156 commented May 6, 2021

When debugging, I sometimes want to see the request body. I don't know if other people feel the same way. If you don't need it, please feel free to close this pull request.

@dangerpr-bot
Copy link

2 Warnings
⚠️ There're library changes, but not tests. That's OK as long as you're refactoring existing code.
⚠️ Unless you're refactoring existing code or improving documentation, please update CHANGELOG.md.

Here's an example of a CHANGELOG.md entry:

* [#374](https://github.com/slack-ruby/slack-ruby-client/pull/374): Enable the logging of the request body - [@ts-3156](https://github.com/ts-3156).

Generated by 🚫 Danger

@dblock
Copy link
Collaborator

dblock commented May 6, 2021

I think we don't want this by default. Did you figure out a way to do this in your own code? I would like us to document how to do this without changing the default configuration, in README.

@ts-3156
Copy link
Author

ts-3156 commented May 7, 2021

@dblock Thank you for taking the time to reply.

I think we don't want this by default.

I have the same opinion. However, I have not come up with a suitable code to pass a value for bodies: true via Slack.configure.

Slack.configure do |config|
  config.log_bodies = true # <- This name feels strange.
end

Did you figure out a way to do this in your own code?

Yes. I found this option when I investigated strange behavior that Slack doesn't display the image preview correctly.

I would like us to document how to do this without changing the default configuration, in README.

If you need this option for logging, I will write how to do it in README. However, in order to use this option, I need to modify not just the documentation, but also the code. Is this really what you want?

You don't need to hurry. It is OK if this idea is discarded.

@dblock
Copy link
Collaborator

dblock commented May 7, 2021

How about introducing logger_options?

If you feel more ambitious, take a look at https://lostisland.github.io/faraday/middleware/logger. It also supports blocks and filtering which we could expose. The https://github.com/ashkan18/graphlient gem does it like so:

 Graphlient::Client.new('http://test-graphql.biz/graphql') do |client|
      client.http do |h|
        h.connection do |c|
          c.adapter Faraday::Adapter::Rack, app
        end
      end
    end

So http and connection yield from within the configuration and you would want to be able to write the following, overriding the :logger part of the configuration.

 Slack.configure do |config|
   config.connection do |conn|
     conn.response :logger, logger, { bodies: true }
   end
 end
end

@dblock dblock force-pushed the master branch 4 times, most recently from 1963b52 to 097b3ca Compare April 30, 2023 03:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants