Merge pull request #12534 from vespa-engine/bjorncs/jdisc-metric-unhandled-exception

Bjorncs/jdisc metric unhandled exception

5 files changed, 81 insertions, 3 deletions

diff --git a/container-core/pom.xml b/container-core/pom.xml
index e51d99c3b78..64e5ebb00d3 100644
--- a/container-core/pom.xml
+++ b/container-core/pom.xml
@@ -242,6 +242,11 @@
       <artifactId>wiremock-standalone</artifactId>
       <scope>test</scope>
     </dependency>
+    <dependency>
+      <groupId>org.assertj</groupId>
+      <artifactId>assertj-core</artifactId>
+      <scope>test</scope>
+    </dependency>
   </dependencies>
   <build>
     <plugins>
diff --git a/container-core/src/main/java/com/yahoo/container/handler/ThreadPoolProvider.java b/container-core/src/main/java/com/yahoo/container/handler/ThreadPoolProvider.java
index 4cc3b48fd1a..b427a58c9b7 100644
--- a/container-core/src/main/java/com/yahoo/container/handler/ThreadPoolProvider.java
+++ b/container-core/src/main/java/com/yahoo/container/handler/ThreadPoolProvider.java
@@ -9,6 +9,7 @@ import com.yahoo.container.di.componentgraph.Provider;
 import com.yahoo.container.protect.ProcessTerminator;
 import com.yahoo.jdisc.Metric;
 
+import java.util.Map;
 import java.util.concurrent.BlockingQueue;
 import java.util.concurrent.Executor;
 import java.util.concurrent.ExecutorService;
@@ -43,7 +44,8 @@ public class ThreadPoolProvider extends AbstractComponent implements Provider<Ex
                                                              threadpoolConfig.maxthreads(),
                                                              0L, TimeUnit.SECONDS,
                                                              new SynchronousQueue<>(false),
-                                                             ThreadFactoryFactory.getThreadFactory("threadpool"));
+                                                             ThreadFactoryFactory.getThreadFactory("threadpool"),
+                                                             metric);
         // Prestart needed, if not all threads will be created by the fist N tasks and hence they might also
         // get the dreaded thread locals initialized even if they will never run.
         // That counters what we we want to achieve with the Q that will prefer thread locality.
@@ -161,17 +163,22 @@ public class ThreadPoolProvider extends AbstractComponent implements Provider<Ex
     /** A thread pool executor which maintains the last time a worker completed */
     private final static class WorkerCompletionTimingThreadPoolExecutor extends ThreadPoolExecutor {
 
+        private static final String UNHANDLED_EXCEPTIONS_METRIC = "jdisc.thread_pool.unhandled_exceptions";
+
         volatile long lastThreadAssignmentTimeMillis = System.currentTimeMillis();
         private final AtomicLong startedCount = new AtomicLong(0);
         private final AtomicLong completedCount = new AtomicLong(0);
+        private final Metric metric;
 
         public WorkerCompletionTimingThreadPoolExecutor(int corePoolSize,
                                                         int maximumPoolSize,
                                                         long keepAliveTime,
                                                         TimeUnit unit,
                                                         BlockingQueue<Runnable> workQueue,
-                                                        ThreadFactory threadFactory) {
+                                                        ThreadFactory threadFactory,
+                                                        Metric metric) {
             super(corePoolSize, maximumPoolSize, keepAliveTime, unit, workQueue, threadFactory);
+            this.metric = metric;
         }
 
         @Override
@@ -185,6 +192,9 @@ public class ThreadPoolProvider extends AbstractComponent implements Provider<Ex
         protected void afterExecute(Runnable r, Throwable t) {
             super.afterExecute(r, t);
             completedCount.incrementAndGet();
+            if (t != null) {
+                metric.add(UNHANDLED_EXCEPTIONS_METRIC, 1L, metric.createContext(Map.of("exception", t.getClass().getSimpleName())));
+            }
         }
 
         @Override
diff --git a/container-core/src/main/java/com/yahoo/container/jdisc/ThreadedHttpRequestHandler.java b/container-core/src/main/java/com/yahoo/container/jdisc/ThreadedHttpRequestHandler.java
index a9ea737c96c..c58d49bf8c8 100644
--- a/container-core/src/main/java/com/yahoo/container/jdisc/ThreadedHttpRequestHandler.java
+++ b/container-core/src/main/java/com/yahoo/container/jdisc/ThreadedHttpRequestHandler.java
@@ -13,6 +13,7 @@ import com.yahoo.log.LogLevel;
 
 import java.io.IOException;
 import java.nio.ByteBuffer;
+import java.util.Map;
 import java.util.concurrent.Executor;
 import java.util.concurrent.TimeUnit;
 import java.util.logging.Logger;
@@ -31,6 +32,7 @@ public abstract class ThreadedHttpRequestHandler extends ThreadedRequestHandler
 
     public static final String CONTENT_TYPE = "Content-Type";
     private static final String RENDERING_ERRORS = "rendering_errors";
+    private static final String UNHANDLED_EXCEPTIONS_METRIC = "jdisc.http.handler.unhandled_exceptions";
 
     /** Logger for subclasses */
     protected final Logger log;
@@ -79,6 +81,7 @@ public abstract class ThreadedHttpRequestHandler extends ThreadedRequestHandler
             channel.setHttpResponse(httpResponse); // may or may not have already been done
             render(httpRequest, httpResponse, channel, jdiscRequest.creationTime(TimeUnit.MILLISECONDS));
         } catch (Exception e) {
+            metric.add(UNHANDLED_EXCEPTIONS_METRIC, 1L, contextFor(request, Map.of("exception", e.getClass().getSimpleName())));
             metric.add(RENDERING_ERRORS, 1, null);
             log.log(LogLevel.ERROR, "Uncaught exception handling request", e);
             if (channel != null) {
diff --git a/container-core/src/main/java/com/yahoo/container/jdisc/ThreadedRequestHandler.java b/container-core/src/main/java/com/yahoo/container/jdisc/ThreadedRequestHandler.java
index 6c8cee8433c..99732af9d31 100644
--- a/container-core/src/main/java/com/yahoo/container/jdisc/ThreadedRequestHandler.java
+++ b/container-core/src/main/java/com/yahoo/container/jdisc/ThreadedRequestHandler.java
@@ -79,7 +79,7 @@ public abstract class ThreadedRequestHandler extends AbstractRequestHandler {
         this.allowAsyncResponse = allowAsyncResponse;
     }
 
-    private Metric.Context contextFor(Request request) {
+    Metric.Context contextFor(Request request, Map<String, String> extraDimensions) {
         BindingMatch match = request.getBindingMatch();
         if (match == null) return null;
         UriPattern matched = match.matched();
@@ -97,9 +97,12 @@ public abstract class ThreadedRequestHandler extends AbstractRequestHandler {
         dimensions.put("port", Integer.toString(uri.getPort()));
         String handlerClassName = getClass().getName();
         dimensions.put("handler-name", handlerClassName);
+        dimensions.putAll(extraDimensions);
         return this.metric.createContext(dimensions);
     }
 
+    private Metric.Context contextFor(Request request) { return contextFor(request, Map.of()); }
+
     /**
      * Handles a request by assigning a worker thread to it.
      *
diff --git a/container-core/src/test/java/com/yahoo/container/jdisc/ThreadedHttpRequestHandlerTest.java b/container-core/src/test/java/com/yahoo/container/jdisc/ThreadedHttpRequestHandlerTest.java
new file mode 100644
index 00000000000..07dba21e5b6
--- /dev/null
+++ b/container-core/src/test/java/com/yahoo/container/jdisc/ThreadedHttpRequestHandlerTest.java
@@ -0,0 +1,57 @@
+// Copyright Verizon Media. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
+package com.yahoo.container.jdisc;
+
+import com.yahoo.jdisc.Metric;
+import org.junit.Test;
+
+import java.util.Map;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.Executors;
+
+import static org.assertj.core.api.Assertions.assertThat;
+
+/**
+ * @author bjorncs
+ */
+public class ThreadedHttpRequestHandlerTest {
+
+    @Test
+    public void unhandled_exceptions_metric_is_incremented_if_subclassed_handler_throws_exception() {
+        MetricMock metricMock = new MetricMock();
+        ThreadedHttpRequestHandlerThrowingException handler = new ThreadedHttpRequestHandlerThrowingException(metricMock);
+        RequestHandlerTestDriver driver = new RequestHandlerTestDriver(handler);
+
+        driver.sendRequest("http://localhost/myhandler");
+        String expectedMetricName = "jdisc.http.handler.unhandled_exceptions";
+        assertThat(metricMock.addInvocations)
+                .containsKey(expectedMetricName);
+        assertThat(metricMock.addInvocations.get(expectedMetricName).dimensions)
+                .containsEntry("exception", "DummyException");
+    }
+
+    private static class MetricMock implements Metric {
+        final ConcurrentHashMap<String, SimpleMetricContext> addInvocations = new ConcurrentHashMap<>();
+
+        @Override public void add(String key, Number val, Context ctx) {
+            addInvocations.put(key, (SimpleMetricContext)ctx);
+        }
+        @Override public void set(String key, Number val, Context ctx) {}
+        @Override public Context createContext(Map<String, ?> properties) { return new SimpleMetricContext(properties); }
+    }
+
+    private static class SimpleMetricContext implements Metric.Context {
+        final Map<String, String> dimensions;
+
+        @SuppressWarnings("unchecked")
+        SimpleMetricContext(Map<String, ?> dimensions) { this.dimensions = (Map<String, String>)dimensions; }
+    }
+
+    private static class ThreadedHttpRequestHandlerThrowingException extends ThreadedHttpRequestHandler {
+        ThreadedHttpRequestHandlerThrowingException(Metric metric) {
+            super(Executors.newSingleThreadExecutor(), metric);
+        }
+        @Override public HttpResponse handle(HttpRequest request) { throw new DummyException(); }
+    }
+
+    private static class DummyException extends RuntimeException {}
+}
\ No newline at end of file