Address comments by @trustin

line · Feb 13, 2019 · 2e387f7 · 2e387f7
1 parent f0cedd4
commit 2e387f7
Show file tree

Hide file tree

Showing 8 changed files with 43 additions and 54 deletions.
diff --git a/core/src/main/java/com/linecorp/armeria/common/AbstractRequestContext.java b/core/src/main/java/com/linecorp/armeria/common/AbstractRequestContext.java
@@ -153,10 +153,10 @@ public boolean isTimedOut() {
      * Marks this {@link RequestContext} as having been timed out. Any callbacks created with
      * {@code makeContextAware} that are run after this will be failed with {@link CancellationException}.
      *
-     * @deprecated Use {@link DefaultServiceRequestContext#setRequestTimedOut()}.
+     * @deprecated Use {@link DefaultServiceRequestContext#setTimedOut()}.
      */
     @Deprecated
-    protected void setTimedOut() {
+    public void setTimedOut() {
         timedOut = true;
     }
 

diff --git a/core/src/main/java/com/linecorp/armeria/common/RequestContext.java b/core/src/main/java/com/linecorp/armeria/common/RequestContext.java
@@ -440,7 +440,7 @@ default <T> CompletableFuture<T> makeContextAware(CompletableFuture<T> future) {
      * Returns whether this {@link RequestContext} has been timed-out (e.g., when the corresponding request
      * passes a deadline).
      *
-     * @deprecated Use {@link ServiceRequestContext#isRequestTimedOut()}.
+     * @deprecated Use {@link ServiceRequestContext#isTimedOut()}.
      */
     @Deprecated
     boolean isTimedOut();

diff --git a/core/src/main/java/com/linecorp/armeria/internal/metric/RequestMetricSupport.java b/core/src/main/java/com/linecorp/armeria/internal/metric/RequestMetricSupport.java
@@ -89,7 +89,7 @@ private static void onResponse(RequestLog log, MeterIdPrefixFunction meterIdPref
                                                                           DefaultServiceRequestMetrics::new);
             updateMetrics(log, metrics);
             if (log.responseCause() instanceof RequestTimeoutException) {
-                metrics.requestTimeout().increment();
+                metrics.requestTimeouts().increment();
             }
             return;
         }
@@ -100,9 +100,9 @@ private static void onResponse(RequestLog log, MeterIdPrefixFunction meterIdPref
         updateMetrics(log, metrics);
         final Throwable responseCause = log.responseCause();
         if (responseCause instanceof WriteTimeoutException) {
-            metrics.writeTimeout().increment();
+            metrics.writeTimeouts().increment();
         } else if (responseCause instanceof ResponseTimeoutException) {
-            metrics.responseTimeout().increment();
+            metrics.responseTimeouts().increment();
         }
     }
 
@@ -163,13 +163,13 @@ private interface RequestMetrics {
     }
 
     private interface ClientRequestMetrics extends RequestMetrics {
-        Counter writeTimeout();
+        Counter writeTimeouts();
 
-        Counter responseTimeout();
+        Counter responseTimeouts();
     }
 
     private interface ServiceRequestMetrics extends RequestMetrics {
-        Counter requestTimeout();
+        Counter requestTimeouts();
     }
 
     private static final class ActiveRequestMetrics extends LongAdder {}
@@ -240,39 +240,41 @@ public Timer totalDuration() {
     private static class DefaultClientRequestMetrics
             extends AbstractRequestMetrics implements ClientRequestMetrics {
 
-        private final Counter writeTimeout;
-        private final Counter responseTimeout;
+        private final Counter writeTimeouts;
+        private final Counter responseTimeouts;
 
         DefaultClientRequestMetrics(MeterRegistry parent, MeterIdPrefix idPrefix) {
             super(parent, idPrefix);
-            writeTimeout = parent.counter(idPrefix.name("writeTimeout"), idPrefix.tags());
-            responseTimeout = parent.counter(idPrefix.name("responseTimeout"), idPrefix.tags());
+            final String timeouts = idPrefix.name("timeouts");
+            writeTimeouts = parent.counter(timeouts, idPrefix.tags("cause", "WriteTimeoutException"));
+            responseTimeouts = parent.counter(timeouts, idPrefix.tags("cause", "ResponseTimeoutException"));
         }
 
         @Override
-        public Counter writeTimeout() {
-            return writeTimeout;
+        public Counter writeTimeouts() {
+            return writeTimeouts;
         }
 
         @Override
-        public Counter responseTimeout() {
-            return responseTimeout;
+        public Counter responseTimeouts() {
+            return responseTimeouts;
         }
     }
 
     private static class DefaultServiceRequestMetrics
             extends AbstractRequestMetrics implements ServiceRequestMetrics {
 
-        private final Counter requestTimeout;
+        private final Counter requestTimeouts;
 
         DefaultServiceRequestMetrics(MeterRegistry parent, MeterIdPrefix idPrefix) {
             super(parent, idPrefix);
-            requestTimeout = parent.counter(idPrefix.name("requestTimeout"), idPrefix.tags());
+            requestTimeouts = parent.counter(idPrefix.name("timeouts"),
+                                             idPrefix.tags("cause", "RequestTimeoutException"));
         }
 
         @Override
-        public Counter requestTimeout() {
-            return requestTimeout;
+        public Counter requestTimeouts() {
+            return requestTimeouts;
         }
     }
 }
diff --git a/core/src/main/java/com/linecorp/armeria/server/DefaultServiceRequestContext.java b/core/src/main/java/com/linecorp/armeria/server/DefaultServiceRequestContext.java
@@ -90,8 +90,6 @@ public class DefaultServiceRequestContext extends NonWrappingRequestContext impl
     private Runnable requestTimeoutHandler;
     private long maxRequestLength;
 
-    private boolean timedOut;
-
     @Nullable
     private volatile RequestTimeoutChangeListener requestTimeoutChangeListener;
     @Nullable
@@ -408,18 +406,13 @@ public void setRequestTimeoutHandler(Runnable requestTimeoutHandler) {
         this.requestTimeoutHandler = requireNonNull(requestTimeoutHandler, "requestTimeoutHandler");
     }
 
-    @Override
-    public boolean isRequestTimedOut() {
-        return timedOut;
-    }
-
     /**
      * Marks this {@link ServiceRequestContext} as having been timed out. Any callbacks created with
      * {@code makeContextAware} that are run after this will be failed with {@link CancellationException}.
      */
-    public void setRequestTimedOut() {
-        timedOut = true;
-        setTimedOut();
+    @Override
+    public void setTimedOut() {
+        super.setTimedOut();
     }
 
     @Override

diff --git a/core/src/main/java/com/linecorp/armeria/server/ServiceRequestContext.java b/core/src/main/java/com/linecorp/armeria/server/ServiceRequestContext.java
@@ -220,7 +220,8 @@ default MediaType negotiatedProduceType() {
      * Returns whether this {@link ServiceRequestContext} has been timed-out (e.g., when the
      * corresponding request passes a deadline).
      */
-    boolean isRequestTimedOut();
+    @Override
+    boolean isTimedOut();
 
     /**
      * Returns the maximum length of the current {@link Request}.

diff --git a/core/src/main/java/com/linecorp/armeria/server/ServiceRequestContextWrapper.java b/core/src/main/java/com/linecorp/armeria/server/ServiceRequestContextWrapper.java
@@ -153,11 +153,6 @@ public void setRequestTimeoutHandler(Runnable requestTimeoutHandler) {
         delegate().setRequestTimeoutHandler(requestTimeoutHandler);
     }
 
-    @Override
-    public boolean isRequestTimedOut() {
-        return delegate().isRequestTimedOut();
-    }
-
     @Override
     public long maxRequestLength() {
         return delegate().maxRequestLength();

diff --git a/core/src/test/java/com/linecorp/armeria/internal/metric/RequestMetricSupportTest.java b/core/src/test/java/com/linecorp/armeria/internal/metric/RequestMetricSupportTest.java
@@ -118,8 +118,10 @@ public void responseTimedOutInClientSide() {
                                                0.0)
                                 .containsEntry("foo.requests#count{httpStatus=0,method=POST,result=failure}",
                                                1.0)
-                                .containsEntry("foo.writeTimeout#count{httpStatus=0,method=POST}", 0.0)
-                                .containsEntry("foo.responseTimeout#count{httpStatus=0,method=POST}", 1.0)
+                                .containsEntry("foo.timeouts#count{cause=WriteTimeoutException," +
+                                               "httpStatus=0,method=POST}", 0.0)
+                                .containsEntry("foo.timeouts#count{cause=ResponseTimeoutException," +
+                                               "httpStatus=0,method=POST}", 1.0)
                                 .containsEntry("foo.responseDuration#count{httpStatus=0,method=POST}", 1.0)
                                 .containsEntry("foo.responseLength#count{httpStatus=0,method=POST}", 1.0)
                                 .containsEntry("foo.totalDuration#count{httpStatus=0,method=POST}", 1.0);
@@ -139,8 +141,10 @@ public void writeTimedOutInClientSide() {
                                                0.0)
                                 .containsEntry("foo.requests#count{httpStatus=0,method=POST,result=failure}",
                                                1.0)
-                                .containsEntry("foo.writeTimeout#count{httpStatus=0,method=POST}", 1.0)
-                                .containsEntry("foo.responseTimeout#count{httpStatus=0,method=POST}", 0.0)
+                                .containsEntry("foo.timeouts#count{cause=WriteTimeoutException," +
+                                               "httpStatus=0,method=POST}", 1.0)
+                                .containsEntry("foo.timeouts#count{cause=ResponseTimeoutException," +
+                                               "httpStatus=0,method=POST}", 0.0)
                                 .containsEntry("foo.responseDuration#count{httpStatus=0,method=POST}", 0.0)
                                 .containsEntry("foo.responseLength#count{httpStatus=0,method=POST}", 0.0)
                                 .containsEntry("foo.totalDuration#count{httpStatus=0,method=POST}", 0.0);
@@ -189,6 +193,8 @@ public void requestTimedOutInServerSide() {
                                "pathMapping=exact:/foo,result=success}", 0.0)
                 .containsEntry("foo.requests#count{hostnamePattern=*,httpStatus=503,method=POST," +
                                "pathMapping=exact:/foo,result=failure}", 1.0)
+                .containsEntry("foo.timeouts#count{cause=RequestTimeoutException,hostnamePattern=*," +
+                               "httpStatus=503,method=POST,pathMapping=exact:/foo}", 1.0)
                 .containsEntry("foo.responseDuration#count{hostnamePattern=*,httpStatus=503,method=POST," +
                                "pathMapping=exact:/foo}", 1.0)
                 .containsEntry("foo.responseLength#count{hostnamePattern=*,httpStatus=503,method=POST," +

diff --git a/core/src/test/java/com/linecorp/armeria/server/DefaultServiceRequestContextTest.java b/core/src/test/java/com/linecorp/armeria/server/DefaultServiceRequestContextTest.java
@@ -17,39 +17,31 @@
 package com.linecorp.armeria.server;
 
 import static org.assertj.core.api.Assertions.assertThat;
-import static org.mockito.Mockito.mock;
 
 import org.junit.Test;
 
-import com.google.common.collect.ImmutableList;
-
 import com.linecorp.armeria.common.DefaultHttpHeaders;
 import com.linecorp.armeria.common.HttpHeaderNames;
 import com.linecorp.armeria.common.HttpMethod;
 import com.linecorp.armeria.common.HttpRequest;
 import com.linecorp.armeria.common.HttpResponse;
-import com.linecorp.armeria.common.MediaType;
 import com.linecorp.armeria.common.Request;
-import com.linecorp.armeria.common.SessionProtocol;
-import com.linecorp.armeria.common.metric.NoopMeterRegistry;
 
-import io.netty.channel.Channel;
 import io.netty.util.AttributeKey;
-import io.netty.util.NetUtil;
 
 public class DefaultServiceRequestContextTest {
 
     @Test
     public void requestTimedOut() {
         final HttpRequest request = HttpRequest.of(HttpMethod.GET, "/hello");
         final ServiceRequestContext ctx = ServiceRequestContextBuilder.of(request).build();
+        assertThat(ctx.isTimedOut()).isFalse();
+
         assert ctx instanceof DefaultServiceRequestContext;
         final DefaultServiceRequestContext defaultCtx = (DefaultServiceRequestContext) ctx;
-        assertThat(defaultCtx.isRequestTimedOut()).isFalse();
-        assertThat(defaultCtx.isTimedOut()).isFalse();
-        defaultCtx.setRequestTimedOut();
-        assertThat(defaultCtx.isRequestTimedOut()).isTrue();
-        assertThat(defaultCtx.isTimedOut()).isTrue();
+        defaultCtx.setTimedOut();
+
+        assertThat(ctx.isTimedOut()).isTrue();
     }
 
     @Test