glassfish
  1. glassfish
  2. GLASSFISH-15923

A monitoring module was added in 3.1, "deployment" - but enable-monitoring doesn't support it

    Details

    • Type: Bug Bug
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 3.1
    • Component/s: monitoring
    • Labels:
      None

      Description

      I added 360 new Monitoring Dev Tests and this problem was detected.

      org.glassfish.api.monitoring.ContainerMonitoring has a hard-coded list of all Monitoring modules. enable-monitoring uses this list.

      Sometime during 3.1 a new module was added – "deployment". But that file was not updated. As a result if you try something like the following you will get an error:

      asadmin enable-monitoring --modules deployment:LOW

      com.sun.enterprise.config.serverbeans.ModuleMonitoringLevels has a getter/setter added for the new module.

      I don't know if other areas are also affected.

      ======

      The fix would be trivial – add a test and call to the getter.

        Activity

        Hide
        Byron Nevins added a comment -

        ========================
        1. How bad is its impact? (Severity)
        The impact is pretty bad. The fix is trivial and has no risk.

        • Identify why the fix needs to occur now:
          o Likely to generate a customer support call

        2. How often does it happen? (Frequency)
        100%

        3. How much effort is required to fix it? (Cost)
        Zero cost – I have a fix ready to go. It is easy.

        4. What is the risk of fixing it? (Risk)
        Very Low risk.

        5. Does a work around for the issue exist? Can the workaround be reasonably employed by the end user?
        Yes and yes. It is the usual answer for any fouled-up command that exclusively sets config – the work-around is to use the low-level "set" command.

        6. If the issue is not fixed should the issue and its workaround (if applicable) be described in the Release Notes?
        Definitely.

        7. How long has the bug existed in the product?
        Since whenever the "deployment" module was added. It might be in 3.0

        8. Do regression tests exist for this issue?
        Yes. Monitoring DevTests detected the problem today!

        9. Which tests should QA (re)run to verify the fix did not destabilize GlassFish?

        It's not that sort of issue. It is self-contained. To be very safe I would add a tiny minimal change in 3.1 branch and do the full change (change to ContainerMonitoring in the api package) to the trunk.

        QA never tested it – it would have failed instantly.

        10. When will a tested fix be ready for integration?
        Code changes – a few minutes. Code review and red-tape – a few hours

        Show
        Byron Nevins added a comment - ======================== 1. How bad is its impact? (Severity) The impact is pretty bad. The fix is trivial and has no risk. Identify why the fix needs to occur now: o Likely to generate a customer support call 2. How often does it happen? (Frequency) 100% 3. How much effort is required to fix it? (Cost) Zero cost – I have a fix ready to go. It is easy. 4. What is the risk of fixing it? (Risk) Very Low risk. 5. Does a work around for the issue exist? Can the workaround be reasonably employed by the end user? Yes and yes. It is the usual answer for any fouled-up command that exclusively sets config – the work-around is to use the low-level "set" command. 6. If the issue is not fixed should the issue and its workaround (if applicable) be described in the Release Notes? Definitely. 7. How long has the bug existed in the product? Since whenever the "deployment" module was added. It might be in 3.0 8. Do regression tests exist for this issue? Yes. Monitoring DevTests detected the problem today! 9. Which tests should QA (re)run to verify the fix did not destabilize GlassFish? It's not that sort of issue. It is self-contained. To be very safe I would add a tiny minimal change in 3.1 branch and do the full change (change to ContainerMonitoring in the api package) to the trunk. QA never tested it – it would have failed instantly. 10. When will a tested fix be ready for integration? Code changes – a few minutes. Code review and red-tape – a few hours
        Hide
        Byron Nevins added a comment -

        DETAILS:

        existing pseudo-code:

        if(moduleName.equals(ContainerMonitoring.HTTP_SERVICE)
        obj.setHttpService(level);
        // 14 similar else blocks for the other 14 modules.

        What I need to do is add one else clause exactly like this:

        else if(moduleName.equals("deployment"))
        obj.setDeployment(level);

        Simple! setDeployment() already exists – there just is no corresponding constant in ContainerMonitoring.

        ======================

        Show
        Byron Nevins added a comment - DETAILS: existing pseudo-code: if(moduleName.equals(ContainerMonitoring.HTTP_SERVICE) obj.setHttpService(level); // 14 similar else blocks for the other 14 modules. What I need to do is add one else clause exactly like this: else if(moduleName.equals("deployment")) obj.setDeployment(level); Simple! setDeployment() already exists – there just is no corresponding constant in ContainerMonitoring. ======================
        Hide
        Byron Nevins added a comment -

        A better solution needs to be Engineered for 3.2 so new modules can be added w/o having to remember to update code in different modules. It's a classic use-case of the Java Bean API and reflection. I'll open a new bug for 3.2 and point it to this one...

        Index: src/main/java/org/glassfish/flashlight/cli/MonitoringConfig.java
        ===================================================================
        — src/main/java/org/glassfish/flashlight/cli/MonitoringConfig.java (revision 45009)
        +++ src/main/java/org/glassfish/flashlight/cli/MonitoringConfig.java (working copy)
        @@ -166,6 +166,8 @@
        param.setJpa(level);
        } else if (moduleName.equals(ContainerMonitoring.JERSEY))

        { param.setJersey(level); + }

        else if (moduleName.equals("deployment"))

        { + param.setDeployment(level); }

        else

        { valueUpdated.set(false); }

        =======================

        I've also turned back on the Monitoring DevTests that identified this bug in the first place...

        Show
        Byron Nevins added a comment - A better solution needs to be Engineered for 3.2 so new modules can be added w/o having to remember to update code in different modules. It's a classic use-case of the Java Bean API and reflection. I'll open a new bug for 3.2 and point it to this one... Index: src/main/java/org/glassfish/flashlight/cli/MonitoringConfig.java =================================================================== — src/main/java/org/glassfish/flashlight/cli/MonitoringConfig.java (revision 45009) +++ src/main/java/org/glassfish/flashlight/cli/MonitoringConfig.java (working copy) @@ -166,6 +166,8 @@ param.setJpa(level); } else if (moduleName.equals(ContainerMonitoring.JERSEY)) { param.setJersey(level); + } else if (moduleName.equals("deployment")) { + param.setDeployment(level); } else { valueUpdated.set(false); } ======================= I've also turned back on the Monitoring DevTests that identified this bug in the first place...
        Hide
        Byron Nevins added a comment -

        Sorry – tons of comments but we are at that phase of the project!

        Output of the Monitoring DevTests:

        dev-report:
        [java] **********************
        [java] * PASSED 375 *
        [java] * FAILED 0 *
        [java] * DID NOT RUN 0 *
        [java] * TOTAL 375 *
        [java] **********************
        [java]
        [echo] Detailed results available under d:/gf/v2/appserv-tests/test_results.html

        BUILD SUCCESSFUL
        Total time: 4 minutes 2 seconds

        Show
        Byron Nevins added a comment - Sorry – tons of comments but we are at that phase of the project! Output of the Monitoring DevTests: dev-report: [java] ********************** [java] * PASSED 375 * [java] * FAILED 0 * [java] * DID NOT RUN 0 * [java] * TOTAL 375 * [java] ********************** [java] [echo] Detailed results available under d:/gf/v2/appserv-tests/test_results.html BUILD SUCCESSFUL Total time: 4 minutes 2 seconds
        Hide
        Byron Nevins added a comment -

        D:\gf\v3\flashlight\framework\foo\framework>svn commit
        Sending src\main\java\org\glassfish\flashlight\cli\MonitoringConfig.java
        Transmitting file data .
        Committed revision 45032.

        This fix is only for the 3.1 Branch.

        Show
        Byron Nevins added a comment - D:\gf\v3\flashlight\framework\foo\framework>svn commit Sending src\main\java\org\glassfish\flashlight\cli\MonitoringConfig.java Transmitting file data . Committed revision 45032. This fix is only for the 3.1 Branch.
        Hide
        Byron Nevins added a comment -

        here is the commit to the trunk

        D:\gf\v3>svn commit flashlight\framework\src\main\java\org\glassfish\flashlight\cli\MonitoringConfig.java common\glassfish-api\src\main\java\org\glass
        fish\api\monitoring\ContainerMonitoring.java
        Sending common\glassfish-api\src\main\java\org\glassfish\api\monitoring\ContainerMonitoring.java
        Sending flashlight\framework\src\main\java\org\glassfish\flashlight\cli\MonitoringConfig.java
        Transmitting file data ..
        Committed revision 45033.

        Show
        Byron Nevins added a comment - here is the commit to the trunk D:\gf\v3>svn commit flashlight\framework\src\main\java\org\glassfish\flashlight\cli\MonitoringConfig.java common\glassfish-api\src\main\java\org\glass fish\api\monitoring\ContainerMonitoring.java Sending common\glassfish-api\src\main\java\org\glassfish\api\monitoring\ContainerMonitoring.java Sending flashlight\framework\src\main\java\org\glassfish\flashlight\cli\MonitoringConfig.java Transmitting file data .. Committed revision 45033.
        Hide
        Byron Nevins added a comment -

        Added a devtest 3/15/2011

        note that the command is

        asadmin enable-monitoring --modules deployment=LOW
        not
        asadmin enable-monitoring --modules deployment:LOW

        !!!!

        Show
        Byron Nevins added a comment - Added a devtest 3/15/2011 note that the command is asadmin enable-monitoring --modules deployment=LOW not asadmin enable-monitoring --modules deployment:LOW !!!!

          People

          • Assignee:
            Byron Nevins
            Reporter:
            Byron Nevins
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved: