Fixes refresh related bugs (#2445)

openzipkin · Mar 15, 2019 · 65a0a33 · 65a0a33
1 parent 3887af6
commit 65a0a33
Show file tree

Hide file tree

Showing 7 changed files with 67 additions and 94 deletions.
diff --git a/pom.xml b/pom.xml
@@ -49,10 +49,10 @@
 
     <main.basedir>${project.basedir}</main.basedir>
 
-    <armeria.version>0.80.0</armeria.version>
-    <brave.version>5.6.1</brave.version>
+    <armeria.version>0.81.0</armeria.version>
+    <brave.version>5.6.3</brave.version>
     <cassandra-driver-core.version>3.7.1</cassandra-driver-core.version>
-    <okhttp.version>3.13.1</okhttp.version>
+    <okhttp.version>3.14.0</okhttp.version>
     <okio.version>1.17.3</okio.version>
     <!-- important to keep this in sync with spring-boot-dependencies -->
     <jooq.version>3.11.9</jooq.version>

diff --git a/zipkin-autoconfigure/ui/src/main/java/zipkin2/autoconfigure/ui/AddHttpHeadersService.java b/zipkin-autoconfigure/ui/src/main/java/zipkin2/autoconfigure/ui/AddHttpHeadersService.java
diff --git a/...in-autoconfigure/ui/src/main/java/zipkin2/autoconfigure/ui/ZipkinUiAutoConfiguration.java b/...in-autoconfigure/ui/src/main/java/zipkin2/autoconfigure/ui/ZipkinUiAutoConfiguration.java
@@ -16,17 +16,19 @@
 import com.fasterxml.jackson.databind.ObjectMapper;
 import com.linecorp.armeria.common.HttpData;
 import com.linecorp.armeria.common.HttpHeaderNames;
-import com.linecorp.armeria.common.HttpHeaders;
 import com.linecorp.armeria.common.HttpRequest;
 import com.linecorp.armeria.common.HttpResponse;
 import com.linecorp.armeria.common.HttpStatus;
+import com.linecorp.armeria.common.MediaType;
 import com.linecorp.armeria.server.AbstractHttpService;
 import com.linecorp.armeria.server.HttpService;
 import com.linecorp.armeria.server.RedirectService;
+import com.linecorp.armeria.server.ServerCacheControl;
+import com.linecorp.armeria.server.ServerCacheControlBuilder;
 import com.linecorp.armeria.server.Service;
 import com.linecorp.armeria.server.ServiceRequestContext;
 import com.linecorp.armeria.server.file.HttpFileBuilder;
-import com.linecorp.armeria.server.file.HttpFileService;
+import com.linecorp.armeria.server.file.HttpFileServiceBuilder;
 import com.linecorp.armeria.spring.ArmeriaServerConfigurator;
 import io.netty.handler.codec.http.cookie.Cookie;
 import io.netty.handler.codec.http.cookie.ServerCookieDecoder;
@@ -46,8 +48,6 @@
 import org.springframework.context.annotation.Lazy;
 import org.springframework.core.io.Resource;
 
-import static com.linecorp.armeria.common.HttpHeaderNames.CACHE_CONTROL;
-import static com.linecorp.armeria.common.HttpHeaderNames.CONTENT_TYPE;
 import static java.nio.charset.StandardCharsets.UTF_8;
 import static zipkin2.autoconfigure.ui.ZipkinUiProperties.DEFAULT_BASEPATH;
 
@@ -77,15 +77,6 @@
 @EnableConfigurationProperties(ZipkinUiProperties.class)
 @ConditionalOnProperty(name = "zipkin.ui.enabled", matchIfMissing = true)
 class ZipkinUiAutoConfiguration {
-  static final HttpHeaders CACHE_YEAR =
-    HttpHeaders.of(CACHE_CONTROL, "max-age=" + TimeUnit.DAYS.toSeconds(365));
-  static final HttpHeaders CONFIG_HEADERS = HttpHeaders.of(HttpStatus.OK)
-    .add(HttpHeaders.of(CONTENT_TYPE, "application/json"))
-    .add(HttpHeaders.of(CACHE_CONTROL, "max-age=" + TimeUnit.MINUTES.toSeconds(10)));
-  static final HttpHeaders INDEX_HEADERS = HttpHeaders.of(HttpStatus.OK)
-    .add(HttpHeaders.of(CONTENT_TYPE, "text/html"))
-    .add(HttpHeaders.of(CACHE_CONTROL, "max-age=" + TimeUnit.MINUTES.toSeconds(1)));
-
   @Autowired
   ZipkinUiProperties ui;
 
@@ -130,27 +121,33 @@ String processedIndexHtml(Resource indexHtml) {
       lensIndex = HttpFileBuilder.of(HttpData.of(processedLensIndexHtml().getBytes(UTF_8)));
     }
 
-    legacyIndex.setHeaders(INDEX_HEADERS);
-    lensIndex.setHeaders(INDEX_HEADERS);
+    ServerCacheControl maxAgeMinute = new ServerCacheControlBuilder().maxAgeSeconds(60).build();
+    legacyIndex.contentType(MediaType.HTML_UTF_8).cacheControl(maxAgeMinute);
+    lensIndex.contentType(MediaType.HTML_UTF_8).cacheControl(maxAgeMinute);
 
+    // In both our old and new UI, assets have hashes in the filenames (generated by webpack).
+    // This allows us to host both simultaneously without conflict as long as we change the index
+    // file to point to the correct files.
     return new IndexSwitchingService(
       legacyIndex.build().asService(), lensIndex.build().asService());
   }
 
   @Bean @Lazy ArmeriaServerConfigurator uiServerConfigurator(
-    IndexSwitchingService indexSwitchingService)
-    throws IOException {
+    IndexSwitchingService indexSwitchingService) throws IOException {
+    ServerCacheControl maxAgeYear =
+      new ServerCacheControlBuilder().maxAgeSeconds(TimeUnit.DAYS.toSeconds(365)).build();
     Service<HttpRequest, HttpResponse> uiFileService =
-      new AddHttpHeadersService(HttpFileService.forClassPath("zipkin-ui")
-        .orElse(HttpFileService.forClassPath("zipkin-lens")), CACHE_YEAR);
-
-
+      HttpFileServiceBuilder.forClassPath("zipkin-ui").cacheControl(maxAgeYear).build()
+        .orElse(HttpFileServiceBuilder.forClassPath("zipkin-lens").cacheControl(maxAgeYear).build());
 
     byte[] config = new ObjectMapper().writeValueAsBytes(ui);
 
     return sb -> sb
-      .service("/zipkin/config.json",
-        HttpFileBuilder.of(HttpData.of(config)).addHeaders(CONFIG_HEADERS).build().asService())
+      .service("/zipkin/config.json", HttpFileBuilder.of(HttpData.of(config))
+        .cacheControl(new ServerCacheControlBuilder().maxAgeSeconds(600).build())
+        .contentType(MediaType.JSON_UTF_8)
+        .build()
+        .asService())
       .service("/zipkin/index.html", indexSwitchingService)
 
       // TODO This approach requires maintenance when new UI routes are added. Change to the following:
@@ -162,9 +159,10 @@ String processedIndexHtml(Resource indexHtml) {
       .service("/zipkin/dependency", indexSwitchingService)
       .service("/zipkin/traceViewer", indexSwitchingService)
 
-      .serviceUnder("/zipkin/", uiFileService)
       .service("/favicon.ico", new RedirectService(HttpStatus.FOUND, "/zipkin/favicon.ico"))
-      .service("/", new RedirectService(HttpStatus.FOUND, "/zipkin/"));
+      .service("/", new RedirectService(HttpStatus.FOUND, "/zipkin/"))
+      .service("/zipkin", new RedirectService(HttpStatus.FOUND, "/zipkin/"))
+      .serviceUnder("/zipkin/", uiFileService);
   }
 
   static class IndexSwitchingService extends AbstractHttpService {

diff --git a/...-autoconfigure/ui/src/test/java/zipkin2/autoconfigure/ui/ITZipkinUiAutoConfiguration.java b/...-autoconfigure/ui/src/test/java/zipkin2/autoconfigure/ui/ITZipkinUiAutoConfiguration.java
@@ -16,7 +16,9 @@
 import com.linecorp.armeria.server.Server;
 import java.io.IOException;
 import java.io.InputStream;
+import java.io.UncheckedIOException;
 import java.net.URL;
+import java.util.stream.Stream;
 import okhttp3.OkHttpClient;
 import okhttp3.Request;
 import okhttp3.Response;
@@ -52,6 +54,35 @@ public class ITZipkinUiAutoConfiguration {
       .isEqualTo("max-age=31536000");
   }
 
+  @Test public void redirectsIndex() throws Exception {
+    String index = get("/zipkin/index.html").body().string();
+
+    client = new OkHttpClient.Builder().followRedirects(true).build();
+
+    Stream.of("/zipkin", "/").forEach(path -> {
+      try {
+        assertThat(get(path).body().string()).isEqualTo(index);
+      } catch (IOException e) {
+        throw new UncheckedIOException(e);
+      }
+    });
+  }
+
+  /** Browsers honor conditional requests such as eTag. Let's make sure the server does */
+  @Test public void conditionalRequests() throws Exception {
+    Stream.of("/zipkin/config.json", "/zipkin/index.html", "/zipkin/test.txt").forEach(path -> {
+      try {
+        String etag = get(path).header("etag");
+        assertThat(conditionalGet(path, etag).code())
+          .isEqualTo(304);
+        assertThat(conditionalGet(path, "aargh").body().contentLength())
+          .isPositive();
+      } catch (IOException e) {
+        throw new UncheckedIOException(e);
+      }
+    });
+  }
+
   /**
    * The test sets the property {@code zipkin.ui.base-path=/foozipkin}, which should reflect in
    * index.html
@@ -87,4 +118,11 @@ private Response get(String path) throws IOException {
       .url("http://localhost:" + server.activePort().get().localAddress().getPort() + path)
       .build()).execute();
   }
+
+  private Response conditionalGet(String path, String etag) throws IOException {
+    return client.newCall(new Request.Builder()
+      .url("http://localhost:" + server.activePort().get().localAddress().getPort() + path)
+      .header("If-None-Match", etag)
+      .build()).execute();
+  }
 }
diff --git a/...utoconfigure/ui/src/test/java/zipkin2/autoconfigure/ui/ZipkinUiAutoConfigurationTest.java b/...utoconfigure/ui/src/test/java/zipkin2/autoconfigure/ui/ZipkinUiAutoConfigurationTest.java
@@ -63,11 +63,11 @@ public void indexHtmlFromClasspath() {
   }
 
   @Test
-  public void indexContentType() throws Exception {
+  public void indexContentType() {
     context = createContext();
     assertThat(
       serveIndex().headers().contentType())
-      .isEqualTo(MediaType.parse("text/html"));
+      .isEqualTo(MediaType.HTML_UTF_8);
   }
 
   @Test

diff --git a/zipkin-server/src/main/java/zipkin2/server/internal/ZipkinServerConfiguration.java b/zipkin-server/src/main/java/zipkin2/server/internal/ZipkinServerConfiguration.java
@@ -15,9 +15,6 @@
 
 import brave.Tracing;
 import com.linecorp.armeria.common.HttpMethod;
-import com.linecorp.armeria.common.HttpResponse;
-import com.linecorp.armeria.common.HttpStatus;
-import com.linecorp.armeria.common.MediaType;
 import com.linecorp.armeria.server.RedirectService;
 import com.linecorp.armeria.server.cors.CorsServiceBuilder;
 import com.linecorp.armeria.spring.ArmeriaServerConfigurator;
@@ -30,7 +27,6 @@
 import org.springframework.beans.factory.config.BeanPostProcessor;
 import org.springframework.boot.actuate.autoconfigure.metrics.MeterRegistryCustomizer;
 import org.springframework.boot.actuate.health.HealthAggregator;
-import org.springframework.boot.actuate.metrics.export.prometheus.PrometheusScrapeEndpoint;
 import org.springframework.boot.autoconfigure.ImportAutoConfiguration;
 import org.springframework.boot.autoconfigure.condition.ConditionalOnMissingBean;
 import org.springframework.context.annotation.Bean;
@@ -61,7 +57,7 @@ public class ZipkinServerConfiguration implements WebMvcConfigurer {
   @Autowired(required = false)
   MetricsHealthController healthController;
 
-  @Bean ArmeriaServerConfigurator serverConfigurator(PrometheusScrapeEndpoint prom) {
+  @Bean ArmeriaServerConfigurator serverConfigurator() {
     return sb -> {
       if (httpQuery != null) {
         sb.annotatedService(httpQuery);
@@ -73,14 +69,6 @@ public class ZipkinServerConfiguration implements WebMvcConfigurer {
       sb.service("/prometheus", new RedirectService("/actuator/prometheus"));
       // Redirects the info endpoint for backward compatibility
       sb.service("/info", new RedirectService("/actuator/info"));
-
-      // TODO: Workaround for https://github.com/line/armeria/issues/1637
-      MediaType promMedia = MediaType.parse("text/plain; version=0.0.4; charset=utf-8");
-      sb.decorator(
-        delegate -> (ctx, req) -> {
-          if (!"/actuator/prometheus".equals(req.path())) return delegate.serve(ctx, req);
-          return HttpResponse.of(HttpStatus.OK, promMedia, prom.scrape());
-        });
     };
   }
 

diff --git a/zipkin-server/src/test/java/zipkin2/server/internal/ZipkinServerConfigurationTest.java b/zipkin-server/src/test/java/zipkin2/server/internal/ZipkinServerConfigurationTest.java
@@ -24,7 +24,6 @@
 import org.springframework.boot.actuate.autoconfigure.endpoint.EndpointAutoConfiguration;
 import org.springframework.boot.actuate.health.HealthAggregator;
 import org.springframework.boot.actuate.health.OrderedHealthAggregator;
-import org.springframework.boot.actuate.metrics.export.prometheus.PrometheusScrapeEndpoint;
 import org.springframework.boot.autoconfigure.context.PropertyPlaceholderAutoConfiguration;
 import org.springframework.boot.test.util.TestPropertyValues;
 import org.springframework.context.annotation.AnnotationConfigApplicationContext;
@@ -46,7 +45,6 @@ public class ZipkinServerConfigurationTest {
   @Test public void httpCollector_enabledByDefault() {
     context.register(
       ArmeriaSpringActuatorAutoConfiguration.class,
-      PrometheusScrapeEndpointConfiguration.class,
       EndpointAutoConfiguration.class,
       PropertyPlaceholderAutoConfiguration.class,
       ZipkinServerConfigurationTest.Config.class,
@@ -58,19 +56,11 @@ public class ZipkinServerConfigurationTest {
     assertThat(context.getBean(ZipkinHttpCollector.class)).isNotNull();
   }
 
-  // TODO: Remove when removing workaround for https://github.com/line/armeria/issues/1637
-  @Deprecated static class PrometheusScrapeEndpointConfiguration {
-    @Bean PrometheusScrapeEndpoint prometheusEndpoint() {
-      return new PrometheusScrapeEndpoint(null);
-    }
-  }
-
   @Test(expected = NoSuchBeanDefinitionException.class)
   public void httpCollector_canDisable() {
     TestPropertyValues.of("zipkin.collector.http.enabled:false").applyTo(context);
     context.register(
       ArmeriaSpringActuatorAutoConfiguration.class,
-      PrometheusScrapeEndpointConfiguration.class,
       EndpointAutoConfiguration.class,
       PropertyPlaceholderAutoConfiguration.class,
       ZipkinServerConfigurationTest.Config.class,
@@ -85,7 +75,6 @@ public void httpCollector_canDisable() {
   @Test public void query_enabledByDefault() {
     context.register(
       ArmeriaSpringActuatorAutoConfiguration.class,
-      PrometheusScrapeEndpointConfiguration.class,
       EndpointAutoConfiguration.class,
       PropertyPlaceholderAutoConfiguration.class,
       ZipkinServerConfigurationTest.Config.class,
@@ -101,7 +90,6 @@ public void httpCollector_canDisable() {
     TestPropertyValues.of("zipkin.query.enabled:false").applyTo(context);
     context.register(
       ArmeriaSpringActuatorAutoConfiguration.class,
-      PrometheusScrapeEndpointConfiguration.class,
       EndpointAutoConfiguration.class,
       PropertyPlaceholderAutoConfiguration.class,
       ZipkinServerConfigurationTest.Config.class,
@@ -121,7 +109,6 @@ public void httpCollector_canDisable() {
     TestPropertyValues.of("zipkin.self-tracing.enabled:true").applyTo(context);
     context.register(
       ArmeriaSpringActuatorAutoConfiguration.class,
-      PrometheusScrapeEndpointConfiguration.class,
       EndpointAutoConfiguration.class,
       PropertyPlaceholderAutoConfiguration.class,
       ZipkinServerConfigurationTest.Config.class,
@@ -137,7 +124,6 @@ public void httpCollector_canDisable() {
     TestPropertyValues.of("zipkin.storage.search-enabled:false").applyTo(context);
     context.register(
       ArmeriaSpringActuatorAutoConfiguration.class,
-      PrometheusScrapeEndpointConfiguration.class,
       EndpointAutoConfiguration.class,
       PropertyPlaceholderAutoConfiguration.class,
       ZipkinServerConfigurationTest.Config.class,