Upgrade to micrometer 1.0.0-rc.5 #11071

jkschneider · 2017-11-17T21:15:11Z

Intended for Boot 2 M7.

jkschneider · 2017-11-17T21:15:48Z

...in/java/org/springframework/boot/actuate/autoconfigure/metrics/MetricsAutoConfiguration.java

@@ -75,6 +78,11 @@
 		SimpleExportConfiguration.class, StatsdExportConfiguration.class })
 @AutoConfigureAfter(DataSourceAutoConfiguration.class)
 public class MetricsAutoConfiguration {
+	@Bean
+	@Order(0)


Sanity check please?

I don't think you can use @Order on @Bean methods.

Scrub that, yes you can.

jkschneider · 2017-11-17T21:16:09Z

...or/src/main/java/org/springframework/boot/actuate/metrics/config/EnvironmentMeterFilter.java

+ */
+public class EnvironmentMeterFilter extends PropertyMeterFilter {
+	private final Environment environment;
+	private final DefaultConversionService conversionService = new DefaultConversionService();


Is there a better way of accessing conversions in Boot 2?

I think it's a bean and you can inject one. See here and the auto-configuration

@spencergibb In a vanilla Spring Boot 1.5.x app, it is not injectable. Must be configured somewhere. I think they've been fiddling with conversion service in 2.x though?

This feels like a brittle way to support nicer Duration parsing. I'm worried that we're going directly to the Environment and possibly bypassing the relaxed binding rules. I'm tempted to pull that out and create a nice general way to bind Durations with a simple format.

jkschneider · 2017-11-17T21:17:05Z

...or/src/main/java/org/springframework/boot/actuate/metrics/config/EnvironmentMeterFilter.java

+		return null;
+	}
+
+	private static Duration simpleParse(String rawTime) {


Feedback from users indicated they wanted to be able to define props like 1s rather than PT1S (sometimes). Not sure if you want this to be a capability of the default Duration conversion.

philwebb · 2017-11-18T20:10:38Z

...a/org/springframework/boot/actuate/autoconfigure/metrics/export/simple/SimpleProperties.java

@@ -42,6 +43,8 @@
 	 */
 	private Duration step = Duration.ofSeconds(10);

+	private CountingMode mode = CountingMode.Cumulative;


This is an unusual enum format. Any reason for Cumulative, Step and not CUMULATIVE, STEP? Binding should work regardless I think.

philwebb · 2017-11-18T20:12:18Z

.../main/java/org/springframework/boot/actuate/metrics/async/ThreadPoolTaskExecutorMetrics.java

+ * wrapped. Timings are provided separately by wrapping the executor service with
+ * {@link TimedThreadPoolTaskExecutor}.
+ *
+ * @author David Held


Has the author signed the CLA? We can't accept this if not.

@dheld-expedia are you the original author for this? Any chance you could consider submitting it as a PR for Boot so the CLA bot confirms that you've signed the CLA.

philwebb · 2017-11-18T20:26:19Z

...or/src/main/java/org/springframework/boot/actuate/metrics/config/EnvironmentMeterFilter.java

+ */
+public class EnvironmentMeterFilter extends PropertyMeterFilter {
+	private final Environment environment;
+	private final DefaultConversionService conversionService = new DefaultConversionService();


This feels like a brittle way to support nicer Duration parsing. I'm worried that we're going directly to the Environment and possibly bypassing the relaxed binding rules. I'm tempted to pull that out and create a nice general way to bind Durations with a simple format.

philwebb · 2017-11-18T23:57:31Z

@jkschneider I've got a simplified branch ready to merge here. This drops the ThreadPoolTaskExecutorMetrics since we need the original author to submit that one. It also removes the Duration parsing in EnvironmentMeterFilter since I think we should add a general solution for that.

Unfortunately, I still can't merge this due to the following:

Caused by: java.lang.NoClassDefFoundError: com/google/common/util/concurrent/AtomicDouble
	at io.micrometer.core.instrument.simple.CumulativeCounter.<init>(CumulativeCounter.java:28)
	at io.micrometer.core.instrument.simple.SimpleMeterRegistry.newCounter(SimpleMeterRegistry.java:128)
	at io.micrometer.core.instrument.MeterRegistry.lambda$registerMeterIfNecessary$5(MeterRegistry.java:629)
	at io.micrometer.core.instrument.MeterRegistry.registerMeterIfNecessary(MeterRegistry.java:664)
	at io.micrometer.core.instrument.MeterRegistry.registerMeterIfNecessary(MeterRegistry.java:629)
	at io.micrometer.core.instrument.MeterRegistry.counter(MeterRegistry.java:130)
	at io.micrometer.core.instrument.Counter$Builder.register(Counter.java:92)
	at io.micrometer.core.instrument.composite.CompositeCounter.registerNewMeter(CompositeCounter.java:50)
	at io.micrometer.core.instrument.composite.CompositeCounter.registerNewMeter(CompositeCounter.java:23)
	at io.micrometer.core.instrument.composite.AbstractCompositeMeter.add(AbstractCompositeMeter.java:93)
	at java.util.concurrent.ConcurrentHashMap$KeySetView.forEach(ConcurrentHashMap.java:4649)
	at io.micrometer.core.instrument.composite.CompositeMeterRegistry.newCounter(CompositeMeterRegistry.java:66)
	at io.micrometer.core.instrument.MeterRegistry.lambda$registerMeterIfNecessary$5(MeterRegistry.java:629)
	at io.micrometer.core.instrument.MeterRegistry.registerMeterIfNecessary(MeterRegistry.java:664)
	at io.micrometer.core.instrument.MeterRegistry.registerMeterIfNecessary(MeterRegistry.java:629)
	at io.micrometer.core.instrument.MeterRegistry.counter(MeterRegistry.java:130)
	at io.micrometer.core.instrument.Counter$Builder.register(Counter.java:92)
	at io.micrometer.core.instrument.binder.logging.MetricsTurboFilter.<init>(LogbackMetrics.java:64)
	at io.micrometer.core.instrument.binder.logging.LogbackMetrics.bindTo(LogbackMetrics.java:49)
	at org.springframework.boot.actuate.autoconfigure.metrics.MetricsAutoConfiguration$MeterRegistryConfigurationSupport.lambda$new$1(MetricsAutoConfiguration.java:128)
	at java.util.LinkedHashMap$LinkedValues.forEach(LinkedHashMap.java:608)
	at org.springframework.boot.actuate.autoconfigure.metrics.MetricsAutoConfiguration$MeterRegistryConfigurationSupport.<init>(MetricsAutoConfiguration.java:128)
	at org.springframework.boot.actuate.autoconfigure.metrics.MetricsAutoConfiguration$MeterRegistryConfigurationSupport$$EnhancerBySpringCGLIB$$befa960d.<init>(<generated>)
	at sun.reflect.NativeConstructorAccessorImpl.newInstance0(Native Method)
	at sun.reflect.NativeConstructorAccessorImpl.newInstance(NativeConstructorAccessorImpl.java:62)
	at sun.reflect.DelegatingConstructorAccessorImpl.newInstance(DelegatingConstructorAccessorImpl.java:45)
	at java.lang.reflect.Constructor.newInstance(Constructor.java:423)
	at org.springframework.beans.BeanUtils.instantiateClass(BeanUtils.java:163)
	... 23 more
Caused by: java.lang.ClassNotFoundException: com.google.common.util.concurrent.AtomicDouble
	at java.net.URLClassLoader.findClass(URLClassLoader.java:381)

I'm guessing that this import is wrong.

jkschneider · 2017-11-28T14:37:57Z

Guava has been removed in rc5 with micrometer-metrics/micrometer#234

See gh-11071

* pr/11071: Polish micrometer rc.5 upgrade Upgrade to micrometer 1.0.0-rc.5

See gh-11071

Upgrade to micrometer 1.0.0-rc.4

3cf6361

jkschneider commented Nov 17, 2017

View reviewed changes

spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Nov 17, 2017

philwebb added priority: normal type: enhancement A general enhancement and removed status: waiting-for-triage An issue we've not yet triaged labels Nov 17, 2017

philwebb added this to the 2.0.0.M7 milestone Nov 17, 2017

philwebb reviewed Nov 18, 2017

View reviewed changes

philwebb added the status: blocked An issue that's blocked on an external project change label Nov 18, 2017

philwebb mentioned this pull request Nov 19, 2017

Allow more flexible Duration parsing #11078

Closed

jkschneider changed the title ~~Upgrade to micrometer 1.0.0-rc.4~~ Upgrade to micrometer 1.0.0-rc.5 Nov 28, 2017

snicoll removed the status: blocked An issue that's blocked on an external project change label Nov 28, 2017

philwebb pushed a commit that referenced this pull request Nov 29, 2017

Upgrade to micrometer 1.0.0-rc.5

2e0a915

See gh-11071

philwebb closed this in e1306c6 Nov 29, 2017

philwebb added a commit that referenced this pull request Nov 29, 2017

Merge pull request #11071 from jkschneider/micrometer-rc4

1bdd42e

* pr/11071: Polish micrometer rc.5 upgrade Upgrade to micrometer 1.0.0-rc.5

snicoll added a commit that referenced this pull request Nov 29, 2017

Polish

6be3004

See gh-11071

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Upgrade to micrometer 1.0.0-rc.5 #11071

Upgrade to micrometer 1.0.0-rc.5 #11071

jkschneider commented Nov 17, 2017

jkschneider Nov 17, 2017

philwebb Nov 18, 2017

philwebb Nov 18, 2017

jkschneider Nov 17, 2017

spencergibb Nov 17, 2017

jkschneider Nov 18, 2017 •

edited

Loading

philwebb Nov 18, 2017

jkschneider Nov 17, 2017

philwebb Nov 18, 2017

philwebb Nov 18, 2017

philwebb Nov 22, 2017

philwebb Nov 18, 2017

philwebb commented Nov 18, 2017

jkschneider commented Nov 28, 2017

Upgrade to micrometer 1.0.0-rc.5 #11071

Upgrade to micrometer 1.0.0-rc.5 #11071

Conversation

jkschneider commented Nov 17, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkschneider Nov 18, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

philwebb commented Nov 18, 2017

jkschneider commented Nov 28, 2017

jkschneider Nov 18, 2017 •

edited

Loading