Add support for Prometheus Client 1.x and simpleclient #40023
Conversation
shakuzen
changed the title
spring-projects-issues
added
the
status: waiting-for-triage
Deprecates the support for simpleclient but ensures that it can work in conjunction with support for the latest Prometheus client auto-configuration. This involves breaking changes to update public classes to support the latest Prometheus client. Deprecated support for Prometheus simpleclient is provided in renamed classes.
shakuzen
force-pushed
the
prom1-auto-config
branch
from
6d2f7b7 to
448b6ea
Compare
|
Thanks for the PR Tommy! If I build this locally and create a boot application using it, using these dependencies: dependencies {
implementation 'org.springframework.boot:spring-boot-starter-actuator'
implementation 'org.springframework.boot:spring-boot-starter-web'
runtimeOnly 'io.micrometer:micrometer-registry-prometheus'
}it fails on startup with I think this is because of private io.micrometer.prometheus.HistogramFlavor histogramFlavor = io.micrometer.prometheus.HistogramFlavor.Prometheus;in Or is it supposed to only work if I have both |
|
If I include both dependencies and do a I think this is because of the auto-configured |
|
It works if i only use |
This error is because of some new validation logic in the new Prometheus client. :( |
I think we need to get it to work with only one or the other but I'm not yet sure how to do that. My initial thoughts are in #39904 which we can hopefully close as superseded by this PR. |
The intention was for it to work with either alone and with both. However, I apparently did not add a test that catches the issue you found. Is there an easy way to run the Two possible solutions for this:
What do you think? As for the jvm_info metric issue, if Boot wants to get this PR into M3, maybe temporarily disabling binding the JvmInfoMetrics is the best solution until RC1 when we should have that fixed on the Micrometer side (see micrometer-metrics/micrometer#4866). |
I think it's too late for that unfortunately. I'd rather give ourselves a bit more time and fix this in RC1. |
| void prometheus() throws Exception { | ||
| this.mockMvc.perform(get("/actuator/prometheus")) | ||
| .andExpect(status().isOk()) | ||
| .andDo(document("prometheus-simpleclient/all")); |
There was a problem hiding this comment.
I moved the documentation snippets for the simpleclient version to prometheus-simpleclient/ but I did not update the documentation itself.
| .withUserConfiguration(BaseConfiguration.class) | ||
| .withPropertyValues("management.endpoints.web.exposure.include=prometheus") | ||
| .run((context) -> assertThat(context).hasSingleBean(PrometheusScrapeEndpoint.class) | ||
| .doesNotHaveBean(PrometheusSimpleclientScrapeEndpoint.class)); |
There was a problem hiding this comment.
This test class is to demonstrate the case when both io.micrometer:micrometer-registry-prometheus and io.micrometer:micrometer-registry-prometheus-simpleclient are on the classpath. Mostly beans from both will be created, but there should only be one scrape endpoint and the one using io.micrometer:micrometer-registry-prometheus takes precedence.
| AutoConfigurations.of(PrometheusExemplarsAutoConfiguration.class, ObservationAutoConfiguration.class, | ||
| BraveAutoConfiguration.class, MicrometerTracingAutoConfiguration.class)); | ||
| .with(MetricsRun.limitedTo()) | ||
| .withConfiguration(AutoConfigurations.of(PrometheusSimpleclientMetricsExportAutoConfiguration.class, |
There was a problem hiding this comment.
This was done to avoid adding the deprecated PrometheusSimpleclientMetricsExportAutoConfiguration to MetricsRun. When exemplar support is fixed for the latest Prometheus client (planned to be in time for Micrometer 1.13.0-RC1), this can be updated to use PrometheusMetricsExportAutoConfiguration. But if we want to continue testing exemplar support with the simpleclient module, this test class will need to be duplicated with this same setup.
| /** | ||
| * Prometheus metrics protobuf. | ||
| */ | ||
| CONTENT_TYPE_PROTOBUF(PrometheusProtobufWriter.CONTENT_TYPE) { |
There was a problem hiding this comment.
Protobuf is a new format supported by Prometheus client 1.x. I have not added tests for this format yet because I'm not sure what is the best way to assert contents of binary protobuf.
There was a problem hiding this comment.
I added a test for it that doesn't assert on the response body except that it is not empty.
| @@ -108,12 +108,12 @@ void scrapePrefersToProduceOpenMetrics100(WebTestClient client) { | |||
| @WebEndpointTest | |||
| void scrapeWithIncludedNames(WebTestClient client) { | |||
| client.get() | |||
| .uri("/actuator/prometheus?includedNames=counter1_total,counter2_total") | |||
| .uri("/actuator/prometheus?includedNames=counter1,counter2") | |||
There was a problem hiding this comment.
This is a difference between simpleclient and the latest 1.x versions. Even though the output will have counter1_total, calls to scrape should filter with the name without the suffix added by the Prometheus client.
| @@ -108,12 +108,12 @@ void scrapePrefersToProduceOpenMetrics100(WebTestClient client) { | |||
| @WebEndpointTest | |||
| void scrapeWithIncludedNames(WebTestClient client) { | |||
| client.get() | |||
| .uri("/actuator/prometheus?includedNames=counter1_total,counter2_total") | |||
| .uri("/actuator/prometheus?includedNames=counter1,counter2") | |||
There was a problem hiding this comment.
The implementations of scrape endpoints provided by the Prometheus client support a different query parameter format for filtering by name on the scrape endpoint as now documented at https://prometheus.github.io/client_java/exporters/filter/.
I don't know if that necessarily means Boot's endpoint should align with that, but it may be something to consider.
mhalbritter
added
type: enhancement
mhalbritter
changed the title
Restores support for exemplars with the latest micrometer-registry-prometheus snapshots, depending on changes in the Prometheus client 1.2.0. Also duplicates the ConfigurationProperties class PrometheusProperties to avoid a NoClassDefFoundError when only micrometer-registry-prometheus is on the classpath.
There was a problem hiding this comment.
I've updated the pull request to use the latest Micrometer snapshots and Prometheus client 1.2.0 (with newly available BOM). This also means we're able to support auto-configuration for exemplars similar to before; I've made the necessary changes for that. I also ended up duplicating PrometheusProperties to have a dedicated class for the simpleclient support, as I've done for other classes. This should avoid the NCDF error that Moritz found but I still don't think there is a test that would necessarily catch it. The JVM info issue should also be fixed with the latest Micrometer snapshots.
| @@ -70,14 +55,6 @@ public void setDescriptions(boolean descriptions) { | |||
| this.descriptions = descriptions; | |||
| } | |||
|
|
|||
| public io.micrometer.prometheus.HistogramFlavor getHistogramFlavor() { | |||
There was a problem hiding this comment.
I removed this from here to avoid the NoClassDefFoundError Moritz found before. I've duplicated the class similar to other classes so there is a simpleclient version now that is used by the corresponding autoconfig. I also removed the Pushgateway related properties until the latest versions of the Prometheus client support it and we can add back auto-configuration support for it here.
There was a problem hiding this comment.
I reverted the change duplicating the ConfigurationProperties class and instead made a HistogramFlavor enum here to avoid requiring the micrometer-registry-prometheus-simpleclient module. The config adapter handles mapping to the Micrometer enum.
| }); | ||
| } | ||
|
|
||
| @Test | ||
| void prometheusOpenMetricsOutputCanBeConfiguredToContainExemplarsOnHistogramCount() { | ||
| this.contextRunner.withSystemProperties("io.prometheus.exporter.exemplarsOnAllMetricTypes=true") |
There was a problem hiding this comment.
The Prometheus client does not add exemplars to the count of a histogram by default. This configuration is needed to restore that behavior that Micrometer had previously. We are in discussion about a way to expose this configuration via Micrometer. You can see more about it in the Prometheus client documentation: https://prometheus.github.io/client_java/config/config/
There was a problem hiding this comment.
Since micrometer-metrics/micrometer#4921 and the corresponding updates I made to this PR in 63a3d2a things are back to having exemplars on the histogram count by default (when using Micrometer). Configuration can be done via Micrometer's PrometheusConfig and consequently Boot's PrometheusProperties now.
|
Hey Tommy, thanks for the update. When using this in a Boot app, it now fails with: I think the problem is @Bean
@ConditionalOnMissingBean
public PrometheusMeterRegistry prometheusMeterRegistry(PrometheusConfig prometheusConfig,
PrometheusRegistry prometheusRegistry, Clock clock, ObjectProvider<SpanContext> spanContext) {The |
|
And we have to do something for the duplicated property classes. Right now, the metadata JSON has this: {
"name": "management.prometheus.metrics.export.enabled",
"type": "java.lang.Boolean",
"description": "Whether exporting of metrics to this backend is enabled.",
"sourceType": "org.springframework.boot.actuate.autoconfigure.metrics.export.prometheus.PrometheusProperties",
"defaultValue": true
},which is the new one. But it also has this: {
"name": "management.prometheus.metrics.export.enabled",
"type": "java.lang.Boolean",
"description": "Whether exporting of metrics to this backend is enabled.",
"sourceType": "org.springframework.boot.actuate.autoconfigure.metrics.export.prometheus.PrometheusSimpleclientProperties",
"defaultValue": true,
"deprecated": true,
"deprecation": {}
},which is the deprecated old one. In IntelliJ, it looks like this:
which is weird. Can we get away with not duplicating the classes, but essentially duplicating the |
Deprecates the support for simpleclient but ensures that it can work in conjunction with support for the latest Prometheus client auto-configuration. This involves breaking changes to update public classes to support the latest Prometheus client. Deprecated support for Prometheus simpleclient is provided in renamed classes. See gh-40023
|
Thanks a lot Tommy! |
mhalbritter
removed
the
status: blocked
mhalbritter
changed the title
Deprecates the support for Prometheus simpleclient but ensures that it can work in conjunction with support for the latest Prometheus client auto-configuration. That is, if both are on the classpath, both will be auto-configured, except for the PrometheusScrapeEndpoint where the one that depends on the latest Prometheus client will take precedence over the simpleclient version. If a user wants to override this behavior, they can declare a bean of
PrometheusSimpleclientScrapeEndpointand it will be configured (seePrometheusSimpleclientScrapeEndpointIntegrationTests). They can also have both endpoints but will need to choose a different ID for one of them by creating a custom class annotated with e.g.@WebEndpoint(id = "customprometheus")and extend the corresponding endpoint class. See an example of this inSecondCustomPrometheusScrapeEndpointIntegrationTests.Pushgateway support is not currently available in the latest Prometheus client, so auto-configuration support for it is only available with simpleclient.