OpenJPA
  1. OpenJPA
  2. OPENJPA-1557

Logging configuration is difficult for running tests

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 1.2.2, 2.0.0-beta2
    • Fix Version/s: 1.0.4, 1.2.3, 1.3.0, 2.0.0-beta3
    • Component/s: None
    • Labels:
      None

      Description

      Logging is difficult to configure and is incorrectly documented for running tests.

      There is a system property that can be set on the command line when running tests using surefire: openjpa.loglevel. This property, set via -Dopenjpa.loglevel, is used in the pom.xml to send logging properties to the runtime. The simple case works, e.g. -Dopenjpa.loglevel=error. This affects enhancement and the test runtime to report only errors.

      However, setting this property also results in warnings:
      enhance.all.entities:
      [echo] running enhancer
      12 WARN [main] openjpa.Runtime - The configuration property named "openjpa.loglevel" was not recognized and will be ignored, although the name closely matches a valid property called "openjpa.Log".
      2 WARN [main] openjpa.Runtime - The configuration property named "openjpa.loglevel" was not recognized and will be ignored, although the name closely matches a valid property called "openjpa.Log".

      This is because the property is both passed to the runtime and also used to configure the log level via the surefire plugin (the relevant part of the pom.xml follows):

      <property>
      <name>openjpa.Log</name>
      <value>DefaultLevel=$

      {openjpa.loglevel}

      </value>
      </property>

      Via "property injection" it's also possible to set any number of other log properties using this mechanism, although it's a bit odd. Just concatenate the extra properties after the level. For example,

      mvn test -Dopenjpa.loglevel=trace,file=openjpa.log,Runtime=info

      This will set the default log level to trace, send the log output to the file openjpa.log, and set the Runtime log level to info.

      The surefire plugin effectively disables the openjpa.Log so the user cannot use this property.

      The warning can be removed by changing the PersistenceTestCase.createNamedEMF:
      Map map = new HashMap(System.getProperties());
      map.remove("openjpa.level");

      But the enhancer doesn't go through this path.

      Before I change the online documentation I'd like to see if anyone else has any ideas how to make this easier.

        Activity

        Hide
        Michael Dick added a comment -

        Hi Craig,

        My comments on the mailing list were wrong. It was actually Marc who added the openjpa.loglevel property - back in http://svn.apache.org/viewvc?view=rev&rev=496880. I made some changes after that to pass it in to the enhancer though - and misremembered when the property was added.

        That said if you don't need to change the loglevel a relatively easy workaround is to set it in .m2/settings.xml - it's not a system property and the enhancer ignores it. This is what I've been using.

        That said I think the right answer is to remove loglevel and use openjpa.Log instead - the surefire plugin config can be changed to accommodate this.. I'll attach a patch shortly.

        Show
        Michael Dick added a comment - Hi Craig, My comments on the mailing list were wrong. It was actually Marc who added the openjpa.loglevel property - back in http://svn.apache.org/viewvc?view=rev&rev=496880 . I made some changes after that to pass it in to the enhancer though - and misremembered when the property was added. That said if you don't need to change the loglevel a relatively easy workaround is to set it in .m2/settings.xml - it's not a system property and the enhancer ignores it. This is what I've been using. That said I think the right answer is to remove loglevel and use openjpa.Log instead - the surefire plugin config can be changed to accommodate this.. I'll attach a patch shortly.
        Hide
        Michael Dick added a comment -

        The attached patch works for simple scenarios (-Dopenjpa.Log=DefaultLevel=FATAL, overrides openjpa.Log set in .m2/settings.xml, etc.).

        Show
        Michael Dick added a comment - The attached patch works for simple scenarios (-Dopenjpa.Log=DefaultLevel=FATAL, overrides openjpa.Log set in .m2/settings.xml, etc.).
        Hide
        Albert Lee added a comment -

        While we are working on the $

        {openjpa.loglevel}

        , can we rename this variable to say "loglevel.openjpa". It is because of the "openjpa." prefix, OpenJPA thought that it is a user mistake and put out a warning message.

        Albert Lee

        Show
        Albert Lee added a comment - While we are working on the $ {openjpa.loglevel} , can we rename this variable to say "loglevel.openjpa". It is because of the "openjpa." prefix, OpenJPA thought that it is a user mistake and put out a warning message. Albert Lee
        Hide
        Michael Dick added a comment -

        Good point - that'll also remove the warning messages.. You can still get yourself into trouble if you use -Dopenjpa.Log=DefaultLevel=FATAL on the maven command line though. System props won't override the value from config (IIRC) and we'll always pass in loglevel.openjpa on the config.

        As a quick fix I'd be okay with that though - but it'd be nice to have a single property that we use throughout the build and 'normal' configuration.

        Show
        Michael Dick added a comment - Good point - that'll also remove the warning messages.. You can still get yourself into trouble if you use -Dopenjpa.Log=DefaultLevel=FATAL on the maven command line though. System props won't override the value from config (IIRC) and we'll always pass in loglevel.openjpa on the config. As a quick fix I'd be okay with that though - but it'd be nice to have a single property that we use throughout the build and 'normal' configuration.
        Hide
        Craig L Russell added a comment -

        With the patch, my concerns go away.

        > You can still get yourself into trouble if you use -Dopenjpa.Log=DefaultLevel=FATAL on the maven command line though. System props won't override the value from config (IIRC) and we'll always pass in loglevel.openjpa on the config.

        I don't follow this comment. If we remove the openjpa.loglevel completely (my understanding of your patch) then you will never get the warning unless you accidentally supply openjpa.loglevel. Still, adding a property loglevel.openjpa is problematic because then you'd be in the same predicament by having loglevel.openjpa overriding the openjpa.Log property.

        With the patch, the only thing the user needs to do is to use the "long form" -Dopenjpa.Log=DefaultLevel=FATAL instead of the "short form" -Dopenjpa.loglevel=FATAL.

        And I'm happy to document the openjpa.Log that you need to use instead of openjpa.loglevel to change the logging levels. I wandered into this issue in the first place after finding conflicting information on our own web site.

        Show
        Craig L Russell added a comment - With the patch, my concerns go away. > You can still get yourself into trouble if you use -Dopenjpa.Log=DefaultLevel=FATAL on the maven command line though. System props won't override the value from config (IIRC) and we'll always pass in loglevel.openjpa on the config. I don't follow this comment. If we remove the openjpa.loglevel completely (my understanding of your patch) then you will never get the warning unless you accidentally supply openjpa.loglevel. Still, adding a property loglevel.openjpa is problematic because then you'd be in the same predicament by having loglevel.openjpa overriding the openjpa.Log property. With the patch, the only thing the user needs to do is to use the "long form" -Dopenjpa.Log=DefaultLevel=FATAL instead of the "short form" -Dopenjpa.loglevel=FATAL. And I'm happy to document the openjpa.Log that you need to use instead of openjpa.loglevel to change the logging levels. I wandered into this issue in the first place after finding conflicting information on our own web site.
        Hide
        Michael Dick added a comment -

        Hi Craig,

        The comment that didn't make sense was related to just renaming the propery loglevel.openjpa. It doesn't apply to the patch I attached. Sorry for the confusion.

        I'll go ahead and commit the patch, if there aren't any last minute objections.

        Show
        Michael Dick added a comment - Hi Craig, The comment that didn't make sense was related to just renaming the propery loglevel.openjpa. It doesn't apply to the patch I attached. Sorry for the confusion. I'll go ahead and commit the patch, if there aren't any last minute objections.
        Hide
        Michael Dick added a comment -

        Committed the patch on trunk - had to merge changes to slice, integration/daytrader and integration/validation.

        For some reason the slice unit tests do not pick up the openjpa.Log parameter when it's passed in as a system arg. They aren't particularly noisy tests so I'm shelving that issue for now.

        Show
        Michael Dick added a comment - Committed the patch on trunk - had to merge changes to slice, integration/daytrader and integration/validation. For some reason the slice unit tests do not pick up the openjpa.Log parameter when it's passed in as a system arg. They aren't particularly noisy tests so I'm shelving that issue for now.
        Hide
        Craig L Russell added a comment -

        I've applied the patch to 1.3.x.

        svn commit -m "OPENJPA-1557 remove openjpa.log from pom.xmls to avoid overwriting openjpa.Log" openjpa-persistence-jdbc/pom.xml openjpa-slice/pom.xml
        Sending openjpa-persistence-jdbc/pom.xml
        Sending openjpa-slice/pom.xml
        Transmitting file data ..
        Committed revision 926209.

        I'll take a look at the doc pages next.

        Show
        Craig L Russell added a comment - I've applied the patch to 1.3.x. svn commit -m " OPENJPA-1557 remove openjpa.log from pom.xmls to avoid overwriting openjpa.Log" openjpa-persistence-jdbc/pom.xml openjpa-slice/pom.xml Sending openjpa-persistence-jdbc/pom.xml Sending openjpa-slice/pom.xml Transmitting file data .. Committed revision 926209. I'll take a look at the doc pages next.
        Hide
        Donald Woods added a comment -

        If there are further doc updates, please checkin against OPENJPA-1557, but do not reopen, since this has been included in the beta3 release.

        Show
        Donald Woods added a comment - If there are further doc updates, please checkin against OPENJPA-1557 , but do not reopen, since this has been included in the beta3 release.
        Hide
        Craig L Russell added a comment -

        Just one last reference to openjpa.loglevel in the top level pom.xml properties:

        <openjpa.loglevel>INFO</openjpa.loglevel>

        Is this needed to avoid breaking anything else? Or should it be removed as well?

        Show
        Craig L Russell added a comment - Just one last reference to openjpa.loglevel in the top level pom.xml properties: <openjpa.loglevel>INFO</openjpa.loglevel> Is this needed to avoid breaking anything else? Or should it be removed as well?
        Hide
        Donald Woods added a comment -

        You should be able to remove it. Thought I got all of those....

        Show
        Donald Woods added a comment - You should be able to remove it. Thought I got all of those....
        Hide
        Craig L Russell added a comment -

        It turns out that openjpa-persistence-jdbc/src/main/ant/enhancer.xml has a dependency on openjpa.loglevel and it's not trivial to get rid of it. More work will be needed to remove it...

        Show
        Craig L Russell added a comment - It turns out that openjpa-persistence-jdbc/src/main/ant/enhancer.xml has a dependency on openjpa.loglevel and it's not trivial to get rid of it. More work will be needed to remove it...
        Hide
        Donald Woods added a comment -

        Which branch and revision are you looking at?
        I removed the remaining openjpa.loglevel usage in 1.3.x using r926702....
        There are no openjpa.loglevel references in the latest /trunk or branches/2.0.x codebases.

        Show
        Donald Woods added a comment - Which branch and revision are you looking at? I removed the remaining openjpa.loglevel usage in 1.3.x using r926702.... There are no openjpa.loglevel references in the latest /trunk or branches/2.0.x codebases.
        Hide
        Michael Dick added a comment -

        1.3.x has been building for me with Donald's last round of changes, fwiw.

        I'll take a look again this weekend and maybe merge to 1.2.x / 1.0.x.

        Show
        Michael Dick added a comment - 1.3.x has been building for me with Donald's last round of changes, fwiw. I'll take a look again this weekend and maybe merge to 1.2.x / 1.0.x.
        Hide
        Michael Dick added a comment -

        reopening to indicate it's fixed in 1.0.4 and 1.2.3

        Show
        Michael Dick added a comment - reopening to indicate it's fixed in 1.0.4 and 1.2.3

          People

          • Assignee:
            Craig L Russell
            Reporter:
            Craig L Russell
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development