Velocity
  1. Velocity
  2. VELOCITY-403

Enhance Velocity's LogSystem and internal use thereof

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 1.5
    • Component/s: Engine
    • Labels:
      None

      Description

      After several very, very long debates with Geir over commons-logging, i have become largely convinced that it is something we should do with hesitation (i.e. let's talk about it for 2.0, but not before). i've also begun to have some frustrations with commons-logging in both work projects and in VelocityTools.

      regardless of these issues, it is clear to me that Velocity's LogSystem and use of it is in great need of improvement. we need to lower the priority of many messages, eliminate some, and above all, upgrade the system itself to be more useful.

      some specific, unevaluated, off-the-cuff ideas are:
      -make logging a null-op if no logger is found, rather than panic and break
      -detect jdk 1.4+ logging
      -add a trace level
      -add is<Level>Enabled
      -make it possible to grab a LogSystem instance of sorts, so Velocity extensions can use it more sensibly (i'm tired of the hacks we must do in Tools)

      those are just a few ideas, and they might not all even be feasible. still, i'm hoping to take a whack at some of this and hoping that others can help me out. my volunteer time is still rather limited.

      1. second.logging.patch
        172 kB
        Nathan Bubna
      2. logging.patch
        136 kB
        Nathan Bubna
      3. docs.logging.patch
        10 kB
        Nathan Bubna

        Activity

        Hide
        Will Glass-Husain added a comment -

        Sounds like a good plan.

        Another item... When an app runs Velocity without configuring a log file, it puts a file "velocity.log" in the current directory. I think this is a terrible idea. For a web app, the "current directory" is wherever the shell user was when he started Tomcat. One of my developers used Velocity in a webapp last year to send mail. I now have "velocity.log" files scattered all over my server. Suggestion. No log file configured - send to standard out or not at all.

        Show
        Will Glass-Husain added a comment - Sounds like a good plan. Another item... When an app runs Velocity without configuring a log file, it puts a file "velocity.log" in the current directory. I think this is a terrible idea. For a web app, the "current directory" is wherever the shell user was when he started Tomcat. One of my developers used Velocity in a webapp last year to send mail. I now have "velocity.log" files scattered all over my server. Suggestion. No log file configured - send to standard out or not at all.
        Hide
        Henning Schmiedehausen added a comment -

        I do know that Geir is very hesitant to consider commons-logging. I can follow his reasoning to some point. However, if we start to rebuild the whole mess with logging detection, JDK 14, log4j, logkit switch and so on, we will end up with a commons-logging Frankenstein inside the velocity jar.

        Which I'd really want to avoid. For Velocity 1.x, we're pretty much stuck with what we have. For 2.x, I'd love to see that we at least consider whether c-l would be an option. We over there at turbine land never ever regretted the switch from the old, 2.1 self rolled logging system which was only usable in the default configuration and you couldn't touch it to "commons-logging everywhere" with a strong leaning towards log4j.

        The whole discussion about log improving leads that we either use some sort of logging bridge (and unfortunately Ceki has chosen to play ideology instead of pragmatism with his "UGLI" attempt (which seems to went AWOL these days)) or just commit to a single logging system (which probably would be log4j these days).

        Show
        Henning Schmiedehausen added a comment - I do know that Geir is very hesitant to consider commons-logging. I can follow his reasoning to some point. However, if we start to rebuild the whole mess with logging detection, JDK 14, log4j, logkit switch and so on, we will end up with a commons-logging Frankenstein inside the velocity jar. Which I'd really want to avoid. For Velocity 1.x, we're pretty much stuck with what we have. For 2.x, I'd love to see that we at least consider whether c-l would be an option. We over there at turbine land never ever regretted the switch from the old, 2.1 self rolled logging system which was only usable in the default configuration and you couldn't touch it to "commons-logging everywhere" with a strong leaning towards log4j. The whole discussion about log improving leads that we either use some sort of logging bridge (and unfortunately Ceki has chosen to play ideology instead of pragmatism with his "UGLI" attempt (which seems to went AWOL these days)) or just commit to a single logging system (which probably would be log4j these days).
        Hide
        Daniel Rall added a comment -

        Logging should absolutely not be a no-op under any circumstances.

        Show
        Daniel Rall added a comment - Logging should absolutely not be a no-op under any circumstances.
        Hide
        Will Glass-Husain added a comment -

        Daniel - makes sense. Alternate suggestion. If logging is not configured, send to standard out instead of creating "velocity.log" in an arbitrary place.

        Show
        Will Glass-Husain added a comment - Daniel - makes sense. Alternate suggestion. If logging is not configured, send to standard out instead of creating "velocity.log" in an arbitrary place.
        Hide
        Nathan Bubna added a comment -

        Ok, i've actually put a little time in on a patch for this. here's a progress update:

        1) agreed on sending logging to standard out if no logging is configured. i'll make a StandardOutLogSystem to go with this patch.

        2) to do much of this, we must add methods to the LogSystem interface. this will break older external LogSystem implementations. we have a few options:
        a) just do it and make people upgrade since the new methods are simple and quick to implement
        b) have a new log interface that extends LogSystem and adds the new methods

        • what would we name it?
        • do we then deprecate LogSystem? or wait until 2.0 to reunify the logging?
          c) abandon the improvements

        thoughts? preferences? IMHO's?

        Show
        Nathan Bubna added a comment - Ok, i've actually put a little time in on a patch for this. here's a progress update: 1) agreed on sending logging to standard out if no logging is configured. i'll make a StandardOutLogSystem to go with this patch. 2) to do much of this, we must add methods to the LogSystem interface. this will break older external LogSystem implementations. we have a few options: a) just do it and make people upgrade since the new methods are simple and quick to implement b) have a new log interface that extends LogSystem and adds the new methods what would we name it? do we then deprecate LogSystem? or wait until 2.0 to reunify the logging? c) abandon the improvements thoughts? preferences? IMHO's?
        Hide
        Nathan Bubna added a comment -

        Ok, I think I've got this all ready to go. It's a big freakin' patch, but that's mostly because logging is frequently used. The rather hefty summary of changes is below. Please let me know if you see any problems. After this comment, i'll attach the patch (which, of course, compiles and passes all the tests).

        Changes in this patch:
        Upgraded LogSystem:

        • added isLevelEnabled(int level)
        • added logVelocityMessage(int level, String message, Throwable t)
        • added TRACE_ID constant
        • deprecated unused DEBUG_ON constant

        Upgraded existing LogSystem implementations:

        • updated Log4JLogSystem with the new methods
        • updated PrimordialLogSystem with the new methods
        • updated NullLogSystem with the new methods
        • updated AvalonLogSystem with the new methods
        • updated SimpleLog4JLogSystem with the new methods
        • updated testcases that implement LogSystem with the new methods
        • ClassloaderChangeTestCase
        • EventHandlingTestCase
        • ExternalLoggerTestCase
        • FilteredEventHandlingTestCase

        Added new LogSystem implementations:

        • JdkLogSystem for the java.util.logging services in JDK 1.4+
        • StandardOutLogSystem for printing logging to System.out and System.err

        Added new Log class:

        • Log class is meant to replace RuntimeLogger interface
        • also added temporary RuntimeLoggerLog hack as deprecation path
        • Log class provides more convenience methods for LogSystem functions
        • Log class takes over some logging code from RuntimeInstance
        • Log instance may be retrieved from RuntimeServices.getLog()

        Upgraded LogManager:

        • createLogSystem(...) now falls back to StandardOutLogSystem on failure
        • added updateLog(...) to take over Log initialization code from RuntimeInstance

        Updated RuntimeInstance:

        • moved logging init code to LogManager and Log
        • deprecated debug(Object), info(Object), warn(Object), and error(Object)
        • added getLog()
        • removed private methods showStackTraces() and log(...)
        • replaced private LogSystem instance with Log instance
        • replaced initializeLogger() with simpler initializeLog() code
        • passed Log to new Introspector instead of self

        Updated RuntimeServices interface:

        • added getLog() method

        Updated default velocity.properties:

        • added JdkLogSystem as last option of runtime.log.logsystem.class property

        Updated UberspectLoggable interface:

        • deprecated setRuntimeLogger(RuntimeLogger)
        • added setLog(Log)
        • updated UberspectImpl to reflect these changes
        • all uberspects on the wiki extend UberspectImpl so these changes won't break them
        • this is easy for other uberspects to impl; Uberspect writers are very capable

        Updated classes that directly use RuntimeLogger (*Executors and Introspector):

        • deprecated ctors that take RuntimeLogger
        • added ctors that take Log
        • deprecated ctors forward to new ctor using RuntimeLoggerLog hack
        • replaced RuntimeLogger refs with Log refs

        Updated internal classes to use new logging API:

        • replace calls to RuntimeServices.debug/info/warn/error with RuntimeService.getLog()...
        • if log messages included an Exception, pass that separately now (i.e. error("Oops!", ex)
        • logging was frequently used in a class/subclasses, add a Log reference
        • if the classes uses few log statements, just s/rsvc.<level>/rsvc.getLog().<level>
        • classes affected are:
          Template, VelocimacroFactory, Parser, SimpleNode, ContentResource,
          ResourceLoader, VelocimacroManager, VelocimacroProxy, ASTStringLiteral,
          ASTIdentifier, ASTMethod, ASTReference, ASTSetDirective, ASTAndNode,
          ASTEQNode, ASTNENode, ASTLTNode, ASTGTNode, ASTLENode, ASTGENode,
          ASTAddNode, ASTSubtractNode, ASTMulNode, ASTDivNode, ASTModNode,
          EventHandlerUtil, EscapeReference, IncludeNotFound, VMProxyArg, Foreach,
          Include, Macro, Parse, ASTIntegerRange, ClasspathResourceLoader,
          DataSourceResourceLoader, RuntimeInstance, ResourceCacheImpl,
          ResourceManagerImpl, ResourceLoader, FileResourceLoader, JarHolder,
          JarResourceLoader, ASTIntegerRange, ResourceLoaderFactory
        Show
        Nathan Bubna added a comment - Ok, I think I've got this all ready to go. It's a big freakin' patch, but that's mostly because logging is frequently used. The rather hefty summary of changes is below. Please let me know if you see any problems. After this comment, i'll attach the patch (which, of course, compiles and passes all the tests). Changes in this patch: Upgraded LogSystem: added isLevelEnabled(int level) added logVelocityMessage(int level, String message, Throwable t) added TRACE_ID constant deprecated unused DEBUG_ON constant Upgraded existing LogSystem implementations: updated Log4JLogSystem with the new methods updated PrimordialLogSystem with the new methods updated NullLogSystem with the new methods updated AvalonLogSystem with the new methods updated SimpleLog4JLogSystem with the new methods updated testcases that implement LogSystem with the new methods ClassloaderChangeTestCase EventHandlingTestCase ExternalLoggerTestCase FilteredEventHandlingTestCase Added new LogSystem implementations: JdkLogSystem for the java.util.logging services in JDK 1.4+ StandardOutLogSystem for printing logging to System.out and System.err Added new Log class: Log class is meant to replace RuntimeLogger interface also added temporary RuntimeLoggerLog hack as deprecation path Log class provides more convenience methods for LogSystem functions Log class takes over some logging code from RuntimeInstance Log instance may be retrieved from RuntimeServices.getLog() Upgraded LogManager: createLogSystem(...) now falls back to StandardOutLogSystem on failure added updateLog(...) to take over Log initialization code from RuntimeInstance Updated RuntimeInstance: moved logging init code to LogManager and Log deprecated debug(Object), info(Object), warn(Object), and error(Object) added getLog() removed private methods showStackTraces() and log(...) replaced private LogSystem instance with Log instance replaced initializeLogger() with simpler initializeLog() code passed Log to new Introspector instead of self Updated RuntimeServices interface: added getLog() method Updated default velocity.properties: added JdkLogSystem as last option of runtime.log.logsystem.class property Updated UberspectLoggable interface: deprecated setRuntimeLogger(RuntimeLogger) added setLog(Log) updated UberspectImpl to reflect these changes all uberspects on the wiki extend UberspectImpl so these changes won't break them this is easy for other uberspects to impl; Uberspect writers are very capable Updated classes that directly use RuntimeLogger (*Executors and Introspector): deprecated ctors that take RuntimeLogger added ctors that take Log deprecated ctors forward to new ctor using RuntimeLoggerLog hack replaced RuntimeLogger refs with Log refs Updated internal classes to use new logging API: replace calls to RuntimeServices.debug/info/warn/error with RuntimeService.getLog()... if log messages included an Exception, pass that separately now (i.e. error("Oops!", ex) logging was frequently used in a class/subclasses, add a Log reference if the classes uses few log statements, just s/rsvc.<level>/rsvc.getLog().<level> classes affected are: Template, VelocimacroFactory, Parser, SimpleNode, ContentResource, ResourceLoader, VelocimacroManager, VelocimacroProxy, ASTStringLiteral, ASTIdentifier, ASTMethod, ASTReference, ASTSetDirective, ASTAndNode, ASTEQNode, ASTNENode, ASTLTNode, ASTGTNode, ASTLENode, ASTGENode, ASTAddNode, ASTSubtractNode, ASTMulNode, ASTDivNode, ASTModNode, EventHandlerUtil, EscapeReference, IncludeNotFound, VMProxyArg, Foreach, Include, Macro, Parse, ASTIntegerRange, ClasspathResourceLoader, DataSourceResourceLoader, RuntimeInstance, ResourceCacheImpl, ResourceManagerImpl, ResourceLoader, FileResourceLoader, JarHolder, JarResourceLoader, ASTIntegerRange, ResourceLoaderFactory
        Hide
        Nathan Bubna added a comment -

        Oh, and i almost forgot, this patch now requires log4j-1.2.12 to compile (but it should run with any log4j version).

        Show
        Nathan Bubna added a comment - Oh, and i almost forgot, this patch now requires log4j-1.2.12 to compile (but it should run with any log4j version).
        Hide
        Nathan Bubna added a comment -

        At Tim Colson's wise behest, i'm adding these comments (previously sent to the dev-list):

        despite the scope of the changes, most apps currently using Velocity probably won't notice any difference, except perhaps the addition and use of StandardOutLogSystem and JdkLogSystem.

        any developers who have implemented their own LogSystem's will have to add two simple methods to their implementations if they decide upgrade to 1.5. this is likely to be uncommon, very easy to do, and won't make their library incompatible with earlier Velocity versions. VelocityTools falls into this category.

        also, any developers who implement their own Uberspect, don't extend UberspectImpl, and implement UberspectLoggable (read: no one) will have to implement one new method in the UberspectLoggable interface, which again, won't make them incompatible with earlier versions of Velocity.

        in other words, the changes are backwards compatible, and the deprecation path is clearly set. there's no reason to wait for 2.0 before adding these enhancements.

        Show
        Nathan Bubna added a comment - At Tim Colson's wise behest, i'm adding these comments (previously sent to the dev-list): despite the scope of the changes, most apps currently using Velocity probably won't notice any difference, except perhaps the addition and use of StandardOutLogSystem and JdkLogSystem. any developers who have implemented their own LogSystem's will have to add two simple methods to their implementations if they decide upgrade to 1.5. this is likely to be uncommon, very easy to do, and won't make their library incompatible with earlier Velocity versions. VelocityTools falls into this category. also, any developers who implement their own Uberspect, don't extend UberspectImpl, and implement UberspectLoggable (read: no one) will have to implement one new method in the UberspectLoggable interface, which again, won't make them incompatible with earlier versions of Velocity. in other words, the changes are backwards compatible, and the deprecation path is clearly set. there's no reason to wait for 2.0 before adding these enhancements.
        Hide
        Will Glass-Husain added a comment -

        Wow, that's quite the patch. Cleans up a lot of annoying issues. Definitely for inclusion in 1.5. Thanks too for the detailed description - very useful.

        Two comments. The first is minor. Introducing JDKLogSystem is our first JDK 1.4 dependency. I'd like to leave this out (maybe put it in the Wiki).

        The second is more significant. Changing LogSystem means that this is not a drop-in replacement for existing installs. For example, my main Velocity-based app implements LogSystem. It's superfluous (I should probably change this to use one of the built-ins) but I don't like making users change
        APIs simply to upgrade a library.

        I suggest we define LogInterface (or similar name) that extends LogSystem. Have all internal uses of the LogSystem change to LogInterface. Recommend in the docs that LogInterface be used. When the user configures a logger, check to see if it is LogSystem or LogInterface. if it is LogSystem then wrap it in an adapter LogSystemToInterface so that it has the full API for internal use.

        Comments?

        Show
        Will Glass-Husain added a comment - Wow, that's quite the patch. Cleans up a lot of annoying issues. Definitely for inclusion in 1.5. Thanks too for the detailed description - very useful. Two comments. The first is minor. Introducing JDKLogSystem is our first JDK 1.4 dependency. I'd like to leave this out (maybe put it in the Wiki). The second is more significant. Changing LogSystem means that this is not a drop-in replacement for existing installs. For example, my main Velocity-based app implements LogSystem. It's superfluous (I should probably change this to use one of the built-ins) but I don't like making users change APIs simply to upgrade a library. I suggest we define LogInterface (or similar name) that extends LogSystem. Have all internal uses of the LogSystem change to LogInterface. Recommend in the docs that LogInterface be used. When the user configures a logger, check to see if it is LogSystem or LogInterface. if it is LogSystem then wrap it in an adapter LogSystemToInterface so that it has the full API for internal use. Comments?
        Hide
        Henning Schmiedehausen added a comment -

        (Nathan, are you able to "take" this issue (i.e. assign it to you?))

        My comment on this is twofold:

        We do need improvement of the logging for 1.5. So I'm cautiously +1 on applying this patch for 1.5

        However, this patch IMHO demonstrates exactly the problem that I do have with Velocity logging: It is already a package of its own. And it is rapidly becoming a commons-logging Frankenstein.

        I know that there is a big reluctance to actually switch to c-l. Such a switch is not 1.x material, it would be 2.x material. But in the light of patches like this one, we should really consider it.

        And for getting another external dependency: Velocity IMHO is not just "another library". If it were, you could get away with throwing exceptions for problems and some debug logging to System.out/System.err. Velocity is a component to build applications. And this is exactly the kind of problem space, that c-l was created for.

        (This would also BTW solve the problem of tools getting to log correctly).

        Show
        Henning Schmiedehausen added a comment - (Nathan, are you able to "take" this issue (i.e. assign it to you?)) My comment on this is twofold: We do need improvement of the logging for 1.5. So I'm cautiously +1 on applying this patch for 1.5 However, this patch IMHO demonstrates exactly the problem that I do have with Velocity logging: It is already a package of its own. And it is rapidly becoming a commons-logging Frankenstein. I know that there is a big reluctance to actually switch to c-l. Such a switch is not 1.x material, it would be 2.x material. But in the light of patches like this one, we should really consider it. And for getting another external dependency: Velocity IMHO is not just "another library". If it were, you could get away with throwing exceptions for problems and some debug logging to System.out/System.err. Velocity is a component to build applications. And this is exactly the kind of problem space, that c-l was created for. (This would also BTW solve the problem of tools getting to log correctly).
        Hide
        Nathan Bubna added a comment -

        Regarding Java 1.4:
        it need only be a compile time dependency. If a user is running 1.3, then it will fail just as if the user didn't have LogKit or Log4j in the classpath. I think that is totally legitimate. Active developers should have 1.4 or 1.5 on their system already. This should be no burden to anyone compiling velocity themselves, and it may be a great benefit to many users who upgrade. i pretty strenously object to leaving this out.

        Regarding the change to LogSystem:
        it's two simple methods. anyone competent enough to implement their own LogSystem can very easily add these in a way that's BC for their app. most importantly, the LogSystem interface is really a developer extension point, far more than a standard user API. i would oppose changing user API w/o a deprecation path, but developers should be far more on top of things. i really don't think it's worth the LogInterface hack for those precious few, highly capable developers' convenience. but, if this will block the commit, i'll look into it more.

        Regarding c-l before 2.0:
        Not gonna happen. We've been over this multiple times. Please don't go there right now. Save it for later. I'll deal with the problem of Tools+Tomcat 5.5 after this patch goes thru.

        Show
        Nathan Bubna added a comment - Regarding Java 1.4: it need only be a compile time dependency. If a user is running 1.3, then it will fail just as if the user didn't have LogKit or Log4j in the classpath. I think that is totally legitimate. Active developers should have 1.4 or 1.5 on their system already. This should be no burden to anyone compiling velocity themselves, and it may be a great benefit to many users who upgrade. i pretty strenously object to leaving this out. Regarding the change to LogSystem: it's two simple methods. anyone competent enough to implement their own LogSystem can very easily add these in a way that's BC for their app. most importantly, the LogSystem interface is really a developer extension point, far more than a standard user API. i would oppose changing user API w/o a deprecation path, but developers should be far more on top of things. i really don't think it's worth the LogInterface hack for those precious few, highly capable developers' convenience. but, if this will block the commit, i'll look into it more. Regarding c-l before 2.0: Not gonna happen. We've been over this multiple times. Please don't go there right now. Save it for later. I'll deal with the problem of Tools+Tomcat 5.5 after this patch goes thru.
        Hide
        Nathan Bubna added a comment -

        FYI, i've decided i can't spend any more time on these things after today. i have paid-work i must do. so, i'm making the change Will requested (but calling the new interface LogChute instead of LogInterface ). It's not as simple as it sounds, because i don't think it's ok to keep FooLogSystem classes around that now impl LogChute instead of LogSystem, but we can't arbitrarily remove those classes either as they are well-used. so i am deprecating those classes, making them nearly-empty subclasses of their new FooLogChute replacements that implement the old and deprecated LogSystem interface.

        i'll attach the new patch (which now also includes fixes for other velocity logging bugs) and let you guys decide and commit whichever you prefer. then i'll be moving on to other things.

        Show
        Nathan Bubna added a comment - FYI, i've decided i can't spend any more time on these things after today. i have paid-work i must do. so, i'm making the change Will requested (but calling the new interface LogChute instead of LogInterface ). It's not as simple as it sounds, because i don't think it's ok to keep FooLogSystem classes around that now impl LogChute instead of LogSystem, but we can't arbitrarily remove those classes either as they are well-used. so i am deprecating those classes, making them nearly-empty subclasses of their new FooLogChute replacements that implement the old and deprecated LogSystem interface. i'll attach the new patch (which now also includes fixes for other velocity logging bugs) and let you guys decide and commit whichever you prefer. then i'll be moving on to other things.
        Hide
        Nathan Bubna added a comment -

        Ok, this second patch should now be 100% backwards compatible.

        Show
        Nathan Bubna added a comment - Ok, this second patch should now be 100% backwards compatible.
        Hide
        Nathan Bubna added a comment -

        er... i meant, it should be 100% backwards compatible with custom LogSystem implementations.

        there still remains slight incovenience to the theoretical (and almost certainly non-existent) developer who has implemented UberspectLoggable w/o extending UberspectImpl.

        hmm, and i suppose anyone who used LogManager.createLogSystem(...) is now mildly inconvenienced, but since that is an internal API, not even a pluggable extension point, much less a user API, i don't care.

        Show
        Nathan Bubna added a comment - er... i meant, it should be 100% backwards compatible with custom LogSystem implementations. there still remains slight incovenience to the theoretical (and almost certainly non-existent) developer who has implemented UberspectLoggable w/o extending UberspectImpl. hmm, and i suppose anyone who used LogManager.createLogSystem(...) is now mildly inconvenienced, but since that is an internal API, not even a pluggable extension point, much less a user API, i don't care.
        Hide
        Will Glass-Husain added a comment -

        Great. Let's discuss the JDK 1.4 compile requirement on the list. I'm positive about it, but it needs a poll.

        I'm not worred about the UberspectLoggable issue.

        Nathan - thanks for your time with this comprehensive patch. I'll be glad to run with this. (probably not for a few days - gotta pay my bills too .

        Show
        Will Glass-Husain added a comment - Great. Let's discuss the JDK 1.4 compile requirement on the list. I'm positive about it, but it needs a poll. I'm not worred about the UberspectLoggable issue. Nathan - thanks for your time with this comprehensive patch. I'll be glad to run with this. (probably not for a few days - gotta pay my bills too .
        Hide
        Will Glass-Husain added a comment -

        Applied patch. Great job!

        Are there any documentation issues to address before we resolve this issue?

        Show
        Will Glass-Husain added a comment - Applied patch. Great job! Are there any documentation issues to address before we resolve this issue?
        Hide
        Nathan Bubna added a comment -

        Good question. If i get some time later this weekend, i'll grep around the docs for anything about the old LogSystem interface. there's probably not much-if anything-but it'd be good to look.

        thanks for applying the patch!

        Show
        Nathan Bubna added a comment - Good question. If i get some time later this weekend, i'll grep around the docs for anything about the old LogSystem interface. there's probably not much- if anything -but it'd be good to look. thanks for applying the patch!
        Hide
        Nathan Bubna added a comment -

        Ok. here's a patch for the developer's guide and the logging examples.

        Show
        Nathan Bubna added a comment - Ok. here's a patch for the developer's guide and the logging examples.
        Hide
        Nathan Bubna added a comment -

        while patching the docs, i noticed that in the Velocity Configuration Keys and Values section of the dev guide (http://jakarta.apache.org/velocity/docs/developer-guide.html#Velocity%20Configuration%20Keys%20and%20Values), there is the following text:

        Show
        Nathan Bubna added a comment - while patching the docs, i noticed that in the Velocity Configuration Keys and Values section of the dev guide ( http://jakarta.apache.org/velocity/docs/developer-guide.html#Velocity%20Configuration%20Keys%20and%20Values ), there is the following text:
        Hide
        Nathan Bubna added a comment -

        while patching the dev guide i noticed the following in the Configuration Keys and Values section:

        runtime.log.error.stacktrace = false
        runtime.log.warn.stacktrace = false
        runtime.log.info.stacktrace = false
        Turns on stacktracing for the three error categories. These produce a large amount of log output.

        sure enough, there are constants to match these properties in the RuntimeConstants interface, but these are largely unused. the error and info constants are referenced only in the TemplateTestSuite, and the warn constant is curiously used to trigger the showStackTraces junk that i blindly retained/mimicked in my patch (moving it from RuntimeInstance to LogManager and Log).

        i say we axe these from the documentation, deprecate them in RuntimeConstants, and if anyone is still interested in the showStackTraces stuff, create and use a single property for that rather than weirdly use the RUNTIME_LOG_WARN_STACKTRACE constant to govern all of it.

        Show
        Nathan Bubna added a comment - while patching the dev guide i noticed the following in the Configuration Keys and Values section: runtime.log.error.stacktrace = false runtime.log.warn.stacktrace = false runtime.log.info.stacktrace = false Turns on stacktracing for the three error categories. These produce a large amount of log output. sure enough, there are constants to match these properties in the RuntimeConstants interface, but these are largely unused. the error and info constants are referenced only in the TemplateTestSuite, and the warn constant is curiously used to trigger the showStackTraces junk that i blindly retained/mimicked in my patch (moving it from RuntimeInstance to LogManager and Log). i say we axe these from the documentation, deprecate them in RuntimeConstants, and if anyone is still interested in the showStackTraces stuff, create and use a single property for that rather than weirdly use the RUNTIME_LOG_WARN_STACKTRACE constant to govern all of it.
        Hide
        Will Glass-Husain added a comment -

        makes sense to me.

        Show
        Will Glass-Husain added a comment - makes sense to me.
        Hide
        Will Glass-Husain added a comment -

        docs.logging.patch applied. Leaving this open. The stack trace issue is still unresolved and there might be other comments.

        Show
        Will Glass-Husain added a comment - docs.logging.patch applied. Leaving this open. The stack trace issue is still unresolved and there might be other comments.
        Hide
        Nathan Bubna added a comment -

        well, no one but Will has spoken about the weird, half-finished showStrackTraces behavior, and i've been thinking about it a bit more today.

        and now i think it should completely go away. seriously, it only ever partly worked; it doesn't appear to have been used much internally (i.e. few, if any, rsvc.info/warn/error(Throwable) calls); and it's no longer really necessary with the advent of the info/warn/error(String,Throwable) methods in the new Log class. now the user can control stacktrace output via choice of LogChute implementation and/or configuration of its end target.

        i say we remove all three properties from the default velocity.properties, deprecate the constants in RuntimeConstants, remove support for showStackTraces from Log and LogManager, and update the TemplateTestSuite to no longer use these useless settings. oh, and of course, remove these wholly from the documentation.

        i'll give this a few days for protests/comments, then i'll commit the change, which by the way, should be totally backwards compatible in that nothing will break and even users' log output won't change much, if it all.

        Show
        Nathan Bubna added a comment - well, no one but Will has spoken about the weird, half-finished showStrackTraces behavior, and i've been thinking about it a bit more today. and now i think it should completely go away. seriously, it only ever partly worked; it doesn't appear to have been used much internally (i.e. few, if any, rsvc.info/warn/error(Throwable) calls); and it's no longer really necessary with the advent of the info/warn/error(String,Throwable) methods in the new Log class. now the user can control stacktrace output via choice of LogChute implementation and/or configuration of its end target. i say we remove all three properties from the default velocity.properties, deprecate the constants in RuntimeConstants, remove support for showStackTraces from Log and LogManager, and update the TemplateTestSuite to no longer use these useless settings. oh, and of course, remove these wholly from the documentation. i'll give this a few days for protests/comments, then i'll commit the change, which by the way, should be totally backwards compatible in that nothing will break and even users' log output won't change much, if it all.
        Hide
        Will Glass-Husain added a comment -

        they seem pretty useless to me. I'm ok with this as long as the API for LogSystem and RuntimeInstance doesn't change.

        Show
        Will Glass-Husain added a comment - they seem pretty useless to me. I'm ok with this as long as the API for LogSystem and RuntimeInstance doesn't change.
        Hide
        Nathan Bubna added a comment -

        Ok, the last changes i checked in (revision 312681) should be the end of this issue.

        Show
        Nathan Bubna added a comment - Ok, the last changes i checked in (revision 312681) should be the end of this issue.
        Hide
        Henning Schmiedehausen added a comment -

        Close all resolved issues for Engine 1.5 release.

        Show
        Henning Schmiedehausen added a comment - Close all resolved issues for Engine 1.5 release.

          People

          • Assignee:
            Will Glass-Husain
            Reporter:
            Nathan Bubna
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development