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

Include logger silence from active support #270

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

Conversation

jlahtinen
Copy link
Contributor

@jlahtinen jlahtinen commented Jan 15, 2025

Suggested way to make logger work more like rails logged does in puma.

@chadlwilson
Copy link
Contributor

Hi @jlahtinen - slightly unrelated to this, but which rack version are you using jruby-rack with?

@jlahtinen
Copy link
Contributor Author

Hi @jlahtinen - slightly unrelated to this, but which rack version are you using jruby-rack with?

rack (2.2.9)

@chadlwilson
Copy link
Contributor

chadlwilson commented Jan 22, 2025

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)

@jlahtinen
Copy link
Contributor Author

jlahtinen commented Jan 22, 2025

@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

@chadlwilson
Copy link
Contributor

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).

class RailsLogger < JRuby::Rack::Logger
include ActiveSupport::LoggerSilence

def info(*args)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why is the info level overriden here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't remember anymore. And now it looks like it should not override it.

Unfortunately I do not have time to test this right now. Maybe later today or tomorrow

@chadlwilson
Copy link
Contributor

chadlwilson commented Jan 22, 2025

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.

@jlahtinen
Copy link
Contributor Author

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?

@chadlwilson
Copy link
Contributor

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 👍

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.

3 participants