Skip to content

Commit 7e7324e

Browse files
author
Adrian Cole
committed
Renames to http.route
1 parent ebba5f1 commit 7e7324e

File tree

5 files changed

+22
-16
lines changed

5 files changed

+22
-16
lines changed

instrumentation/http-tests/src/main/java/brave/http/ITHttpServer.java

+4-4
Original file line numberDiff line numberDiff line change
@@ -218,7 +218,7 @@ public <Req> void request(HttpAdapter<Req, ?> adapter, Req req, SpanCustomizer c
218218

219219
/**
220220
* The "/items/{itemId}" endpoint should return the itemId in the response body, which proves
221-
* templating worked (including that it ignores query parameters). Note the template format is
221+
* templating worked (including that it ignores query parameters). Note the route format is
222222
* framework specific, ex "/items/:itemId" in vert.x
223223
*/
224224
@Test
@@ -233,13 +233,13 @@ public <Req> void request(HttpAdapter<Req, ?> adapter, Req req, SpanCustomizer c
233233
@Override public <Resp> void response(HttpAdapter<?, Resp> adapter, Resp res, Throwable error,
234234
SpanCustomizer customizer) {
235235
super.response(adapter, res, error, customizer);
236-
String template = adapter.template(res);
236+
String template = adapter.route(res);
237237
if (template != null) customizer.name(template);
238238
}
239239
}).build();
240240
init();
241241

242-
// Reading the template parameter from the response ensures the test endpoint is correct
242+
// Reading the route parameter from the response ensures the test endpoint is correct
243243
assertThat(get("/items/1?foo").body().string())
244244
.isEqualTo("1");
245245
assertThat(get("/items/2?bar").body().string())
@@ -255,7 +255,7 @@ public <Req> void request(HttpAdapter<Req, ?> adapter, Req req, SpanCustomizer c
255255
.containsEntry("http.path", "/items/2")
256256
.containsEntry("http.url", url("/items/2?bar"));
257257

258-
// We don't know the exact format of the http template as it is framework specific
258+
// We don't know the exact format of the http route as it is framework specific
259259
// However, we know that it should match both requests and include the common part of the path
260260
Set<String> templates = new LinkedHashSet<>(Arrays.asList(span1.name(), span2.name()));
261261
assertThat(templates).hasSize(1);

instrumentation/http/src/main/java/brave/http/HttpAdapter.java

+13-7
Original file line numberDiff line numberDiff line change
@@ -38,22 +38,28 @@ public abstract class HttpAdapter<Req, Resp> {
3838
@Nullable public abstract String requestHeader(Req request, String name);
3939

4040
/**
41-
* An expression representing an application endpoint, used to group similar requests together.
41+
* An expression such as "/items/:itemId" representing an application endpoint, conventionally
42+
* associated with the tag key "http.route".
4243
*
43-
* <p>For example, the template "/products/{key}", would match "/products/1" and "/products/2".
44-
* There is no format required for the encoding, as it is sometimes application defined. The
45-
* important part is that the value namespace is low cardinality.
44+
* <p>The http route groups similar requests together, so results in limited cardinality, often
45+
* better choice for a span name vs the http method. However, the route can be absent on redirect
46+
* or file-not-found. Also, not all frameworks include support for http route expressions,
47+
* and some don't expose templates programmatically for readback.
4648
*
47-
* <p>Conventionally associated with the key "http.template"
49+
* <p>For example, the route "/users/{userId}", matches "/users/25f4c31d" and "/users/e3c553be".
50+
* If a span name function used the http path instead, it could DOS-style attack vector on your
51+
* span name index, as it would grow unbounded vs "/users/{userId}". Even if different frameworks
52+
* use different formats, like "/users/[0-9a-f]+" or "/users/:userId", the cardinality is still
53+
* fixed with regards to request count.
4854
*
49-
* <p>Eventhough the template is associated with the request, not the response, this is present
55+
* <p>Eventhough the route is associated with the request, not the response, this is present
5056
* on the response object. The reasons is that many server implementations process the request
5157
* before they can identify the route route.
5258
*/
5359
// BRAVE5: It isn't possible for a user to easily consume HttpServerAdapter, which is why this
5460
// method, while generally about the server, is pushed up to the HttpAdapter. The signatures for
5561
// sampling and parsing could be changed to make it more convenient.
56-
@Nullable public String template(Resp response) {
62+
@Nullable public String route(Resp response) {
5763
return null;
5864
}
5965

instrumentation/jaxrs2/src/main/java/brave/jaxrs2/ContainerAdapter.java

+2-2
Original file line numberDiff line numberDiff line change
@@ -38,8 +38,8 @@ public final class ContainerAdapter
3838
return request.getHeaderString(name);
3939
}
4040

41-
@Override public String template(ContainerResponseContext response) {
42-
// There's no portable means to get the template eventhough there is a way in jersey2
41+
@Override public String route(ContainerResponseContext response) {
42+
// There's no portable means to get the route eventhough there is a way in jersey2
4343
return null;
4444
}
4545

instrumentation/spring-webmvc/src/main/java/brave/spring/webmvc/TracingHandlerInterceptor.java

+1-1
Original file line numberDiff line numberDiff line change
@@ -47,7 +47,7 @@ public static HandlerInterceptor create(HttpTracing httpTracing) {
4747
@Autowired TracingHandlerInterceptor(HttpTracing httpTracing) { // internal
4848
tracer = httpTracing.tracing().tracer();
4949
handler = HttpServerHandler.create(httpTracing, new HttpServletAdapter() {
50-
@Override public String template(HttpServletResponse response) {
50+
@Override public String route(HttpServletResponse response) {
5151
return response instanceof HttpServletResponseWithTemplate
5252
? ((HttpServletResponseWithTemplate) response).template : null;
5353
}

instrumentation/vertx-web/src/main/java/brave/vertx/web/TracingRoutingContextHandler.java

+2-2
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,7 @@
2424
* <p>The hint that we need to re-attach the headers handler on re-route came from looking at
2525
* {@code TracingHandler} in https://github.com/opentracing-contrib/java-vertx-web
2626
*
27-
* <h3>Why use a thread local for the http template when parsing {@linkplain HttpServerResponse}?</h3>
27+
* <h3>Why use a thread local for the http route when parsing {@linkplain HttpServerResponse}?</h3>
2828
* <p>When parsing the response, we use a thread local to make the current route's path visible.
2929
* This is an alternative to wrapping {@linkplain HttpServerResponse} or declaring a custom type.
3030
* We don't wrap {@linkplain HttpServerResponse}, because this would lock the instrumentation to the
@@ -130,7 +130,7 @@ static final class Adapter extends HttpServerAdapter<HttpServerRequest, HttpServ
130130
return request.headers().get(name);
131131
}
132132

133-
@Override public String template(HttpServerResponse response) {
133+
@Override public String route(HttpServerResponse response) {
134134
return currentTemplate.get();
135135
}
136136

0 commit comments

Comments
 (0)