Improve metrics for /document/v1

Make more consistent with legacy feed api

4 files changed, 59 insertions, 31 deletions

diff --git a/vespaclient-container-plugin/src/main/java/com/yahoo/document/restapi/resource/DocumentV1ApiHandler.java b/vespaclient-container-plugin/src/main/java/com/yahoo/document/restapi/resource/DocumentV1ApiHandler.java
index 10040ab6d0f..5b28f975564 100644
--- a/vespaclient-container-plugin/src/main/java/com/yahoo/document/restapi/resource/DocumentV1ApiHandler.java
+++ b/vespaclient-container-plugin/src/main/java/com/yahoo/document/restapi/resource/DocumentV1ApiHandler.java
@@ -462,7 +462,7 @@ public class DocumentV1ApiHandler extends AbstractRequestHandler {
                 DocumentOperationParameters parameters = parametersFromRequest(request, ROUTE)
                         .withResponseHandler(response -> {
                             outstanding.decrementAndGet();
-                            updatePutMetrics(response.outcome());
+                            updatePutMetrics(response.outcome(), latencyOf(request));
                             handleFeedOperation(path, put.fullyApplied(), handler, response);
                         });
                 return () -> dispatchOperation(() -> asyncSession.put((DocumentPut)put.operation(), parameters));
@@ -486,7 +486,7 @@ public class DocumentV1ApiHandler extends AbstractRequestHandler {
                 DocumentOperationParameters parameters = parametersFromRequest(request, ROUTE)
                         .withResponseHandler(response -> {
                             outstanding.decrementAndGet();
-                            updateUpdateMetrics(response.outcome(), update.getCreateIfNonExistent());
+                            updateUpdateMetrics(response.outcome(), latencyOf(request), update.getCreateIfNonExistent());
                             handleFeedOperation(path, parsed.fullyApplied(), handler, response);
                         });
                 return () -> dispatchOperation(() -> asyncSession.update(update, parameters));
@@ -507,7 +507,7 @@ public class DocumentV1ApiHandler extends AbstractRequestHandler {
             DocumentOperationParameters parameters = parametersFromRequest(request, ROUTE)
                     .withResponseHandler(response -> {
                         outstanding.decrementAndGet();
-                        updateRemoveMetrics(response.outcome());
+                        updateRemoveMetrics(response.outcome(), latencyOf(request));
                         handleFeedOperation(path, true, handler, response);
                     });
             return () -> dispatchOperation(() -> asyncSession.remove(remove, parameters));
@@ -1090,32 +1090,50 @@ public class DocumentV1ApiHandler extends AbstractRequestHandler {
         handle(path, null, handler, response, (document, jsonResponse) -> jsonResponse.commit(Response.Status.OK, fullyApplied));
     }
 
-    private void updatePutMetrics(Outcome outcome) {
+    private static double latencyOf(HttpRequest r) { return (System.nanoTime() - r.relativeCreatedAtNanoTime()) / 1e+9d; }
+
+    private void updatePutMetrics(Outcome outcome, double latency) {
+        incrementMetricNumOperations(); incrementMetricNumPuts(); sampleLatency(latency);
         switch (outcome) {
-            case SUCCESS -> metric.add(MetricNames.SUCCEEDED, 1, null);
-            case CONDITION_FAILED -> metric.add(MetricNames.CONDITION_NOT_MET, 1, null);
-            default -> metric.add(MetricNames.FAILED, 1, null);
+            case SUCCESS -> incrementMetricSucceeded();
+            case CONDITION_FAILED -> { incrementMetricSucceeded(); incrementMetricConditionNotMet(); }
+            default -> incrementMetricFailed();
         }
     }
 
-    private void updateUpdateMetrics(Outcome outcome, boolean create) {
+    private void updateUpdateMetrics(Outcome outcome, double latency, boolean create) {
         if (create && outcome == Outcome.NOT_FOUND) outcome = Outcome.SUCCESS; // >_<
+        incrementMetricNumOperations(); incrementMetricNumUpdates(); sampleLatency(latency);
         switch (outcome) {
-            case SUCCESS -> metric.add(MetricNames.SUCCEEDED, 1, null);
-            case NOT_FOUND -> metric.add(MetricNames.NOT_FOUND, 1, null);
-            case CONDITION_FAILED -> metric.add(MetricNames.CONDITION_NOT_MET, 1, null);
-            default -> metric.add(MetricNames.FAILED, 1, null);
+            case SUCCESS -> incrementMetricSucceeded();
+            case NOT_FOUND -> { incrementMetricSucceeded(); incrementMetricNotFound(); }
+            case CONDITION_FAILED -> { incrementMetricSucceeded(); incrementMetricConditionNotMet(); }
+            default -> incrementMetricFailed();
         }
     }
 
-    private void updateRemoveMetrics(Outcome outcome) {
+    private void updateRemoveMetrics(Outcome outcome, double latency) {
+        incrementMetricNumOperations(); incrementMetricNumRemoves(); sampleLatency(latency);
         switch (outcome) {
-            case SUCCESS, NOT_FOUND -> metric.add(MetricNames.SUCCEEDED, 1, null);
-            case CONDITION_FAILED -> metric.add(MetricNames.CONDITION_NOT_MET, 1, null);
-            case INSUFFICIENT_STORAGE, TIMEOUT, ERROR -> metric.add(MetricNames.FAILED, 1, null);
+            case SUCCESS -> incrementMetricSucceeded();
+            case NOT_FOUND -> { incrementMetricSucceeded(); incrementMetricNotFound(); }
+            case CONDITION_FAILED -> { incrementMetricSucceeded(); incrementMetricConditionNotMet(); }
+            default -> incrementMetricFailed();
         }
     }
 
+    private void sampleLatency(double latency) { setMetric(MetricNames.LATENCY, latency); }
+    private void incrementMetricNumOperations() { incrementMetric(MetricNames.NUM_OPERATIONS); }
+    private void incrementMetricNumPuts() { incrementMetric(MetricNames.NUM_PUTS); }
+    private void incrementMetricNumRemoves() { incrementMetric(MetricNames.NUM_REMOVES); }
+    private void incrementMetricNumUpdates() { incrementMetric(MetricNames.NUM_UPDATES); }
+    private void incrementMetricFailed() { incrementMetric(MetricNames.FAILED); }
+    private void incrementMetricConditionNotMet() { incrementMetric(MetricNames.CONDITION_NOT_MET); }
+    private void incrementMetricSucceeded() { incrementMetric(MetricNames.SUCCEEDED); }
+    private void incrementMetricNotFound() { incrementMetric(MetricNames.NOT_FOUND); }
+    private void incrementMetric(String n) { metric.add(n, 1, null); }
+    private void setMetric(String n, Number v) { metric.set(n, v, null); }
+
     // ------------------------------------------------- Visits ------------------------------------------------
 
     private VisitorParameters parseGetParameters(HttpRequest request, DocumentPath path, boolean streamed) {
@@ -1446,17 +1464,20 @@ public class DocumentV1ApiHandler extends AbstractRequestHandler {
 
         @Override
         public ContentChannel handleResponse(Response response) {
-            var statusCodeGroup = response.getStatus() / 100;
-            // Status code 412 - condition not met - is considered OK
-            if (statusCodeGroup == 2 || response.getStatus() == 412)
-                metrics.reportSuccessful(type, start);
-            else if (statusCodeGroup == 4)
-                metrics.reportFailure(type, DocumentOperationStatus.REQUEST_ERROR);
-            else if (statusCodeGroup == 5)
-                metrics.reportFailure(type, DocumentOperationStatus.SERVER_ERROR);
+            switch (response.getStatus()) {
+                case 200 -> report(DocumentOperationStatus.OK);
+                case 400 -> report(DocumentOperationStatus.REQUEST_ERROR);
+                case 404 -> report(DocumentOperationStatus.OK, DocumentOperationStatus.NOT_FOUND);
+                case 412 -> report(DocumentOperationStatus.OK, DocumentOperationStatus.CONDITION_FAILED);
+                case 429 -> report(DocumentOperationStatus.TOO_MANY_REQUESTS);
+                case 500,502,503,504,507 -> report(DocumentOperationStatus.SERVER_ERROR);
+                default -> throw new IllegalStateException("Unexpected status code '%s'".formatted(response.getStatus()));
+            }
             return delegate.handleResponse(response);
         }
 
+        private void report(DocumentOperationStatus... status) { metrics.report(type, start, status); }
+
     }
 
     static class StorageCluster {
diff --git a/vespaclient-container-plugin/src/main/java/com/yahoo/documentapi/metrics/DocumentApiMetrics.java b/vespaclient-container-plugin/src/main/java/com/yahoo/documentapi/metrics/DocumentApiMetrics.java
index 890f06dd094..f1335351ac6 100644
--- a/vespaclient-container-plugin/src/main/java/com/yahoo/documentapi/metrics/DocumentApiMetrics.java
+++ b/vespaclient-container-plugin/src/main/java/com/yahoo/documentapi/metrics/DocumentApiMetrics.java
@@ -10,6 +10,7 @@ import com.yahoo.metrics.simple.Point;
 
 import java.time.Duration;
 import java.time.Instant;
+import java.util.EnumSet;
 import java.util.HashMap;
 import java.util.Map;
 import java.util.concurrent.ExecutionException;
@@ -52,16 +53,22 @@ public class DocumentApiMetrics {
         feeds.add(point);
     }
 
-    public void reportSuccessful(DocumentOperationType documentOperationType, Instant startTime) {
-        double latency = Duration.between(startTime, Instant.now()).toMillis() / 1000.0d;
-        reportSuccessful(documentOperationType, latency);
-    }
-
     public void reportFailure(DocumentOperationType documentOperationType, DocumentOperationStatus documentOperationStatus) {
         Point point = points.get(documentOperationStatus).get(documentOperationType);
         feeds.add(point);
     }
 
+    public void report(DocumentOperationType type, Instant start, DocumentOperationStatus... status) {
+        double latency = latency(start);
+        for (var s : status) {
+            var point = points.get(s).get(type);
+            feedLatency.sample(latency, point);
+            feeds.add(point);
+        }
+    }
+
+    private static double latency(Instant start) { return Duration.between(start, Instant.now()).toMillis() / 1000d; }
+
     public void reportHttpRequest(String clientVersion) {
         if (clientVersion != null) {
             try {
diff --git a/vespaclient-container-plugin/src/main/java/com/yahoo/documentapi/metrics/DocumentOperationStatus.java b/vespaclient-container-plugin/src/main/java/com/yahoo/documentapi/metrics/DocumentOperationStatus.java
index 97e5e9e63d8..f22eedeb5bc 100644
--- a/vespaclient-container-plugin/src/main/java/com/yahoo/documentapi/metrics/DocumentOperationStatus.java
+++ b/vespaclient-container-plugin/src/main/java/com/yahoo/documentapi/metrics/DocumentOperationStatus.java
@@ -17,7 +17,7 @@ import java.util.Set;
  */
 public enum DocumentOperationStatus {
 
-    OK, REQUEST_ERROR, SERVER_ERROR;
+    OK, REQUEST_ERROR, SERVER_ERROR, CONDITION_FAILED, NOT_FOUND, TOO_MANY_REQUESTS;
 
     public static DocumentOperationStatus fromMessageBusErrorCodes(Set<Integer> errorCodes) {
         if (errorCodes.size() == 1 && errorCodes.contains(DocumentProtocol.ERROR_NO_SPACE))
diff --git a/vespaclient-container-plugin/src/test/java/com/yahoo/document/restapi/resource/DocumentV1ApiTest.java b/vespaclient-container-plugin/src/test/java/com/yahoo/document/restapi/resource/DocumentV1ApiTest.java
index 973d0a98b24..ea143b7e480 100644
--- a/vespaclient-container-plugin/src/test/java/com/yahoo/document/restapi/resource/DocumentV1ApiTest.java
+++ b/vespaclient-container-plugin/src/test/java/com/yahoo/document/restapi/resource/DocumentV1ApiTest.java
@@ -845,7 +845,7 @@ public class DocumentV1ApiTest {
                 handler.get().handleResponse(new Response(0));  // response may eventually arrive, but too late.
         }
 
-        assertEquals(3, metric.metrics().get("httpapi_succeeded").get(Map.of()), 0);
+        assertEquals(5, metric.metrics().get("httpapi_succeeded").get(Map.of()), 0);
         assertEquals(1, metric.metrics().get("httpapi_condition_not_met").get(Map.of()), 0);
         assertEquals(1, metric.metrics().get("httpapi_not_found").get(Map.of()), 0);
         assertEquals(1, metric.metrics().get("httpapi_failed").get(Map.of()), 0);