-
Notifications
You must be signed in to change notification settings - Fork 40.2k
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
Broken content negotiation for OpenMetrics #28130
Comments
This seems like a hack and Producible seems weird since it is both for consuming and producing. The getProducedMimeType is used for consuming first. fixes spring-projectsgh-28130
This seems like a hack and Producible seems weird. fixes spring-projectsgh-28130
I might have a
|
Thanks, @jonatan-ivanov. Your test is failing partly because it's parsing multiple media types into one: MediaType.parseMediaType("application/openmetrics-text; version=0.0.1,text/plain;version=0.0.4;q=0.5,*/*;q=0.1"); It's also using A test that uses a single media type and the "correct" version passes: @WebEndpointTest
void scrapeWithSingleAcceptibleMediaTypeCanProduceOpenMetrics100(WebTestClient client) {
MediaType openMetrics = MediaType.parseMediaType("application/openmetrics-text;version=1.0.0");
client.get().uri("/actuator/prometheus").accept(openMetrics).exchange().expectStatus().isOk().expectHeader()
.contentType(TextFormat.CONTENT_TYPE_OPENMETRICS_100).expectBody(String.class)
.value((body) -> assertThat(body).contains("counter1_total").contains("counter2_total")
.contains("counter3_total"));
} However, this test which uses multiple media types parsed individually fails: @WebEndpointTest
void scrapeWithMultipleAcceptibleMediaTypesCanProduceOpenMetrics100(WebTestClient client) {
client.get().uri("/actuator/prometheus")
.accept(MediaType.parseMediaType("application/openmetrics-text;version=1.0.0"),
MediaType.parseMediaType("text/plain;version=0.0.4;q=0.5"),
MediaType.parseMediaType("*/*;q=0.1"))
.exchange().expectStatus().isOk().expectHeader().contentType(TextFormat.CONTENT_TYPE_OPENMETRICS_100)
.expectBody(String.class).value((body) -> assertThat(body).contains("counter1_total")
.contains("counter2_total").contains("counter3_total"));
} It results in a request being sent with a single accept header that has a comma-separated value:
Actuator handles this incorrectly, treating it as a single value. I've opened #28137 to fix it. Let's wait and see what the Prometheus Java Client say about the version. I suspect that aligning with the server will be the right thing to do for this part of the problem. |
I was wrong above. Actuator correctly handles a single header with multiple comma-separated values. The problem is that we punted on proper quality support when we implemented #25738. We can dodge that limitation by reordering values in |
I agree that the Prometheus client and server version constants should match (I have no idea why they don't). If Prometheus fixes this on the server side (both client and server will use What do you think about delegating handling this issue to the prometheus client? |
Thanks for the suggestion, but I'm not sure that ignoring the version is a good idea. It feels like replacing one bug that was waiting to happen with another one. If we ignore the version and |
I totally agree with you, on the other hand I expect the prometheus client (
My assumption is that once the version will be important either to I'm not sure if you want to depend on such an assumption but if it seems fine, we could do something like this: @ReadOperation
public WebEndpointResponse<String> scrape(@RequestHeader(value = ACCEPT) String acceptHeader, @Nullable Set<String> includedNames) {
try {
Enumeration<MetricFamilySamples> samples = (includedNames != null)
? this.collectorRegistry.filteredMetricFamilySamples(includedNames)
: this.collectorRegistry.metricFamilySamples();
Writer writer = new StringWriter();
String contentType = TextFormat.chooseContentType(acceptHeader);
TextFormat.writeFormat(contentType, writer, samples);
return new WebEndpointResponse<>(writer.toString(), contentType);
}
catch (IOException ex) {
// This actually never happens since StringWriter doesn't throw an IOException
throw new IllegalStateException("Writing metrics failed", ex);
}
} |
We're going to align with |
This is easier said than done. Our current We either need to enhance |
Add an `isDefault()` method to `Producible` which can be used to indicate which of the enum values should be used when the accept header is `*/*` or `null`. Prior to this commit, the last enum value was always used as the default. See gh-28130
Update Prometheus `TextOutputFormat` so that OpenMetrics is used in preference to text output when an appropriate accept header is found. If the accept header contains `*/*` or is missing then the text format will be used. See gh-28130
It seems
PrometheusScrapeEndpoint
is unable to produce content in the OpenMetrics format.The
latest
(2.30.0) Prometheus server sends the following headers:In the
Accept
header, the version ofopenmetrics-text
is0.0.1
.In the Java client, the
Content-Type
constant does not match to this, its version is1.0.0
and it also has acharset
(see
TextFormat
):Since
PrometheusScrapeEndpoint
usesTextOutputFormat
that uses the aboveTextFormat
@WebEndpoint
seems to take the version (and seemingly other fields) into considerationThe
PrometheusScrapeEndpoint
never returns anything in the OpenMetrics format, it always falls back totext/plain
.I wrote a test which demonstrates the issue: jonatan-ivanov@d5e6b36
Also opened an issue for the Prometheus Java client to fix the version mismatch: prometheus/client_java#702
A couple things to call out:
1.0.0
instead of0.0.1
thePrometheusScrapeEndpoint
is still broken(because of
charset
)0.0.1
instead of1.0.0
thePrometheusScrapeEndpoint
is still broken(because of
charset
)TextFormat::chooseContentType
takes this into accountThe text was updated successfully, but these errors were encountered: