Skip to content
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.

Sign up for GitHub

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

Add HTTP response to HttpServerMetricsTagsContributor.Context #45744

Merged
merged 2 commits into from
Jan 21, 2025
Merged

Add HTTP response to HttpServerMetricsTagsContributor.Context #45744

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9ac63d5
Add HTTP response to HttpServerMetricsTagsContributor.Context
geoand Jan 21, 2025
bd0b5e1
Add HTTP response to HttpClientMetricsTagsContributor.Context
geoand Jan 21, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion docs/src/main/asciidoc/telemetry-micrometer.adoc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -588,7 +588,9 @@ link:https://micrometer.io/docs/concepts[official documentation].

=== Use `HttpServerMetricsTagsContributor` for server HTTP requests

By providing CDI beans that implement `io.quarkus.micrometer.runtime.HttpServerMetricsTagsContributor`, user code can contribute arbitrary tags based on the details of HTTP request
By providing CDI beans that implement `io.quarkus.micrometer.runtime.HttpServerMetricsTagsContributor`, user code can contribute arbitrary tags based on the details of HTTP request and responses.

CAUTION: When creating tags using this interface, it's important to limit the cardinality of the values, otherwise there is a risk of severely degrading the metrics system's capacity.

=== Use `HttpClientMetricsTagsContributor` for client HTTP requests

Expand Down
3 changes: 3 additions & 0 deletions ...runtime/src/main/java/io/quarkus/micrometer/runtime/HttpClientMetricsTagsContributor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
import io.micrometer.core.instrument.Tags;
import io.micrometer.core.instrument.config.MeterFilter;
import io.vertx.core.spi.observability.HttpRequest;
import io.vertx.core.spi.observability.HttpResponse;

/**
* Allows code to add additional Micrometer {@link Tags} to the metrics collected for completed HTTP client requests.
Expand All @@ -20,5 +21,7 @@ public interface HttpClientMetricsTagsContributor {

interface Context {
HttpRequest request();

HttpResponse response();
}
}
3 changes: 3 additions & 0 deletions ...runtime/src/main/java/io/quarkus/micrometer/runtime/HttpServerMetricsTagsContributor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
import io.micrometer.core.instrument.Tags;
import io.micrometer.core.instrument.config.MeterFilter;
import io.vertx.core.http.HttpServerRequest;
import io.vertx.core.spi.observability.HttpResponse;

/**
* Allows code to add additional Micrometer {@link Tags} to the metrics collected for completed HTTP server requests.
Expand All @@ -20,5 +21,7 @@ public interface HttpServerMetricsTagsContributor {

interface Context {
HttpServerRequest request();

HttpResponse response();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have serious doubts about this. I think we are giving users a way to shoot their feet.
This can explode the cardinality and it should be accompanied by a documentation warning.
What do you think @ebullient ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How would they shoot theirselves in the foot more than what we already give them?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Data from the request can also explode the cardinality, yet it is included. I've already seen how improper usage of request headers explode the cardinality.

People need to be careful how they use the data from the request and response, so I think a warning in the documention would be a good idea 🙂.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so I think a warning in the documention would be a good idea 🙂.

Definitely +1 for this.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to think in a broad case.
Server side will not have control over the response nor the different amount of values they are getting.
To be used safely, users should not just grab a field from the response but map it to a finite amount of values they control, otherwise there will be a cardinality explosion and in some cases OoM.
This is, most likely for debugging purposes and is the type of thing that should be placed on the request's trace, not metrics.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But if we document it, I'm fine.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Server side will not have control over the response

Could you clarify that? I would think you have more control on the response than on the request so I might be missing something?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR updated

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Server side will not have control over the response

Could you clarify that? I would think you have more control on the response than on the request so I might be missing something?

From the metrics system perspective there is not much control.

}
}
5 changes: 3 additions & 2 deletions ...time/src/main/java/io/quarkus/micrometer/runtime/binder/vertx/VertxHttpClientMetrics.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -172,7 +172,7 @@ public void responseEnd(RequestTracker tracker, long bytesRead) {
.and(HttpCommonTags.status(tracker.response.statusCode()))
.and(HttpCommonTags.outcome(tracker.response.statusCode()));
if (!httpClientMetricsTagsContributors.isEmpty()) {
HttpClientMetricsTagsContributor.Context context = new DefaultContext(tracker.request);
HttpClientMetricsTagsContributor.Context context = new DefaultContext(tracker.request, tracker.response);
for (int i = 0; i < httpClientMetricsTagsContributors.size(); i++) {
try {
Tags additionalTags = httpClientMetricsTagsContributors.get(i).contribute(context);
Expand Down Expand Up @@ -254,6 +254,7 @@ public String getNormalizedUriPath(Map<Pattern, String> serverMatchPatterns, Lis
}
}

private record DefaultContext(HttpRequest request) implements HttpClientMetricsTagsContributor.Context {
private record DefaultContext(HttpRequest request,
HttpResponse response) implements HttpClientMetricsTagsContributor.Context {
}
}
5 changes: 3 additions & 2 deletions ...time/src/main/java/io/quarkus/micrometer/runtime/binder/vertx/VertxHttpServerMetrics.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -205,7 +205,7 @@ public void responseEnd(HttpRequestMetric requestMetric, HttpResponse response,
VertxMetricsTags.outcome(response),
HttpCommonTags.status(response.statusCode()));
if (!httpServerMetricsTagsContributors.isEmpty()) {
HttpServerMetricsTagsContributor.Context context = new DefaultContext(requestMetric.request());
HttpServerMetricsTagsContributor.Context context = new DefaultContext(requestMetric.request(), response);
for (int i = 0; i < httpServerMetricsTagsContributors.size(); i++) {
try {
Tags additionalTags = httpServerMetricsTagsContributors.get(i).contribute(context);
Expand Down Expand Up @@ -258,6 +258,7 @@ public void disconnected(LongTaskTimer.Sample websocketMetric) {
}
}

private record DefaultContext(HttpServerRequest request) implements HttpServerMetricsTagsContributor.Context {
private record DefaultContext(HttpServerRequest request,
HttpResponse response) implements HttpServerMetricsTagsContributor.Context {
}
}
20 changes: 20 additions & 0 deletions integration-tests/micrometer-prometheus/src/main/java/io/quarkus/ResponseHeaderTag.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
package io.quarkus;

import jakarta.inject.Singleton;

import io.micrometer.core.instrument.Tags;
import io.quarkus.micrometer.runtime.HttpServerMetricsTagsContributor;

@Singleton
public class ResponseHeaderTag implements HttpServerMetricsTagsContributor {

@Override
public Tags contribute(Context context) {
var headerValue = context.response().headers().get("foo-response");
String value = "UNSET";
if ((headerValue != null) && !headerValue.isEmpty()) {
value = headerValue;
}
return Tags.of("foo-response", value);
}
}
8 changes: 7 additions & 1 deletion ...icrometer-prometheus/src/main/java/io/quarkus/it/micrometer/prometheus/FruitResource.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@
import jakarta.ws.rs.GET;
import jakarta.ws.rs.Path;

import org.jboss.resteasy.reactive.RestResponse;

import io.quarkus.hibernate.orm.panache.PanacheQuery;
import io.smallrye.common.annotation.Blocking;

Expand All @@ -26,9 +28,13 @@ public void trigger() {

@GET
@Path("all")
public void retrieveAll() {
public RestResponse<Object> retrieveAll() {
PanacheQuery<Fruit> query = Fruit.find(
"select name from Fruit");
List<Fruit> all = query.list();

return RestResponse.ResponseBuilder.noContent()
.header("foo-response", "value")
.build();
}
}
2 changes: 1 addition & 1 deletion ...micrometer-prometheus/src/test/java/io/quarkus/it/micrometer/prometheus/ExemplarTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ void testExemplar() {
when().get("/example/prime/7919").then().statusCode(200);

String metricMatch = "http_server_requests_seconds_count{dummy=\"value\",env=\"test\"," +
"env2=\"test\",foo=\"UNSET\",method=\"GET\",outcome=\"SUCCESS\"," +
"env2=\"test\",foo=\"UNSET\",foo_response=\"UNSET\",method=\"GET\",outcome=\"SUCCESS\"," +
"registry=\"prometheus\",status=\"200\",uri=\"/example/prime/{number}\"} 2.0 # {span_id=\"";

await().atMost(5, SECONDS).untilAsserted(() -> {
Expand Down
7 changes: 4 additions & 3 deletions ...heus/src/test/java/io/quarkus/it/micrometer/prometheus/PrometheusMetricsRegistryTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -134,14 +134,15 @@ void testPrometheusScrapeEndpointTextPlain() {
.body(containsString("outcome=\"SUCCESS\""))
.body(containsString("dummy=\"value\""))
.body(containsString("foo=\"bar\""))
.body(containsString("foo_response=\"value\""))
.body(containsString("uri=\"/message/match/{id}/{sub}\""))
.body(containsString("uri=\"/message/match/{other}\""))

.body(containsString(
"http_server_requests_seconds_count{dummy=\"value\",env=\"test\",env2=\"test\",foo=\"UNSET\",method=\"GET\",outcome=\"SUCCESS\",registry=\"prometheus\",status=\"200\",uri=\"/template/path/{value}\""))
"http_server_requests_seconds_count{dummy=\"value\",env=\"test\",env2=\"test\",foo=\"UNSET\",foo_response=\"UNSET\",method=\"GET\",outcome=\"SUCCESS\",registry=\"prometheus\",status=\"200\",uri=\"/template/path/{value}\""))

.body(containsString(
"http_server_requests_seconds_count{dummy=\"value\",env=\"test\",env2=\"test\",foo=\"UNSET\",method=\"GET\",outcome=\"SUCCESS\",registry=\"prometheus\",status=\"200\",uri=\"/root/{rootParam}/sub/{subParam}\""))
"http_server_requests_seconds_count{dummy=\"value\",env=\"test\",env2=\"test\",foo=\"UNSET\",foo_response=\"UNSET\",method=\"GET\",outcome=\"SUCCESS\",registry=\"prometheus\",status=\"200\",uri=\"/root/{rootParam}/sub/{subParam}\""))

// Verify Hibernate Metrics
.body(containsString(
Expand Down Expand Up @@ -233,7 +234,7 @@ void testPrometheusScrapeEndpointOpenMetrics() {
.body(containsString("uri=\"/message/match/{other}\""))

.body(containsString(
"http_server_requests_seconds_count{dummy=\"value\",env=\"test\",env2=\"test\",foo=\"UNSET\",method=\"GET\",outcome=\"SUCCESS\",registry=\"prometheus\",status=\"200\",uri=\"/template/path/{value}\""))
"http_server_requests_seconds_count{dummy=\"value\",env=\"test\",env2=\"test\",foo=\"UNSET\",foo_response=\"UNSET\",method=\"GET\",outcome=\"SUCCESS\",registry=\"prometheus\",status=\"200\",uri=\"/template/path/{value}\""))

// Verify Hibernate Metrics
.body(containsString(
Expand Down
Loading