Skip to content

Commit

Permalink
Provide full request URL for "http.url" keyvalue
Browse files Browse the repository at this point in the history
Prior to this commit, the Observation filter for Servlet applications
would only use the request pathInfo as an "http.url" high cardinality
keyvalue. This commit ensures that we're using the full request URL as a
value there.

This also polishes gh-29254.

Fixes gh-29257
See gh-29254
  • Loading branch information
bclozel committed Oct 5, 2022
1 parent 7dd6afc commit 681cf0d
Show file tree
Hide file tree
Showing 5 changed files with 13 additions and 19 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ public class DefaultClientHttpObservationConvention implements ClientHttpObserva

private static final KeyValue EXCEPTION_NONE = KeyValue.of(ClientHttpObservation.LowCardinalityKeyNames.EXCEPTION, "none");

private static final KeyValue URI_EXPANDED_NONE = KeyValue.of(ClientHttpObservation.HighCardinalityKeyNames.HTTP_URL, "none");
private static final KeyValue HTTP_URL_NONE = KeyValue.of(ClientHttpObservation.HighCardinalityKeyNames.HTTP_URL, "none");

private static final KeyValue CLIENT_NAME_NONE = KeyValue.of(ClientHttpObservation.HighCardinalityKeyNames.CLIENT_NAME, "none");

Expand Down Expand Up @@ -141,7 +141,7 @@ protected KeyValue requestUri(ClientHttpObservationContext context) {
if (context.getCarrier() != null) {
return KeyValue.of(ClientHttpObservation.HighCardinalityKeyNames.HTTP_URL, context.getCarrier().getURI().toASCIIString());
}
return URI_EXPANDED_NONE;
return HTTP_URL_NONE;
}

protected KeyValue clientName(ClientHttpObservationContext context) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ public class DefaultHttpRequestsObservationConvention implements HttpRequestsObs

private static final KeyValue EXCEPTION_NONE = KeyValue.of(HttpRequestsObservation.LowCardinalityKeyNames.EXCEPTION, "none");

private static final KeyValue URI_EXPANDED_UNKNOWN = KeyValue.of(HttpRequestsObservation.HighCardinalityKeyNames.HTTP_URL, "UNKNOWN");
private static final KeyValue HTTP_URL_UNKNOWN = KeyValue.of(HttpRequestsObservation.HighCardinalityKeyNames.HTTP_URL, "UNKNOWN");

private final String name;

Expand Down Expand Up @@ -138,10 +138,9 @@ protected KeyValue outcome(HttpRequestsObservationContext context) {

protected KeyValue uriExpanded(HttpRequestsObservationContext context) {
if (context.getCarrier() != null) {
String uriExpanded = (context.getCarrier().getPathInfo() != null) ? context.getCarrier().getPathInfo() : "/";
return KeyValue.of(HttpRequestsObservation.HighCardinalityKeyNames.HTTP_URL, uriExpanded);
return KeyValue.of(HttpRequestsObservation.HighCardinalityKeyNames.HTTP_URL, context.getCarrier().getRequestURI());
}
return URI_EXPANDED_UNKNOWN;
return HTTP_URL_UNKNOWN;
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ public class DefaultHttpRequestsObservationConvention implements HttpRequestsObs

private static final KeyValue EXCEPTION_NONE = KeyValue.of(HttpRequestsObservation.LowCardinalityKeyNames.EXCEPTION, "none");

private static final KeyValue URI_EXPANDED_UNKNOWN = KeyValue.of(HttpRequestsObservation.HighCardinalityKeyNames.HTTP_URL, "UNKNOWN");
private static final KeyValue HTTP_URL_UNKNOWN = KeyValue.of(HttpRequestsObservation.HighCardinalityKeyNames.HTTP_URL, "UNKNOWN");

private final String name;

Expand Down Expand Up @@ -85,7 +85,7 @@ public KeyValues getLowCardinalityKeyValues(HttpRequestsObservationContext conte

@Override
public KeyValues getHighCardinalityKeyValues(HttpRequestsObservationContext context) {
return KeyValues.of(uriExpanded(context));
return KeyValues.of(httpUrl(context));
}

protected KeyValue method(HttpRequestsObservationContext context) {
Expand Down Expand Up @@ -143,12 +143,12 @@ protected KeyValue outcome(HttpRequestsObservationContext context) {
return HttpOutcome.UNKNOWN.asKeyValue();
}

protected KeyValue uriExpanded(HttpRequestsObservationContext context) {
protected KeyValue httpUrl(HttpRequestsObservationContext context) {
if (context.getCarrier() != null) {
String uriExpanded = context.getCarrier().getPath().toString();
return KeyValue.of(HttpRequestsObservation.HighCardinalityKeyNames.HTTP_URL, uriExpanded);
}
return URI_EXPANDED_UNKNOWN;
return HTTP_URL_UNKNOWN;
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,6 @@ void supportsOnlyHttpRequestsObservationContext() {
void addsKeyValuesForExchange() {
this.request.setMethod("POST");
this.request.setRequestURI("/test/resource");
this.request.setPathInfo("/test/resource");

assertThat(this.convention.getLowCardinalityKeyValues(this.context)).hasSize(5)
.contains(KeyValue.of("method", "POST"), KeyValue.of("uri", "UNKNOWN"), KeyValue.of("status", "200"),
Expand All @@ -72,7 +71,6 @@ void addsKeyValuesForExchange() {
@Test
void addsKeyValuesForExchangeWithPathPattern() {
this.request.setRequestURI("/test/resource");
this.request.setPathInfo("/test/resource");
this.context.setPathPattern("/test/{name}");

assertThat(this.convention.getLowCardinalityKeyValues(this.context)).hasSize(5)
Expand All @@ -85,7 +83,6 @@ void addsKeyValuesForExchangeWithPathPattern() {
@Test
void addsKeyValuesForErrorExchange() {
this.request.setRequestURI("/test/resource");
this.request.setPathInfo("/test/resource");
this.context.setError(new IllegalArgumentException("custom error"));
this.response.setStatus(500);

Expand All @@ -99,7 +96,6 @@ void addsKeyValuesForErrorExchange() {
@Test
void addsKeyValuesForRedirectExchange() {
this.request.setRequestURI("/test/redirect");
this.request.setPathInfo("/test/redirect");
this.response.setStatus(302);
this.response.addHeader("Location", "https://example.org/other");

Expand All @@ -113,7 +109,6 @@ void addsKeyValuesForRedirectExchange() {
@Test
void addsKeyValuesForNotFoundExchange() {
this.request.setRequestURI("/test/notFound");
this.request.setPathInfo("/test/notFound");
this.response.setStatus(404);

assertThat(this.convention.getLowCardinalityKeyValues(this.context)).hasSize(5)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ public class DefaultClientObservationConvention implements ClientObservationConv

private static final KeyValue EXCEPTION_NONE = KeyValue.of(ClientObservation.LowCardinalityKeyNames.EXCEPTION, "none");

private static final KeyValue URI_EXPANDED_NONE = KeyValue.of(ClientHttpObservation.HighCardinalityKeyNames.HTTP_URL, "none");
private static final KeyValue HTTP_URL_NONE = KeyValue.of(ClientHttpObservation.HighCardinalityKeyNames.HTTP_URL, "none");

private static final KeyValue CLIENT_NAME_NONE = KeyValue.of(ClientHttpObservation.HighCardinalityKeyNames.CLIENT_NAME, "none");

Expand Down Expand Up @@ -135,14 +135,14 @@ protected KeyValue outcome(ClientObservationContext context) {

@Override
public KeyValues getHighCardinalityKeyValues(ClientObservationContext context) {
return KeyValues.of(uriExpanded(context), clientName(context));
return KeyValues.of(httpUrl(context), clientName(context));
}

protected KeyValue uriExpanded(ClientObservationContext context) {
protected KeyValue httpUrl(ClientObservationContext context) {
if (context.getCarrier() != null) {
return KeyValue.of(ClientObservation.HighCardinalityKeyNames.HTTP_URL, context.getCarrier().url().toASCIIString());
}
return URI_EXPANDED_NONE;
return HTTP_URL_NONE;
}

protected KeyValue clientName(ClientObservationContext context) {
Expand Down

0 comments on commit 681cf0d

Please sign in to comment.