fix micrometer bridge auto configuration annotation #13083
Conversation
zeitlinger
changed the title
yes - I even found the test that I broke with this PR. |
zeitlinger
force-pushed
the
micrometer-bridge-auto-config
branch
from
a888249 to
b9a2ab1
Compare
| testing { | ||
| suites { | ||
| val testPrometheus by registering(JvmTestSuite::class) { | ||
| dependencies { | ||
| runtimeOnly("io.micrometer:micrometer-registry-prometheus:1.14.3") | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
what's the failure for this test without your change to OpenTelemetryMeterRegistry?
There was a problem hiding this comment.
just tried... there is no failure - which confirms that the error does not always appear
but I've improved the test
There was a problem hiding this comment.
what's the failure now (without your change to OpenTelemetryMeterRegistry)?
There was a problem hiding this comment.
@trask the test is still passing with the unchanged code
I don't know how to force spring to use a different resolution order from the provided constraints
There was a problem hiding this comment.
the test is still passing the old code - but I don't have an idea how to make a better test - you'd have to force spring to randomize the bean order within the provided constraints
| @AutoConfigureAfter(SimpleMetricsExportAutoConfiguration.class) | ||
| @AutoConfigureAfter({ | ||
| SimpleMetricsExportAutoConfiguration.class, | ||
| PrometheusMetricsExportAutoConfiguration.class |
There was a problem hiding this comment.
is there a way to do this generically for all other MeterRegistries? (not only simple and prometheus?)
There was a problem hiding this comment.
do you want to add more different configs that load different metric registries? sounds too much - @jeanbisutti what's your take?
There was a problem hiding this comment.
no, asking if there's a way to do it generically, so that it loads after all other MeterRegistries, without having to hard-code specific meter registries
There was a problem hiding this comment.
the problem is that each metric registry is typically in it's own library - see https://mvnrepository.com/artifact/io.micrometer
If you don't add any of those, you only have the simple meter registry.
There was a problem hiding this comment.
You could try approaching this differently. Instead of providing the otel MeterRegistry bean and relying on spring getting the order right you could call add(MicrometerSingletons.meterRegistry()) on the CompositeMeterRegistry bean.
There was a problem hiding this comment.
There are multiple ways we could try for solving this. We could make instruments from our registry readable. Though I suspect that what these instruments should return isn't well defined so what we return may be different from what the originally used registry might return. We could also override the find method in our registry so that it doesn't find anything. This should make the code in https://github.com/spring-projects/spring-boot/blob/3.2.x/spring-boot-project/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/metrics/MetricsEndpoint.java#L115-L122 proceed to searching from the next registry. Idk if this method is used for anything else. We could also try to enforce the order of registries with
@Bean
static BeanPostProcessor postProcessCompositeMeterRegistry() {
return new BeanPostProcessor() {
@Override
public Object postProcessAfterInitialization(Object bean, String beanName) {
if (bean instanceof CompositeMeterRegistry) {
CompositeMeterRegistry original = (CompositeMeterRegistry) bean;
List<MeterRegistry> list = new ArrayList<>(original.getRegistries());
// sort otel registry last
list.sort(
Comparator.comparingInt(
value -> value == MicrometerSingletons.meterRegistry() ? 1 : 0));
Set<MeterRegistry> registries = new LinkedHashSet<>(list);
return new CompositeMeterRegistry(
original.config().clock(), Collections.singletonList(original)) {
@Override
public Set<MeterRegistry> getRegistries() {
return registries;
}
};
}
return bean;
}
};
}so that our registry would be last.
There was a problem hiding this comment.
I hadn't thought about that possibility - but it sounds like this could work 😄
There was a problem hiding this comment.
@laurit do you want to give your solution a try?
There was a problem hiding this comment.
@zeitlinger I wasn't planning to do anything with this since you were already working on it.
There was a problem hiding this comment.
@laurit added your solution now
zeitlinger
force-pushed
the
micrometer-bridge-auto-config
branch
from
0f347b6 to
84abdcc
Compare
| testing { | ||
| suites { | ||
| val testPrometheus by registering(JvmTestSuite::class) { | ||
| dependencies { | ||
| runtimeOnly("io.micrometer:micrometer-registry-prometheus:1.14.3") | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
since there are no source for this suite it won't run any tests as far as I can tell
| try { | ||
| Class.forName("io.micrometer.prometheusmetrics.PrometheusMeterRegistry"); | ||
| match.anyMatch(r -> r.getClass().getSimpleName().equals("PrometheusMeterRegistry")); | ||
| } catch (ClassNotFoundException e) { | ||
| // not testing prometheus | ||
| } |
There was a problem hiding this comment.
when running this test the prometheus dependency is never available as far as I can tell. Consider whether you want to test the prometheus here at all. If you want then instead of adding a suite consider adding the prometheus dependency to the main test dependencies.
| assertThat(((CompositeMeterRegistry) meterRegistry).getRegistries()) | ||
| .anyMatch(r -> r.getClass().getSimpleName().equals("OpenTelemetryMeterRegistry")) | ||
| .anyMatch(r -> r.getClass().getSimpleName().equals("SimpleMeterRegistry")); |
There was a problem hiding this comment.
Considering that the issue is about ordering consider whether it would make sense to test that these registries appear in the right order.
Fixes #12719 - hopefully