From 5fe709c234b5783588a52b4adfb87ad378889191 Mon Sep 17 00:00:00 2001 From: Adrian Cole Date: Thu, 22 Feb 2018 19:42:52 +0800 Subject: [PATCH] Defined http.route key and ensures server span name is preferred (#1924) Especially when using http route based span names, we have to be careful to not pick a bad name. In shared spans, for example, the UI prefers server-side's name of the service. However, we have no name priority logic, so it is arbitrary. This ensures the server's span name wins in a race. See https://github.com/openzipkin/zipkin/issues/1874 --- zipkin/src/main/java/zipkin/Span.java | 19 ++++++++---- zipkin/src/main/java/zipkin/TraceKeys.java | 34 +++++++++++++++------- zipkin/src/test/java/zipkin/SpanTest.java | 24 ++++++++++++++- 3 files changed, 59 insertions(+), 18 deletions(-) diff --git a/zipkin/src/main/java/zipkin/Span.java b/zipkin/src/main/java/zipkin/Span.java index 805bf9a6508..9677e023261 100644 --- a/zipkin/src/main/java/zipkin/Span.java +++ b/zipkin/src/main/java/zipkin/Span.java @@ -1,5 +1,5 @@ /** - * Copyright 2015-2017 The OpenZipkin Authors + * Copyright 2015-2018 The OpenZipkin Authors * * Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except * in compliance with the License. You may obtain a copy of the License at @@ -237,9 +237,6 @@ public Builder merge(Span that) { if (this.traceIdHigh == null || this.traceIdHigh == 0) { this.traceIdHigh = that.traceIdHigh; } - if (this.name == null || this.name.length() == 0 || this.name.equals("unknown")) { - this.name = that.name; - } if (this.id == null) { this.id = that.id; } @@ -248,20 +245,26 @@ public Builder merge(Span that) { } // When we move to span model 2, remove this code in favor of using Span.kind == CLIENT - boolean thisIsClientSpan = this.isClientSpan; - boolean thatIsClientSpan = false; + boolean thisIsClientSpan = isClientSpan, thatIsClientSpan = false, thatIsServerSpan = false; // This guards to ensure we don't add duplicate annotations or binary annotations on merge if (!that.annotations.isEmpty()) { boolean thisHadNoAnnotations = this.annotations == null; for (Annotation a : that.annotations) { if (a.value.equals(Constants.CLIENT_SEND)) thatIsClientSpan = true; + if (a.value.equals(Constants.SERVER_RECV)) thatIsServerSpan = true; if (thisHadNoAnnotations || !this.annotations.contains(a)) { addAnnotation(a); } } } + if (nameUnknown(this)) { + this.name = that.name; + } else if (thisIsClientSpan && thatIsServerSpan && !that.name.isEmpty()) { + this.name = that.name; // prefer the server's span name on collision + } + if (!that.binaryAnnotations.isEmpty()) { boolean thisHadNoBinaryAnnotations = this.binaryAnnotations == null; for (BinaryAnnotation a : that.binaryAnnotations) { @@ -519,4 +522,8 @@ Object readResolve() throws ObjectStreamException { } } } + + static boolean nameUnknown(Span.Builder span) { + return span.name == null || span.name.length() == 0 || span.name.equals("unknown"); + } } diff --git a/zipkin/src/main/java/zipkin/TraceKeys.java b/zipkin/src/main/java/zipkin/TraceKeys.java index dddec84fcc3..0723779240c 100644 --- a/zipkin/src/main/java/zipkin/TraceKeys.java +++ b/zipkin/src/main/java/zipkin/TraceKeys.java @@ -1,5 +1,5 @@ /** - * Copyright 2015-2017 The OpenZipkin Authors + * Copyright 2015-2018 The OpenZipkin Authors * * Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except * in compliance with the License. You may obtain a copy of the License at @@ -14,7 +14,7 @@ package zipkin; /** - * Well-known {@link BinaryAnnotation#key binary annotation keys}. + * Well-known {@link BinaryAnnotation#key binary annotation aka Tag keys}. * *

Overhead of adding Trace Data

* @@ -53,20 +53,32 @@ public final class TraceKeys { /** * The absolute http path, without any query parameters. Ex. "/objects/abcd-ff" * - * Used to filter against an http route, portably with zipkin v1. + *

Used as a filter or to clarify the request path for a given route. For example, the path for + * a route "/objects/:objectId" could be "/objects/abdc-ff". This does not limit cardinality like + * {@link #HTTP_ROUTE} can, so is not a good input to a span name. * - *

In zipkin v1, only equals filters are supported. Dropping query parameters makes the number - * of distinct URIs less. For example, one can query for the same resource, regardless of signing - * parameters encoded in the query line. This does not reduce cardinality to a HTTP single route. - * For example, it is common to express a route as an http URI template like - * /resource/{resource_id}. In systems where only equals queries are available, searching for - * http/path=/resource won't match if the actual request was /resource/abcd-ff. + *

The Zipkin query api only supports equals filters. Dropping query parameters makes the + * number of distinct URIs less. For example, one can query for the same resource, regardless of + * signing parameters encoded in the query line. Dropping query parameters also limits the + * security impact of this tag. * - *

Historical note: This was commonly expressed as "http.uri" in zipkin, eventhough it was most - * often just a path. + *

Historical note: This was commonly expressed as "http.uri" in zipkin, even though it was most */ public static final String HTTP_PATH = "http.path"; + /** + * The route which a request matched or "" (empty string) if routing is supported, but there was + * no match. Ex "/objects/{objectId}" + * + *

Often used as a span name when known, with empty routes coercing to "not_found" or + * "redirected" based on {@link #HTTP_STATUS_CODE}. + * + *

Unlike {@link #HTTP_PATH}, this value is fixed cardinality, so is a safe input to a span + * name function or a metrics dimension. Different formats are possible. For example, the + * following are all valid route templates: "/objects" "/objects/:objectId" "/objects/*" + */ + public static final String HTTP_ROUTE = "http.route"; + /** * The entire URL, including the scheme, host and query parameters if available. Ex. * "https://mybucket.s3.amazonaws.com/objects/abcd-ff?X-Amz-Algorithm=AWS4-HMAC-SHA256..." diff --git a/zipkin/src/test/java/zipkin/SpanTest.java b/zipkin/src/test/java/zipkin/SpanTest.java index edb1bb9decb..cb8c6c61aab 100644 --- a/zipkin/src/test/java/zipkin/SpanTest.java +++ b/zipkin/src/test/java/zipkin/SpanTest.java @@ -1,5 +1,5 @@ /** - * Copyright 2015-2017 The OpenZipkin Authors + * Copyright 2015-2018 The OpenZipkin Authors * * Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except * in compliance with the License. You may obtain a copy of the License at @@ -126,6 +126,28 @@ public void mergeWhenBinaryAnnotationsSentSeparately() { assertThat(part2.toBuilder().merge(part1).build()).isEqualTo(expected); } + @Test + public void mergePrefersServerSpanName() { + Span clientPart = Span.builder() + .traceId(1L) + .name("get") + .id(1L) + .addAnnotation(Annotation.create(1L, CLIENT_SEND, APP_ENDPOINT)) + .build(); + + Span serverPart = Span.builder() + .traceId(1L) + .name("/users/:userid") + .id(1L) + .addAnnotation(Annotation.create(2L, SERVER_RECV, APP_ENDPOINT)) + .build(); + + assertThat(clientPart.toBuilder().merge(serverPart).build().name) + .isEqualTo("/users/:userid"); + assertThat(serverPart.toBuilder().merge(clientPart).build().name) + .isEqualTo("/users/:userid"); + } + /** * Test merging of client and server spans into a single span, with a clock skew. Final timestamp * and duration for the span should be same as client.