From 84abd6b3eb4d6aedad87e8239ee69e7c50518d08 Mon Sep 17 00:00:00 2001 From: Adrian Cole Date: Wed, 18 Jan 2017 14:25:26 +0800 Subject: [PATCH] Supports backfilling timestamp for one-way spans Before, we only supported backfilling timestamp and duration for spans that have at least two annotations. This special cases an incomplete one-way span (ex only cs), by adding its timestamp. This prevents the UI from crashing. --- .../internal/ApplyTimestampAndDuration.java | 33 ++++++++++------- .../ApplyTimestampAndDurationTest.java | 37 ++++++++++++++++++- 2 files changed, 55 insertions(+), 15 deletions(-) diff --git a/zipkin/src/main/java/zipkin/internal/ApplyTimestampAndDuration.java b/zipkin/src/main/java/zipkin/internal/ApplyTimestampAndDuration.java index 1ff51cff624..86a062bda1b 100644 --- a/zipkin/src/main/java/zipkin/internal/ApplyTimestampAndDuration.java +++ b/zipkin/src/main/java/zipkin/internal/ApplyTimestampAndDuration.java @@ -1,5 +1,5 @@ /** - * Copyright 2015-2016 The OpenZipkin Authors + * Copyright 2015-2017 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 @@ -20,30 +20,37 @@ /** *

Derived timestamp and duration

* - *

Instrumentation should log timestamp and duration, but since these fields are recent - * (Nov-2015), a lot of tracers will not. Accordingly, this will backfill timestamp and duration to - * if possible, based on interpretation of annotations. + *

Instrumentation should log timestamp and duration in most cases, but since these fields are + * recent (Nov-2015), a lot of tracers will not. They also will not log timestamp or duration in + * one-way spans ("cs", "sr"). This includes a utility to backfill timestamp and duration at query + * time. It also includes a utility to guess a timestamp, which is useful when indexing incomplete + * spans. */ public class ApplyTimestampAndDuration { - // For spans that core client annotations, the distance between "cs" and "cr" should be the - // authoritative duration. We are special-casing this to avoid setting incorrect duration - // when there's skew between the client and the server. + /** + * For RPC two-way spans, the duration between "cs" and "cr" is authoritative. RPC one-way spans + * lack a response, so the duration is between "cs" and "sr". We special-case this to avoid + * setting incorrect duration when there's skew between the client and the server. + * + *

Note: this should only be used for query, not storage commands! + */ public static Span apply(Span span) { // Don't overwrite authoritatively set timestamp and duration! if (span.timestamp != null && span.duration != null) { return span; } - // Only calculate span.timestamp and duration on complete spans. This avoids - // persisting an inaccurate timestamp due to a late arriving annotation. + // We cannot backfill duration on a span with less than two annotations. However, we can + // backfill timestamp. if (span.annotations.size() < 2) { - return span; + if (span.timestamp != null) return span; + Long guess = guessTimestamp(span); + if (guess == null) return span; + return span.toBuilder().timestamp(guess).build(); } - // For spans that core client annotations, the distance between "cs" and "cr" should be the - // authoritative duration. We are special-casing this to avoid setting incorrect duration - // when there's skew between the client and the server. + // Prefer RPC one-way (cs -> sr) vs arbitrary annotations. Long first = span.annotations.get(0).timestamp; Long last = span.annotations.get(span.annotations.size() - 1).timestamp; for (int i = 0, length = span.annotations.size(); i < length; i++) { diff --git a/zipkin/src/test/java/zipkin/internal/ApplyTimestampAndDurationTest.java b/zipkin/src/test/java/zipkin/internal/ApplyTimestampAndDurationTest.java index 3ab44d4eb6b..b76de76fe60 100644 --- a/zipkin/src/test/java/zipkin/internal/ApplyTimestampAndDurationTest.java +++ b/zipkin/src/test/java/zipkin/internal/ApplyTimestampAndDurationTest.java @@ -1,5 +1,5 @@ /** - * Copyright 2015-2016 The OpenZipkin Authors + * Copyright 2015-2017 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 @@ -19,8 +19,11 @@ import zipkin.Span; import static org.assertj.core.api.Assertions.assertThat; +import static zipkin.Constants.CLIENT_RECV; import static zipkin.Constants.CLIENT_SEND; import static zipkin.Constants.SERVER_RECV; +import static zipkin.Constants.SERVER_SEND; +import static zipkin.internal.ApplyTimestampAndDuration.apply; import static zipkin.internal.ApplyTimestampAndDuration.guessTimestamp; public class ApplyTimestampAndDurationTest { @@ -28,13 +31,43 @@ public class ApplyTimestampAndDurationTest { Endpoint frontend = Endpoint.builder().serviceName("frontend").ipv4(192 << 24 | 12 << 16 | 1).port(8080).build(); Annotation cs = Annotation.create((50) * 1000, CLIENT_SEND, frontend); + Annotation cr = Annotation.create((100) * 1000, CLIENT_RECV, frontend); Endpoint backend = Endpoint.builder().serviceName("backend").ipv4(192 << 24 | 12 << 16 | 2).port(8080).build(); - Annotation sr = Annotation.create((95) * 1000, SERVER_RECV, backend); + Annotation sr = Annotation.create((70) * 1000, SERVER_RECV, backend); + Annotation ss = Annotation.create((80) * 1000, SERVER_SEND, backend); Span.Builder span = Span.builder().traceId(1).name("method1").id(666); + @Test + public void apply_onlyCs() { + assertThat(apply(span.addAnnotation(cs).build()).timestamp) + .isEqualTo(cs.timestamp); + } + + @Test + public void apply_rpcSpan() { + assertThat(apply(span + .addAnnotation(cs) + .addAnnotation(sr) + .addAnnotation(ss) + .addAnnotation(cr).build()).duration) + .isEqualTo(cr.timestamp - cs.timestamp); + } + + @Test + public void apply_serverOnly() { + assertThat(apply(span.addAnnotation(sr).addAnnotation(ss).build()).duration) + .isEqualTo(ss.timestamp - sr.timestamp); + } + + @Test + public void apply_oneWay() { + assertThat(apply(span.addAnnotation(cs).addAnnotation(sr).build()).duration) + .isEqualTo(sr.timestamp - cs.timestamp); + } + @Test public void bestTimestamp_isSpanTimestamp() { assertThat(guessTimestamp(span.timestamp(1L).build()))