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

Suppress request strategy log messages for every request #543

Merged
merged 1 commit into from
Jul 23, 2017

Conversation

Borzik
Copy link
Contributor

@Borzik Borzik commented Jul 22, 2017

Right now all requests are being wrapped with Chewy.strategy(Chewy.request_strategy) { @app.call(env) }. So every request will have two log entries for default Chewy.request_strategy added and removed from stack.
Here's what current log looks like:

Started GET "/articles" for 127.0.0.1 at 2017-07-22 20:25:44 +0300
DEBUG: Chewy strategies stack: [2] <- atomic @ **/chewy/lib/chewy/railtie.rb:17
Processing by ArticlesController#shared as HTML
  ArticlesIndex::Article Search (5.9ms) {:index=>["designs"], ***}
Rendered ActiveModel::Serializer::CollectionSerializer with ActiveModelSerializers::Adapter::JsonApi (2.3ms)
Completed 200 OK in 12ms (Views: 2.9ms | ActiveRecord: 0.0ms)
DEBUG: Chewy strategies stack: [2] -> atomic @ **/chewy/lib/chewy/railtie.rb:17

Since most developers are aware of their Chewy.request_strategy setting, there's no much need to remind them about it each time. So I decided to add a log message which will appear on server load (added to railties.rb). Also I've removed DEBUG: portion of a message because it's quite obvious anyway, and it is a responsibility of Logger::Formatter to format logs.
This is what it looks like now:

# somewhere in server load log
=> Run `rails server -h` for more startup options
Chewy strategies stack: [1] <- atomic

For every request that doesn't change strategy, it will not show any strategy messages:

Started GET "/articles" for 127.0.0.1 at 2017-07-22 20:25:44 +0300
Processing by ArticlesController#shared as HTML
  ArticlesIndex::Article Search (5.9ms) {:index=>["articles"], ***}
Rendered ActiveModel::Serializer::CollectionSerializer with ActiveModelSerializers::Adapter::JsonApi (2.3ms)
Completed 200 OK in 12ms (Views: 2.9ms | ActiveRecord: 0.0ms)

I have also added a now message when a strategy is removed from stack - that makes it much easier to follow strategy changes. And here's what happens if we wrap controller action with Chewy.strategy(:bypass) block:

Started GET "/articles" for 127.0.0.1 at 2017-07-22 20:25:44 +0300
Chewy strategies stack: [2] <- bypass @ **/app/controllers/articles_controller.rb:24
Processing by ArticlesController#shared as HTML
  ArticlesIndex::Article Search (5.9ms) {:index=>["articles"], ***}
Rendered ActiveModel::Serializer::CollectionSerializer with ActiveModelSerializers::Adapter::JsonApi (2.3ms)
Chewy strategies stack: [2] -> bypass, now atomic @ **/app/controllers/articles_controller.rb:24
Completed 200 OK in 12ms (Views: 2.9ms | ActiveRecord: 0.0ms)

That now atomic message shows which strategy will be used after bypass is removed from stack.

Please let me know if that makes sense.

@pyromaniac
Copy link
Contributor

How about to make it less noisy without any setting?

@Borzik
Copy link
Contributor Author

Borzik commented Jul 22, 2017

Usually I try to make PRs which don't change previous behavior (which becomes default one) because I don't know the reason why it was done that way. If you think there's no need for such setting and initial strategy log entry can be suppressed for everyone - I can do that. I myself would prefer that message to appear only on server startup, or never.

@pyromaniac
Copy link
Contributor

Sure, agree, let's suppress it a bit

@Borzik Borzik changed the title Add log_initial_strategy setting Suppress request strategy log messages for every request Jul 23, 2017
@Borzik
Copy link
Contributor Author

Borzik commented Jul 23, 2017

@pyromaniac I've updated PR and its description. Please let me know your thoughts about it. Also let me know if the default request strategy notifier on server load is needed.

@pyromaniac
Copy link
Contributor

Looks good, just fix Rubocop offences

@Borzik
Copy link
Contributor Author

Borzik commented Jul 23, 2017

@pyromaniac done

@pyromaniac pyromaniac merged commit ff49175 into toptal:master Jul 23, 2017
@pyromaniac
Copy link
Contributor

Cool, merged, thanks!

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.

2 participants