jruby
Include logger silence from active support
Hi @jlahtinen - slightly unrelated to this, but which rack version are you using jruby-rack with?
Hi @jlahtinen - slightly unrelated to this, but which rack version are you using jruby-rack with?
rack (2.2.9)
Interesting. Do you use with rails? (Probably a.dumb question given you are trying to make it work better with Puma here - so probably 'no' which would explain why you don't have some of the same issues I see, and the rails appraisal tests have)
@chadlwilson sorry I did not understand puma reasoning :). Most of the developer time goes running rails app with puma.
We are using rails (6.1.7.7) currently
In production there is tomcat 9 and jryby-rack in action
OK my bad, I don't know Puma - I thought it might include a framework alternative to Rails, and was confused about your earlier references to using Tomcat as webserver if there is also Puma in the mix.
But that's probably similar to my setup in a sense (Rails 6.1).
PR still needs work to make tests pass as well.
Id also feel more comfortable if we get #271 in first which tests more with rails by enabling the old rails appraisals and will avoid unintentionally making certain breaking changes for certain rails versions.
I also think this is not ready to be merged. But I hope we can discuss how silence should be implemented.
I was also thinking that should jruby-rack allow rails to initialize rails default logger somehow when no logger is specified. If I have understood correctly, jruby-rack now sets some jruby-rack specific default logger as rails logger if no other is set. Could this jruby-rack spesific default logger not be the default and let rails set the default like it does when running in e.g. puma? Does this make any sense?
Yeah, the project I inherited has some kind of silence related hack as well as a custom slf4j logger. Been meaning to dig into that during the 1.2.x upgrade (since there are.logging changes there too). Currently still stuck on both jruby-rack 1.1 and rails 6.1 due to needing some fixes from here first.
But we're close 👍
Could it be an idea to add a configuration option, like config.x.jruby.use_rails_default_logger = true? The goal would be to let Rails handle the default logger setup instead of jruby-rack setting its own when not explicitly configured.
The reason for this is that I don't like to set the logger to the default one explicitly. I'm not entirely sure how to do that, and it might change when the Rails version changes.
From my perspective that could work, but I am no expert 😅
Sadly we still seem to be a bit short on maintainer time here as my PR #271 is stuck (which fixes Rails 7 and reinstates some of the appraisals to sanity check with various Rails versions).
I am maintaining a fork temporarily (Maven/jar distribution only, not rubygems) at https://github.com/gocd-contrib/jruby-rack if you wanted to give your options a go alongside the appraisals.
MR is incomplete, someone just needs to figure out the best based way forward (based on supported Rails versions and the Rails.logger APIs) and finish it up.
Sadly we still seem to be a bit short on maintainer time here as my PR https://github.com/jruby/jruby-rack/pull/271 is stuck (which fixes Rails 7 and reinstates some of the appraisals to sanity check with various Rails versions).
Merged in, I do not see any issues with having that in 1.2.x mostly just benefits. Thanks for working on that.
was thinking about this a bit more, while cleaning up the railtie.
not sure about the approach here, maybe there's a performance consideration, given the original logger gets cloned and extended (upon ActiveSupport::TaggedLogging.new(logger)) we could just do the same (will give it a go to get feedback).