Use Micrometer metrics instead of in memory one #1477
Conversation
|
@marcingrzejszczak would it be worth using the Noop metrics reporter if |
|
Micrometer is optional in boot, it is pulled in with the actuator starter. |
|
|
||
| @Configuration | ||
| @ConditionalOnBean(MeterRegistry.class) | ||
| static class TraceMicrometerMetricsConfiguration { |
There was a problem hiding this comment.
This should be a toplevel auto configuration conditional on a micrometer class
that's already the case
@spencergibb I wasn't sure of that, initially I did exactly what you suggested (with class check) and then I removed it. I'll bring it back. |
|
is there a reason we aren't using this? if so, maybe we mention https://github.com/openzipkin/zipkin-reporter-java/tree/master/metrics-micrometer |
|
Most likely cause I didn't know that it's a thing |
marcingrzejszczak
changed the title
Codecov Report
@@ Coverage Diff @@
## master #1477 +/- ##
============================================
+ Coverage 58.87% 59.01% +0.13%
+ Complexity 817 816 -1
============================================
Files 154 154
Lines 4401 4406 +5
Branches 480 480
============================================
+ Hits 2591 2600 +9
+ Misses 1593 1591 -2
+ Partials 217 215 -2
Continue to review full report at Codecov.
|
| <artifactId>zipkin-reporter-metrics-micrometer</artifactId> | ||
| <exclusions> | ||
| <exclusion> | ||
| <groupId>io.micrometer</groupId> |
There was a problem hiding this comment.
@shakuzen reminds me.. we probably should have had this set to "provided" scope in upstream anyway
|
|
||
| @Configuration | ||
| @ConditionalOnMissingClass("io.micrometer.core.instrument.MeterRegistry") | ||
| static class TraceMetricsNoOpConfiguration { |
There was a problem hiding this comment.
should this be
| static class TraceMetricsNoOpConfiguration { | |
| static class TraceMetricsInMemoryConfiguration { |
if no-op is desired, that would be different and cheaper.
Since I'm no metrics expert I need feedback from such as experts as @shakuzen @adriancole @msmsimondean and the rest of the experts interested in this topic ;)
I've added an initial implementation that uses
counters andgauges from micrometer for metrics. I've passed some default values for metric names but I'm not sure if those are the best.This would actually start defaulting to the Micrometer version of a metric reporter since it's pretty much always on the classpath. If someone doesn't have a
meterRegistrybean, then the in memory option will be taken into account.I don't think we can have an option that there is no micrometer on the classpath since it comes with boot.
UPDATE: As @spencergibb mentioned, micrometer comes as an dependency with an actuator, however actuator doesn't always have to be there. As @adriancole mentioned there already is a zipkin micrometer reporter that we can reuse.