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

beyond JRuby's StandardErrorLogger #3469

Merged
merged 21 commits into from
Apr 25, 2016
Merged

beyond JRuby's StandardErrorLogger #3469

merged 21 commits into from
Apr 25, 2016

Conversation

kares
Copy link
Member

@kares kares commented Nov 18, 2015

improvements and some new features for JRuby's internal logging
there seems to be a lot of commits but usually with little change - did not want to squash them as some of them have detailed messages.

... gist of changes :

  • -Xlog.backtraces can get hard to read with multiple threads ... targeted for jruby-1_7 log exception backtrace in one log record #3403 as well
  • -Xlog.callers is double logged and hard to read with a to_s array formatting - this is changed to use MRI backtrace formatting
  • some Java and even JRuby libraries (e.g. daemonization) change System.err which causes the StandardErrorLogger to print "nowhere" if JRuby's classes are loaded before the stream change - we'll know allways read the System.err field
  • "improved" default (StandardErrorLogger) logger format: 2015-11-04T11:29:41.759+01:00 [main] INFO SampleLogger : hello world ... includes thread-name and level compared to current state
  • possibility to use a delegating logger - with JUL and SLF4J impls provided. this will be quite useful in embed-ing scenarios. due this a LoggerFactory.getLogger(Class) is introduced and should be preferred of the (String) version so that one could potentially configure loggers easily e.g. for the whole org.jruby.xxx package. note that default logger still behave the same and only uses simple name of the class as the logger name

@kares kares added this to the JRuby 9.1.0.0 milestone Dec 17, 2015
@kares kares force-pushed the jruby-logger-ng branch 2 times, most recently from 83f68c1 to f180fbd Compare February 18, 2016 10:58
@kares
Copy link
Member Author

kares commented Feb 23, 2016

would it be OK to have these in for 9.1 // cc @enebo @headius ?
fully understand logging is mostly intended "internal" as is but still some of it is already useful and other parts might be tuned as needed to be used in a "traditional" Java logging setup.

@enebo
Copy link
Member

enebo commented Feb 23, 2016

So one comment is that the use of debug("foo {}", argument) should be a net
win for us performance wise versus our use of string concat when debugging
is disabled.

I was also worried when I saw some static final DEBUGs disappear but I see
no particular harm in any individual case (over time we have ended up with
expensive parameters to log statements kill performance even with debugging
off. The only potential downside of some removed static final DEBUG checks
is increased codesize of the method since the debug call will now need to
get inlined out of existence by JVM.

@kares my main question is how could us including slf4j end up affecting
existing users? If they are already using slf4j would this end up
potentially break their application? If that is the case a second question
would be whether we can shade this or not?

-Tom

On Mon, Feb 22, 2016 at 10:58 PM, Karol Bucek notifications@github.com
wrote:

would it be OK to have these in for 9.1 // cc @enebo
https://github.com/enebo @headius https://github.com/headius ?
fully understand logging is mostly intended "internal" as is but still
some of it is already useful and other parts might be tuned as needed to be
used in a "traditional" Java logging setup.


Reply to this email directly or view it on GitHub
#3469 (comment).

blog: http://blog.enebo.com twitter: tom_enebo
mail: tom.enebo@gmail.com

@kares
Copy link
Member Author

kares commented Feb 23, 2016

@enebo the old way of static final checks can stay here and there - removed some that are useful in embed (e.g. while running with jruby-rack its annoying to always have to look for System.out / err)

we do not include SLF4J ... its an optional dependency and would need to be manually added, a SLF4J logger impl (SLF4J only required to have it compiled) is provided but anyone willing to use it must add and setup SLF4J on their own and use the 'new' -Djruby.logger.class=org.jruby.log.SLF4JLogger

@enebo
Copy link
Member

enebo commented Feb 23, 2016

@kares thanks for the explanation. This all looked fine with what I saw.
@headius may want to take a gander due to it size. Another set of eyes
will not hurt...

On Tue, Feb 23, 2016 at 9:13 AM, Karol Bucek notifications@github.com
wrote:

@enebo https://github.com/enebo the old way of static final checks can
stay here and there - removed some that are useful in embed (e.g. while
running with jruby-rack its annoying to always have to look for System.out
/ err)

we do not include SLF4J ... its an optional dependency and would need to
be manually added, a SLF4J logger impl (SLF4J only required to have it
compiled) is provided but anyone willing to use it must add and setup SLF4J
on their own and use the 'new'
-Djruby.logger.class=org.jruby.log.SLF4JLogger


Reply to this email directly or view it on GitHub
#3469 (comment).

blog: http://blog.enebo.com twitter: tom_enebo
mail: tom.enebo@gmail.com

@kares
Copy link
Member Author

kares commented Feb 24, 2016

@enebo thanks for the review, will wait for another 🚦 - as I do not want to force this on you unless you are comfortable with the change (tried to make sure it works out-of-the box the same way as so far).

@kares
Copy link
Member Author

kares commented Mar 1, 2016

@headius how's this looking for you? (obviously will need a rebase of master)

@kares kares force-pushed the jruby-logger-ng branch from f180fbd to 7102a3f Compare March 14, 2016 11:59
@kares
Copy link
Member Author

kares commented Mar 17, 2016

@headius 🎱 still want this reviewed ... 💭 please

@kares kares force-pushed the jruby-logger-ng branch 3 times, most recently from 8d82307 to 6e5b671 Compare March 21, 2016 17:27
@kares
Copy link
Member Author

kares commented Apr 1, 2016

release is getting close ... should we still wait for @headius to get us a review ?

@enebo
Copy link
Member

enebo commented Apr 1, 2016

@kares I believe @headius will not be online much until monday. We will land then I guess.

kares added 11 commits April 19, 2016 15:23
* added a OutputStreamLogger base-class
* improved {} parameter substitution performance (and failure reporting)
* message+throwable writes are synchronized to make sure they stay together
… on Ruby land

... e.g. with "daemonizing" libraries that also replace Java's System.err but after JRuby
has been loaded
…ssage e.g. :

2015-11-04T11:29:41.759+01:00 [main] INFO SampleLogger : hello world
…skipped

this is for delegated logging such as with SLF4J where we do not want to pollute the
"default" logger name space + it's also easier to set logging verbosity for org.jruby.xxx
…ing a class down

... default std-err logging will still behave the same - using only the class simple name
but delegated loggers such as SLF4J will have the desired package parent structure
@kares kares force-pushed the jruby-logger-ng branch from 6e1a5cd to 74f0d4f Compare April 19, 2016 13:23
@kares kares merged commit 74f0d4f into master Apr 25, 2016
@kares
Copy link
Member Author

kares commented Apr 25, 2016

merged in to have -Xlog.callers and backtraces consistent and usable in 9.1

@kares kares deleted the jruby-logger-ng branch September 7, 2016 04:43
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

2 participants