Details

    • Similar Issues:

      Description

      Now that there is a significant number of Jenkins releases requiring Java 8, we can upgrade to DropWizard Metrics 4.0.x series.

      This will require a core version bump

        Attachments

          Activity

          Hide
          stephenconnolly Stephen Connolly added a comment -

          Analysis of upgrade from 3.1.2 to 4.0.2 using https://github.com/lvc/japi-compliance-checker

          Library Source Compatibility Binary Compatibility
          metrics-core 81.2% 81.2%
          metrics-healthchecks 100% 100%
          metrics-json 100% 100%
          metrics-jvm 100% 100%
          metrics-servlet 100% 100%

          So the only risk is metrics-core where the JMX classes have been removed and moved to metrics-jmx under a new package name

          Show
          stephenconnolly Stephen Connolly added a comment - Analysis of upgrade from 3.1.2 to 4.0.2 using https://github.com/lvc/japi-compliance-checker Library Source Compatibility Binary Compatibility metrics-core 81.2% 81.2% metrics-healthchecks 100% 100% metrics-json 100% 100% metrics-jvm 100% 100% metrics-servlet 100% 100% So the only risk is metrics-core where the JMX classes have been removed and moved to metrics-jmx under a new package name
          Hide
          stephenconnolly Stephen Connolly added a comment -
          Search Hits
          https://github.com/search?q=user%3Ajenkinsci+Clock.CpuTimeClock&type=Code NONE
          https://github.com/search?q=user%3Ajenkinsci+DefaultObjectNameFactory&type=Code NONE
          https://github.com/search?q=user%3Ajenkinsci+JmxAttributeGauge&type=Code NONE
          https://github.com/search?q=user%3Ajenkinsci+JmxReporter&type=Code 1 (metrics-plugin)
          https://github.com/search?q=user%3Ajenkinsci+JvmAttributeGaugeSet&type=Code NONE
          https://github.com/search?q=user%3Ajenkinsci+ObjectNameFactory&type=Code NONE

          On this basis the removed classes do not pose a risk as the only reference will be fixed by the upgrade.

          Remaining upgrade risk assessment is the change in the interfaces of Timer.Context, which now only implements AutoClosable rather than Closeable. I need to check that code compiled against Java 6 with the old contract runs with the new contract on Java 8

          Show
          stephenconnolly Stephen Connolly added a comment - Search Hits https://github.com/search?q=user%3Ajenkinsci+Clock.CpuTimeClock&type=Code NONE https://github.com/search?q=user%3Ajenkinsci+DefaultObjectNameFactory&type=Code NONE https://github.com/search?q=user%3Ajenkinsci+JmxAttributeGauge&type=Code NONE https://github.com/search?q=user%3Ajenkinsci+JmxReporter&type=Code 1 (metrics-plugin) https://github.com/search?q=user%3Ajenkinsci+JvmAttributeGaugeSet&type=Code NONE https://github.com/search?q=user%3Ajenkinsci+ObjectNameFactory&type=Code NONE On this basis the removed classes do not pose a risk as the only reference will be fixed by the upgrade. Remaining upgrade risk assessment is the change in the interfaces of Timer.Context , which now only implements AutoClosable rather than Closeable . I need to check that code compiled against Java 6 with the old contract runs with the new contract on Java 8
          Hide
          stephenconnolly Stephen Connolly added a comment -

          Ok, the change in super-interfaces of Timer.Context should not cause any issue directly, as both the new and old versions of Timer.Context override the close method so any latent references should be to the overridden method...

          e.g. I compiled this code

          package test;
          
          import com.codahale.metrics.Timer;
          
          public class It {
              public static void main(String[] args) throws Exception {
                  Timer t = new Timer();
                  Timer.Context ctx = t.time();
                  try {
                      Thread.sleep(100);
                  } finally {
                      ctx.close();
                  }
                  System.out.println(t.getMeanRate());
              }
          }
          

          Using Java 6... this decompiles to

          public class test.It {
            public test.It();
              Code:
                 0: aload_0
                 1: invokespecial #1                  // Method java/lang/Object."<init>":()V
                 4: return
          
            public static void main(java.lang.String[]) throws java.lang.Exception;
              Code:
                 0: new           #2                  // class com/codahale/metrics/Timer
                 3: dup
                 4: invokespecial #3                  // Method com/codahale/metrics/Timer."<init>":()V
                 7: astore_1
                 8: aload_1
                 9: invokevirtual #4                  // Method com/codahale/metrics/Timer.time:()Lcom/codahale/metrics/Timer$Context;
                12: astore_2
                13: ldc2_w        #5                  // long 100l
                16: invokestatic  #7                  // Method java/lang/Thread.sleep:(J)V
                19: aload_2
                20: invokevirtual #8                  // Method com/codahale/metrics/Timer$Context.close:()V
                23: goto          33
                26: astore_3
                27: aload_2
                28: invokevirtual #8                  // Method com/codahale/metrics/Timer$Context.close:()V
                31: aload_3
                32: athrow
                33: getstatic     #9                  // Field java/lang/System.out:Ljava/io/PrintStream;
                36: aload_1
                37: invokevirtual #10                 // Method com/codahale/metrics/Timer.getMeanRate:()D
                40: invokevirtual #11                 // Method java/io/PrintStream.println:(D)V
                43: return
              Exception table:
                 from    to  target type
                    13    19    26   any
                    26    27    26   any
          }
          

          And we can see that the invokevirtual references the com/codahale/metrics/Timer$Context.close)V method, which has not been removed.

          There could be an issue if somebody has used an old version of IOUtils.closeQuietly(Closeable)...

          https://github.com/search?q=user%3Ajenkinsci+%22Timer.Context%22&type=Code reveals 9 uses of Timer.Context, most in metrics plugin, so not an issue.

          On this basis I conclude that the binary API changes are low risk and limited to the JMX classes only

          Show
          stephenconnolly Stephen Connolly added a comment - Ok, the change in super-interfaces of Timer.Context should not cause any issue directly, as both the new and old versions of Timer.Context override the close method so any latent references should be to the overridden method... e.g. I compiled this code package test; import com.codahale.metrics.Timer; public class It { public static void main( String [] args) throws Exception { Timer t = new Timer(); Timer.Context ctx = t.time(); try { Thread .sleep(100); } finally { ctx.close(); } System .out.println(t.getMeanRate()); } } Using Java 6... this decompiles to public class test.It { public test.It(); Code: 0: aload_0 1: invokespecial #1 // Method java/lang/ Object . "<init>" :()V 4: return public static void main(java.lang. String []) throws java.lang.Exception; Code: 0: new #2 // class com/codahale/metrics/Timer 3: dup 4: invokespecial #3 // Method com/codahale/metrics/Timer. "<init>" :()V 7: astore_1 8: aload_1 9: invokevirtual #4 // Method com/codahale/metrics/Timer.time:()Lcom/codahale/metrics/Timer$Context; 12: astore_2 13: ldc2_w #5 // long 100l 16: invokestatic #7 // Method java/lang/ Thread .sleep:(J)V 19: aload_2 20: invokevirtual #8 // Method com/codahale/metrics/Timer$Context.close:()V 23: goto 33 26: astore_3 27: aload_2 28: invokevirtual #8 // Method com/codahale/metrics/Timer$Context.close:()V 31: aload_3 32: athrow 33: getstatic #9 // Field java/lang/ System .out:Ljava/io/PrintStream; 36: aload_1 37: invokevirtual #10 // Method com/codahale/metrics/Timer.getMeanRate:()D 40: invokevirtual #11 // Method java/io/PrintStream.println:(D)V 43: return Exception table: from to target type 13 19 26 any 26 27 26 any } And we can see that the invokevirtual references the com/codahale/metrics/Timer$Context.close )V method, which has not been removed. There could be an issue if somebody has used an old version of IOUtils.closeQuietly(Closeable)... https://github.com/search?q=user%3Ajenkinsci+%22Timer.Context%22&type=Code reveals 9 uses of Timer.Context , most in metrics plugin, so not an issue. support-core uses Timer.Context but does not use the close method, calling stop instead https://github.com/jenkinsci/support-core-plugin/blob/2889a28ae41ab7379127c413ae112e9243c778ba/src/main/java/com/cloudbees/jenkins/support/SupportMetricsFilter.java#L98-L105 jenkow plugin is a false match due to text search mesos-plugin is the only plugin with significant usage outside of metrics plugin, and it only ever calls stop directly On this basis I conclude that the binary API changes are low risk and limited to the JMX classes only
          Hide
          stephenconnolly Stephen Connolly added a comment -

          Could also be a risk if somebody did

          try (Closeable ctx = timer.time()) {
            ...
          }
          

          But given there are only 9 uses of Timer: https://github.com/search?q=user%3Ajenkinsci+com.codahale.metrics.Timer&type=Code (and I checked the closed source usage that I have access to, and none of them cast to Closeable) I consider this low risk

          Show
          stephenconnolly Stephen Connolly added a comment - Could also be a risk if somebody did try (Closeable ctx = timer.time()) { ... } But given there are only 9 uses of Timer: https://github.com/search?q=user%3Ajenkinsci+com.codahale.metrics.Timer&type=Code (and I checked the closed source usage that I have access to, and none of them cast to Closeable ) I consider this low risk
          Hide
          oleg_nenashev Oleg Nenashev added a comment -

          I have went through the analysis and some links (like https://abi-laboratory.pro/index.php?view=compat_report&lang=java&l=metrics-core&v1=3.2.6&v2=4.0.0&obj=2932a&kind=bin#Removed). It seems that the analysis done by Stephen Connolly is correct.

          I am fine with upgrading assuming that ATH tests are done with this plugin (assuming they have some coverage for it). Even if there is no ATH tests for Metrics specifically (https://github.com/jenkinsci/acceptance-test-harness/tree/master/src/main/java/org/jenkinsci/test/acceptance/plugins), such test may reveal some issues if Codahale Metrics is used in libraries bundled in other plugins.

          Actually, such transient usages in libs are still the risk, because they are not visible in the GitHub search. Have you investigated that there is no plugins bundling the Codahale Metrics JAR? I am not aware about any such usages, but double-checking dependency trees could be useful to reduce the risk.

          Show
          oleg_nenashev Oleg Nenashev added a comment - I have went through the analysis and some links (like https://abi-laboratory.pro/index.php?view=compat_report&lang=java&l=metrics-core&v1=3.2.6&v2=4.0.0&obj=2932a&kind=bin#Removed ). It seems that the analysis done by Stephen Connolly is correct. I am fine with upgrading assuming that ATH tests are done with this plugin (assuming they have some coverage for it). Even if there is no ATH tests for Metrics specifically ( https://github.com/jenkinsci/acceptance-test-harness/tree/master/src/main/java/org/jenkinsci/test/acceptance/plugins ), such test may reveal some issues if Codahale Metrics is used in libraries bundled in other plugins. Actually, such transient usages in libs are still the risk, because they are not visible in the GitHub search. Have you investigated that there is no plugins bundling the Codahale Metrics JAR? I am not aware about any such usages, but double-checking dependency trees could be useful to reduce the risk.
          Hide
          stephenconnolly Stephen Connolly added a comment -

          Oleg Nenashev thanks for your review of my analysis. As regards secondary usage, if they bundle the dropwizard metrics jar then likely they have dependency conflicts in any Jenkins with both plugins installed already (unless they do child first classloader, in which case this change does not affect them)

          If they depend on the metrics plugin (recommended) and use an exclusion then there is a risk, but I checked all the pom.xml files for metrics ( https://github.com/search?l=Maven+POM&p=3&q=user%3Ajenkinsci+metrics&type=Code ), and none of them look to be pulling in any transitive consumers.

          Given that none of the plugins in the ATH depend on the metrics plugin, I don't see much value in the ATH for this upgrade... I'd have more confidence in my testing with some proprietary plugins that have more advanced usage of metrics

          Show
          stephenconnolly Stephen Connolly added a comment - Oleg Nenashev thanks for your review of my analysis. As regards secondary usage, if they bundle the dropwizard metrics jar then likely they have dependency conflicts in any Jenkins with both plugins installed already (unless they do child first classloader, in which case this change does not affect them) If they depend on the metrics plugin (recommended) and use an exclusion then there is a risk, but I checked all the pom.xml files for metrics ( https://github.com/search?l=Maven+POM&p=3&q=user%3Ajenkinsci+metrics&type=Code ), and none of them look to be pulling in any transitive consumers. Given that none of the plugins in the ATH depend on the metrics plugin, I don't see much value in the ATH for this upgrade... I'd have more confidence in my testing with some proprietary plugins that have more advanced usage of metrics
          Hide
          oleg_nenashev Oleg Nenashev added a comment -

          Agreed

          Show
          oleg_nenashev Oleg Nenashev added a comment - Agreed

            People

            • Assignee:
              stephenconnolly Stephen Connolly
              Reporter:
              stephenconnolly Stephen Connolly
            • Votes:
              0 Vote for this issue
              Watchers:
              2 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: