-
New Feature
-
Resolution: Fixed
-
Major
-
None
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
[JENKINS-52061] Upgrade metrics to 4.0.x
| 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
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
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
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 stephenconnolly 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.
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
Analysis of upgrade from 3.1.2 to 4.0.2 using https://github.com/lvc/japi-compliance-checker
So the only risk is metrics-core where the JMX classes have been removed and moved to metrics-jmx under a new package name