New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Introduce micrometer module #791

Merged
merged 20 commits into from Feb 4, 2019

Conversation

@mvanzelst
Copy link

mvanzelst commented Sep 24, 2018

Since spring boot 2 removed dropwizard metrics in favour of micrometer I think it will be a good idea if the axon-metrics module also uses micrometer.

I didn't check the backward compatibility with spring boot 1 yet. Micrometer does support spring boot 1 via the legacy module: http://micrometer.io/docs/ref/spring/1.5. So presumably by adding the legacy module to a spring boot 1 application the axon-metrics module should work in the same way. It's hard to use automated tests to verify this since the axon-spring-boot-autoconfigure tests use spring boot 2. I'm not sure how important the backward compatibility with spring boot 1 is for the metrics module.

Marijn van Zelst added some commits Aug 24, 2018

Marijn van Zelst
Replaced dropwizard metrics with micrometer in metrics module
- changed transitive dependency of spring-boot-starter-actuator in axon-spring-boot-autoconfigure to test dependency
- reinstate the creation of the meterRegistry when missing
Marijn van Zelst

@mvanzelst mvanzelst changed the title Replaced dropwizard metrics with micrometer in metrics module Replace dropwizard metrics with micrometer in metrics module Sep 25, 2018

Marijn van Zelst added some commits Oct 5, 2018

Marijn van Zelst
Merge remote-tracking branch 'axon/master' into micrometer-metrics
# Conflicts:
#	spring-boot-autoconfigure/src/test/java/org/axonframework/boot/AxonAutoConfigurationWithEventSerializerPropertiesTest.java
Marijn van Zelst
Replaced dropwizard metrics with micrometer in metrics module
- Create DropwizardMeterRegistry when dropwizard is on the classpath (fixes backwards compatibility for spring boot 1 applications)
- Removed test dependency
@mvanzelst

This comment has been minimized.

Copy link
Author

mvanzelst commented Oct 12, 2018

Made some changes to support metrics in Spring Boot 1 applications.

In order for the spring boot 1 actuator metrics endpoint to work there needs to be a dropwizard metricregistry. I added an auto configure class to add a micrometer wrapped dropwizard registry. This fixes the metrics endpoint.

I did remove the dropwizard-metrics dependency from the metrics module. So spring boot 1 users should add the dependency on dropwizard metrics to their own project.

@mvanzelst

This comment has been minimized.

Copy link
Author

mvanzelst commented Oct 19, 2018

Build seems to fail on an unrelated test. Build does work on my machine ;) How can I trigger a rebuild?

Marijn van Zelst added some commits Oct 19, 2018

Marijn van Zelst
Replaced dropwizard metrics with micrometer in metrics module
- Fix spring autoconfiguration warning
Marijn van Zelst
Replaced dropwizard metrics with micrometer in metrics module
- Wait for spring metrics autoconfiguration to complete
Marijn van Zelst
Replaced dropwizard metrics with micrometer in metrics module
- Add percentiles to timer metrics
@abuijze

This comment has been minimized.

Copy link
Member

abuijze commented Dec 12, 2018

I like the feature (and it seems properly implemented as well), but I'd rather have this feature as an additional module, instead of a replacement to Dropwizard Metrics. There is still a usecase of Metrics, albeit not for Spring Boot 2.

@smcvb

This comment has been minimized.

Copy link
Member

smcvb commented Dec 13, 2018

That sounds like a fair idea..smart one.
@mvanzelst, would you be up to adding a new module, called axon-micrometer, into master to support micrometer as a means to get monitoring working?
The axon-metrics module is still a working piece of code, just not for the latest Spring version. It still is for those who for example use Dropwizard to create their applications.

@mvanzelst

This comment has been minimized.

Copy link
Author

mvanzelst commented Jan 3, 2019

Thanks for the feedback. I think that's doable. Let me give it a try.

Marijn van Zelst added some commits Jan 3, 2019

@mvanzelst

This comment has been minimized.

Copy link
Author

mvanzelst commented Jan 3, 2019

I've added micrometer metrics as a new module.

@abuijze abuijze changed the title Replace dropwizard metrics with micrometer in metrics module Introduce micrometer module Jan 3, 2019

@smcvb smcvb requested review from smcvb and abuijze and removed request for smcvb and abuijze Jan 4, 2019

@smcvb smcvb requested review from smcvb and m1l4n54v1c Jan 4, 2019

@smcvb smcvb added this to the Release 4.1 milestone Jan 4, 2019

@abuijze
Copy link
Member

abuijze left a comment

A few change-requests, but this is the direction that we'd like to go in.

Show resolved Hide resolved metrics-micrometer/pom.xml Outdated
Show resolved Hide resolved ...main/java/org/axonframework/metrics/micrometer/GlobalMetricRegistry.java Outdated
<version>${micrometer.version}</version>
</dependency>

<dependency>

This comment has been minimized.

@abuijze

abuijze Jan 8, 2019

Member

Is metrics still used as a dependency?

This comment has been minimized.

@mvanzelst

mvanzelst Jan 8, 2019

Author

Only as a test dependency. I've reused the approach of testing the GlobalMetricRegistry from the metrics module. Is it better to remove the test dependency?

Show resolved Hide resolved metrics-micrometer/pom.xml Outdated
Show resolved Hide resolved .../src/main/java/org/axonframework/metrics/micrometer/CapacityMonitor.java Outdated
@m1l4n54v1c
Copy link
Member

m1l4n54v1c left a comment

Looks like a good approach. A couple of undocumented methods. I do agree with Allard's suggestions.

Marijn van Zelst
Marijn van Zelst
- Added javadoc
- Added test for SlidingTimeWindowReservoir
@smcvb
Copy link
Member

smcvb left a comment

Couple of remarks

Show resolved Hide resolved ...a/org/axonframework/micrometer/reservoir/SlidingTimeWindowReservoir.java
"org.axonframework.micrometer.GlobalMetricRegistry"
})
@EnableConfigurationProperties(MetricsProperties.class)
public class MicrometerMetricsAutoConfiguration {

This comment has been minimized.

@smcvb

smcvb Jan 8, 2019

Member

Would you mind specifying that the old MetricsAutoConfiguration should happen after this guy? So with the @AutoConfigureAfter() annotation. I feel we should give precedence over the new approach set by Spring Boot in favor of the old one in the auto configuration set up.

This comment has been minimized.

@mvanzelst

mvanzelst Jan 14, 2019

Author

Ok I've done that. In the current setup both the old dropwizard metrics and the new micrometer metrics will be loaded simultaneously. Is that desirable?

This comment has been minimized.

@smcvb

smcvb Jan 14, 2019

Member

🤔 you're specifying it after the other one, so if anything, it's in order, not simultaneous, right?

Nonetheless, I'd expect that existence of the micrometer beans to ensure the Dropwizard-Metrics beans to not be created. This holds as there's a @ConditionalOnMissingBean on the bean creator functions, as far as I am aware.

If my assumptions are incorrect here, please call me out on them @mvanzelst!

This comment has been minimized.

@mvanzelst

mvanzelst Jan 17, 2019

Author

It's a tricky one 😄 I've modified the autoconfig class of the metrics module to NOT load if the micrometer module is loaded. Also I've added some tests to verify that if the metrics module beans are NOT loaded when the micrometer module is loaded. Would this be the best solution?

This comment has been minimized.

@smcvb

smcvb Jan 28, 2019

Member

I think we should take preference over Micrometer i.o. Dropwizard Metrics, so I'd say yes, this is the best solution. That's however my take on it. Maybe @abuijze / @m1l4n54v1c / @idugalic have a different perspective on this.

This comment has been minimized.

@idugalic

idugalic Jan 28, 2019

Member

Yes, I agree

Show resolved Hide resolved ...framework/springboot/AxonAutoConfigurationWithMicrometerMetricsTest.java Outdated
Show resolved Hide resolved metrics-micrometer/pom.xml Outdated

Marijn van Zelst added some commits Jan 14, 2019

Marijn van Zelst
Marijn van Zelst
- Added javadoc
- Auto configure micrometer metrics before dropwizard metrics
- Updated module names
Marijn van Zelst
- Don't load the MetricsAutoConfiguration bean when the MicrometerMet…
…ricsAutoConfiguration bean is already loaded

- Assert that the metric module beans are NOT loaded when importing both the micrometer and metrics module (they are both test dependencies of axon-spring-boot-autoconfigure)
@smcvb
Copy link
Member

smcvb left a comment

I've just now noticed there are quite some copyright things incorrect in this PR.
After that's been fixed, I'd say this is good to go.

Show resolved Hide resolved ...ter/src/main/java/org/axonframework/micrometer/GlobalMetricRegistry.java Outdated
Show resolved Hide resolved ...ter/src/main/java/org/axonframework/micrometer/GlobalMetricRegistry.java Outdated
Show resolved Hide resolved .../java/org/axonframework/micrometer/EventProcessorLatencyMonitorTest.java Outdated
Show resolved Hide resolved ...eter/src/test/java/org/axonframework/micrometer/CapacityMonitorTest.java Outdated
Show resolved Hide resolved ...eter/src/main/java/org/axonframework/micrometer/MessageTimerMonitor.java Outdated
Show resolved Hide resolved ...src/test/java/org/axonframework/micrometer/GlobalMetricRegistryTest.java
Show resolved Hide resolved ...a/org/axonframework/micrometer/reservoir/SlidingTimeWindowReservoir.java
Show resolved Hide resolved ...c/test/java/org/axonframework/micrometer/MessageCountingMonitorTest.java
Show resolved Hide resolved ...g/axonframework/micrometer/reservoir/SlidingTimeWindowReservoirTest.java
"org.axonframework.micrometer.GlobalMetricRegistry"
})
@EnableConfigurationProperties(MetricsProperties.class)
public class MicrometerMetricsAutoConfiguration {

This comment has been minimized.

@smcvb

smcvb Jan 28, 2019

Member

I think we should take preference over Micrometer i.o. Dropwizard Metrics, so I'd say yes, this is the best solution. That's however my take on it. Maybe @abuijze / @m1l4n54v1c / @idugalic have a different perspective on this.

@smcvb

smcvb approved these changes Feb 4, 2019

@abuijze

abuijze approved these changes Feb 4, 2019

@smcvb smcvb merged commit 0e9bdcb into AxonFramework:master Feb 4, 2019

3 checks passed

Codacy/PR Quality Review Up to standards. A positive pull request.
Details
clahub All contributors have signed the Contributor License Agreement.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment