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

Conversation

Projects
None yet
2 participants
@kares
Member

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 #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

This comment has been minimized.

Show comment
Hide comment
@kares

kares Feb 23, 2016

Member

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.

Member

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

This comment has been minimized.

Show comment
Hide comment
@enebo

enebo Feb 23, 2016

Member

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

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

This comment has been minimized.

Show comment
Hide comment
@kares

kares Feb 23, 2016

Member

@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

Member

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

This comment has been minimized.

Show comment
Hide comment
@enebo

enebo Feb 23, 2016

Member

@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

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

This comment has been minimized.

Show comment
Hide comment
@kares

kares Feb 24, 2016

Member

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

Member

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

This comment has been minimized.

Show comment
Hide comment
@kares

kares Mar 1, 2016

Member

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

Member

kares commented Mar 1, 2016

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

@kares

This comment has been minimized.

Show comment
Hide comment
@kares

kares Mar 17, 2016

Member

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

Member

kares commented Mar 17, 2016

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

@kares

This comment has been minimized.

Show comment
Hide comment
@kares

kares Apr 1, 2016

Member

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

Member

kares commented Apr 1, 2016

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

@enebo

This comment has been minimized.

Show comment
Hide comment
@enebo

enebo Apr 1, 2016

Member

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

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 some commits Nov 4, 2015

reinvent logger impl behind JRuby's default StandardErrorLogger
* added a OutputStreamLogger base-class
* improved {} parameter substitution performance (and failure reporting)
* message+throwable writes are synchronized to make sure they stay together
make sure redirected System.err is picked up - not uncommon to happen…
… on Ruby land

... e.g. with "daemonizing" libraries that also replace Java's System.err but after JRuby
has been loaded
include current thread-name as well as level in logger's formatted me…
…ssage e.g. :

2015-11-04T11:29:41.759+01:00 [main] INFO SampleLogger : hello world
support getLogger with a Class parameter - where package name is not …
…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
refactor to using the new preferred way of getting a logger - by pass…
…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
use debug level to log + setDebugEnable on JIT_LOADING_DEBUG
this way logging will work as expected with delegated logging such as SLF4J as well

@kares kares merged commit 74f0d4f into master Apr 25, 2016

0 of 3 checks passed

continuous-integration/appveyor/branch AppVeyor build failed
Details
continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build failed
Details
@kares

This comment has been minimized.

Show comment
Hide comment
@kares

kares Apr 25, 2016

Member

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

Member

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 Sep 7, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment