Trailing comma is lost from prometheus metrics

[fedinskiy]

Describe the bug

In all released versions (including 2.15.3.Final), prometheus metrics (accessible at 'q/metrics') has format of json object with a trailing comma. In the current main branch (tested on dc85cd0) the format is changed, and the comma was lost. Is this an expected change? Prometheus format allows[1] for both options, but that change may broke parsing of logs and therefore it would be nice to have it documented

[1] https://prometheus.io/docs/prometheus/latest/querying/operators/#aggregation-operators

Expected behavior

Example:
http_server_requests_seconds_count{method="GET",outcome="SUCCESS",status="200",uri="/hello",} 1.0

Actual behavior

http_server_requests_seconds_count{method="GET",outcome="SUCCESS",status="200",uri="/hello"} 6.0

How to Reproduce?

1. mvn io.quarkus.platform:quarkus-maven-plugin:2.15.3.Final:create -DprojectGroupId=org.acme -DprojectArtifactId=micrometer-quickstart -Dextensions='resteasy-reactive,micrometer-registry-prometheus'
2. cd micrometer-quickstart
3. mvn quarkus:dev -Dquarkus.platform.version=999-SNAPSHOT
4. curl http://localhost:8080/hello/
5. curl http://localhost:8080/q/metrics | grep http_server_requests_seconds_count # example, many (all?) other metrics also changed

for comparision, replace step 3 with mvn quarkus:dev -Dquarkus.platform.version=2.15.3.Final

Output of uname -a or ver

6.0.18-300.fc37.x86_64

Output of java -version

17.0.4, vendor: GraalVM Community

GraalVM version (if different from Java)

No response

Quarkus version or git rev

dc85cd0

Build tool (ie. output of mvnw --version or gradlew --version)

Apache Maven 3.8.6 (84538c9988a25aec085021c365c560670ad80f63)

Additional information

No response

[quarkus-bot]

/cc @ebullient (metrics), @jmartisk (metrics)

[ebullient]

@jtama -- this is a side-effect of support for exemplars (#28360). The the default format for Prometheus changed to OpenMetrics, which does have some differences, most notably the trailng comments.

We'll need to add comments to the release notes for 2.16, and make sure they are noted for 2.15 and previous. We should also ensure that if an accept header for prometheus comes in, we honor that (we aren't currently checking for the accept header).

[jtama]

I agree it should be documented, just where should I add those pieces of information ?

[ebullient]

Let's work with @gsmet to get text where it needs to be for 2.16 release notes and potential 2.15 backport.

[gsmet]

Just add the text here and I will integrate it in the 2.15 migration guide.

[jtama]

We'll need to add comments to the release notes for 2.16, and make sure they are noted for 2.15 and previous. We should also ensure that if an accept header for prometheus comes in, we honor that (we aren't currently checking for the accept header).

I confirm whatever the accept header comes in, the PrometheusHandler returns a application/openmetrics-text; version=1.0.0; charset=utf-8"format response. @ebullient We should maybe create a separate issue for this one no ?

For @gsmet :

Starting from now on (replace with exact 2.15 and 2.16 versions 😀) the format for /q/metrics endpoint response switched from prometheus to the OpenMetrics's one. They do have some differences, most notably the trailng commas. You can check the full specification here.

Does it suits you ?

[ebullient]

We'll need to add comments to the release notes for 2.16, and make sure they are noted for 2.15 and previous. We should also ensure that if an accept header for prometheus comes in, we honor that (we aren't currently checking for the accept header).

I confirm whatever the accept header comes in, the PrometheusHandler returns a application/openmetrics-text; version=1.0.0; charset=utf-8"format response. @ebullient We should maybe create a separate issue for this one no ?

yes

[jtama]

Sorry for the delay.

@ebullient We have to choose the default MIME_TYPE for the /metrics endpoint. Either plain-text or application/openmetrics-text.

We have an issue though io.smallrye.metrics.MetricsRequestHandler#isKnownMediaType only accepts :

• text/plain
• application/json
• */*

So we have to either do a PR on small-rye or say that if we receive */* then the expected format is application/openmetrics-text (but I don't like this solution)

[ebullient]

Not sure why that matters. Smallrye Metrics makes its own assertions about format, and the Micrometer extension makes no pretensions about being "compliant"... it's a best-effort/don't-break-the-universe kind of arrangement.

We just need to look at the accept headers and do the right thing here:

quarkus/extensions/micrometer/runtime/src/main/java/io/quarkus/micrometer/runtime/export/handlers/PrometheusHandler.java

Line 36 in 1672206

ManagedContext requestContext = Arc.container().requestContext();

Specifically so that we call the right scrape variant here:

quarkus/extensions/micrometer/runtime/src/main/java/io/quarkus/micrometer/runtime/export/handlers/PrometheusHandler.java

Line 52 in 1672206

.end(Buffer.buffer(registry.scrape(TextFormat.CONTENT_TYPE_OPENMETRICS_100)));

[jtama]

Yes, I already have something working for this part. But the /q/metrics endpoint is deployed here (i think) :

quarkus/extensions/smallrye-metrics/runtime/src/main/java/io/quarkus/smallrye/metrics/runtime/SmallRyeMetricsRecorder.java

Line 84 in 4cef3b7

public SmallRyeMetricsHandler handler(String metricsPath) {

And it prevents the user to any other MIME Type than the ones described in the above comment.

[ebullient]

that only applies when the smallrye metrics extension is enabled... Micrometer registers its own, here:

quarkus/extensions/micrometer/deployment/src/main/java/io/quarkus/micrometer/deployment/export/PrometheusRegistryProcessor.java

Line 88 in 1672206

void createPrometheusRoute(BuildProducer<RouteBuildItem> routes,

(micrometer & smallrye-metrics extensions are mutually exclusive)

[jtama]

Ok so that's what trigger's a 406:

quarkus/extensions/micrometer/runtime/src/main/java/io/quarkus/micrometer/runtime/export/PrometheusRecorder.java

Line 28 in 1672206

route.order(1).produces("text/plain");

So I have something running, I can put up a PR.

[ebullient]

IIRC, the challenge was also having the json exporter (compat with MP) enabled.. and something w/ precedence for the route..

The test of interest...
https://github.com/quarkusio/quarkus/tree/main/integration-tests/micrometer-mp-metrics

[jtama]

Ok, the only question is : what should we default to: text/plain or application/openmetrics-text

[ebullient]

I've lost the thread: text/plain was before (with commas), application/openmetrics-text is the change (which removes the commas?) .. previously, there wasn't a choice, and so you always got the output for application/openmetrics-text regardless of what you sent in?

Update you suggested for docs (with accidental edits):

Starting from now on (replace with exact 2.15 and 2.16 versions 😀) the format for /q/metrics endpoint response has switched to application/openmetrics-text, which provides support for exemplars. This format does have some differences, most notably that trailing commas have been removed. You can check the full specification here.

• No Accept header: use application/openmetrics-text; version=1.0.0; charset=utf-8
• If we receive text/plain, return text/plain; version=0.0.4; charset=utf-8 (prometheus format)
• If we receive application/json, that should go to the json exporter if it is enabled, otherwise return a 406 (pretty sure that's the right one)