Merge pull request #25868 from vespa-engine/olaa/metrics-v2-blocklist-dimensions

Replace dimension allowlisting with blocklist in /metrics/v2

7 files changed, 81 insertions, 76 deletions

diff --git a/metrics-proxy/src/main/java/ai/vespa/metricsproxy/http/application/NodeMetricsClient.java b/metrics-proxy/src/main/java/ai/vespa/metricsproxy/http/application/NodeMetricsClient.java
index 3f93cdda61c..1d7f7000c5a 100644
--- a/metrics-proxy/src/main/java/ai/vespa/metricsproxy/http/application/NodeMetricsClient.java
+++ b/metrics-proxy/src/main/java/ai/vespa/metricsproxy/http/application/NodeMetricsClient.java
@@ -41,8 +41,6 @@ public class NodeMetricsClient {
 
     private static final Logger log = Logger.getLogger(NodeMetricsClient.class.getName());
 
-    private static final int MAX_DIMENSIONS = 10;
-
     final Node node;
     private final CloseableHttpAsyncClient httpClient;
     private final Clock clock;
@@ -89,7 +87,7 @@ public class NodeMetricsClient {
         var metrics = processAndBuild(GenericJsonUtil.toMetricsPackets(respons),
                 new ServiceIdDimensionProcessor(),
                 new ClusterIdDimensionProcessor(),
-                new PublicDimensionsProcessor(MAX_DIMENSIONS));
+                new PublicDimensionsProcessor());
         snapshotsRetrieved.incrementAndGet();
         log.log(FINE, () -> "Successfully retrieved " + metrics.size() + " metrics packets from " + metricsUri);
         snapshots.put(consumer, new Snapshot(Instant.now(clock), metrics));
diff --git a/metrics-proxy/src/main/java/ai/vespa/metricsproxy/http/application/PublicDimensionsProcessor.java b/metrics-proxy/src/main/java/ai/vespa/metricsproxy/http/application/PublicDimensionsProcessor.java
index 50eb74e8693..1ade79521f2 100644
--- a/metrics-proxy/src/main/java/ai/vespa/metricsproxy/http/application/PublicDimensionsProcessor.java
+++ b/metrics-proxy/src/main/java/ai/vespa/metricsproxy/http/application/PublicDimensionsProcessor.java
@@ -1,6 +1,7 @@
 // Copyright Yahoo. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
 package ai.vespa.metricsproxy.http.application;
 
+import ai.vespa.metricsproxy.metric.dimensions.BlocklistDimensions;
 import ai.vespa.metricsproxy.metric.dimensions.PublicDimensions;
 import ai.vespa.metricsproxy.metric.model.DimensionId;
 import ai.vespa.metricsproxy.metric.model.MetricsPacket;
@@ -12,59 +13,19 @@ import java.util.Set;
 import java.util.stream.Collectors;
 
 /**
- * Ensures that only whitelisted dimensions are retained in the given metrics packet, and that
- * there are no more dimensions than the given maximum number.
+ * Ensures that blocklisted dimensions are removed from the metric set
  *
  * @author gjoranv
  */
 public class PublicDimensionsProcessor implements MetricsProcessor {
 
-    private final int maxDimensions;
-    private Set<DimensionId> publicDimensions = getPublicDimensions();
-
-    public PublicDimensionsProcessor(int maxDimensions) {
-        int numCommonDimensions = PublicDimensions.commonDimensions.size();
-        if (numCommonDimensions > maxDimensions) {
-            throw new IllegalArgumentException(String.format(
-                    ("The maximum number of dimensions (%d) cannot be smaller than the number of " +
-                             "common metrics dimensions (%d)."), maxDimensions, numCommonDimensions));
-        }
-        this.maxDimensions = maxDimensions;
-    }
+    private final Set<DimensionId> blocklistDimensions = BlocklistDimensions.getAll();
 
     @Override
     public void process(MetricsPacket.Builder builder) {
         Set<DimensionId> dimensionsToRetain = builder.getDimensionIds();
-        dimensionsToRetain.retainAll(publicDimensions);
-
-        if (dimensionsToRetain.size() > maxDimensions) {
-            for (var metricDim : getMetricDimensions()) {
-                dimensionsToRetain.remove(metricDim);
-                if (dimensionsToRetain.size() <= maxDimensions) break;
-            }
-        }
-
+        dimensionsToRetain.removeAll(blocklistDimensions);
         builder.retainDimensions(dimensionsToRetain);
-
-        // Extra safeguard, to make sure we don't exceed the limit of some metric systems.
-        if (builder.getDimensionIds().size() > maxDimensions) {
-            throw new IllegalStateException(String.format(
-                    "Metrics packet is only allowed to have %d dimensions, but has: %s", maxDimensions, builder.getDimensionIds()));
-        }
     }
 
-    static Set<DimensionId> getPublicDimensions() {
-        return toDimensionIds(PublicDimensions.publicDimensions);
-    }
-
-    static Set<DimensionId> getMetricDimensions() {
-        return toDimensionIds(PublicDimensions.metricDimensions);
-    }
-
-    static Set<DimensionId> toDimensionIds(Collection<String> dimensionNames) {
-        return dimensionNames.stream()
-                .map(DimensionId::toDimensionId)
-                .collect(Collectors.toCollection(LinkedHashSet::new));
-
-    }
 }
diff --git a/metrics-proxy/src/main/java/ai/vespa/metricsproxy/http/metrics/MetricsV2Handler.java b/metrics-proxy/src/main/java/ai/vespa/metricsproxy/http/metrics/MetricsV2Handler.java
index 339f06c67b8..1aa7244363b 100644
--- a/metrics-proxy/src/main/java/ai/vespa/metricsproxy/http/metrics/MetricsV2Handler.java
+++ b/metrics-proxy/src/main/java/ai/vespa/metricsproxy/http/metrics/MetricsV2Handler.java
@@ -40,7 +40,6 @@ public class MetricsV2Handler  extends HttpHandlerBase {
 
     public static final String V2_PATH = "/metrics/v2";
     public static final String VALUES_PATH = V2_PATH + "/values";
-    private static final int MAX_DIMENSIONS = 10;
 
     private final ValuesFetcher valuesFetcher;
     private final NodeInfoConfig nodeInfoConfig;
@@ -68,7 +67,7 @@ public class MetricsV2Handler  extends HttpHandlerBase {
             List<MetricsPacket> metrics = processAndBuild(valuesFetcher.fetchMetricsAsBuilders(consumer),
                                                           new ServiceIdDimensionProcessor(),
                                                           new ClusterIdDimensionProcessor(),
-                                                          new PublicDimensionsProcessor(MAX_DIMENSIONS));
+                                                          new PublicDimensionsProcessor());
 
             Node localNode = new Node(nodeInfoConfig.role(), nodeInfoConfig.hostname(), 0, "");
             Map<Node, List<MetricsPacket>> metricsByNode = Map.of(localNode, metrics);
diff --git a/metrics-proxy/src/main/java/ai/vespa/metricsproxy/metric/dimensions/BlocklistDimensions.java b/metrics-proxy/src/main/java/ai/vespa/metricsproxy/metric/dimensions/BlocklistDimensions.java
new file mode 100644
index 00000000000..964a7fbf7c8
--- /dev/null
+++ b/metrics-proxy/src/main/java/ai/vespa/metricsproxy/metric/dimensions/BlocklistDimensions.java
@@ -0,0 +1,68 @@
+// Copyright Yahoo. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
+package ai.vespa.metricsproxy.metric.dimensions;
+
+import ai.vespa.metricsproxy.metric.model.DimensionId;
+
+import java.util.EnumSet;
+import java.util.LinkedHashSet;
+import java.util.List;
+import java.util.Set;
+import java.util.stream.Collectors;
+
+/**
+ * @author olaa
+ */
+public enum BlocklistDimensions {
+
+    /**
+     * Deployment related metrics - most of which are redundant
+     * E.g. app/applicationName/tenantName/instanceName is already included in applicationId
+     */
+    APP("app"),
+    APPLICATION_NAME("applicationName"),
+    CLUSTER_NAME("clustername"),
+    CLUSTER_ID("clusterid"),
+    CLUSTER_TYPE("clustertype"),
+    DEPLOYMENT_CLUSTER("deploymentCluster"),
+    GROUP_ID("groupId"),
+    INSTANCE("instance"),
+    INSTANCE_NAME("instanceName"),
+    TENANT_NAME("tenantName"),
+
+    /**
+     * State related dimensions - will always be the same value for a given snapshot
+     */
+    METRIC_TYPE("metrictype"),
+    ORCHESTRATOR_STATE("orchestratorState"),
+    ROLE("role"),
+    STATE("state"),
+    SYSTEM("system"),
+    VESPA_VERSION("vespaVersion"),
+
+    /**  Metric specific dimensions  **/
+    ARCHITECTURE("arch"),
+    AUTHZ_REQUIRED("authz-equired"),
+    HOME("home"),
+    PORT("port"),
+    SCHEME("scheme"),
+    DRYRUN("dryrun"),
+    VERSION("version");
+
+    private final DimensionId dimensionId;
+
+    BlocklistDimensions(String dimensionId) {
+        this.dimensionId = DimensionId.toDimensionId(dimensionId);
+    }
+
+    public DimensionId getDimensionId() {
+        return dimensionId;
+    }
+
+    public static Set<DimensionId> getAll() {
+        return EnumSet.allOf(BlocklistDimensions.class)
+                .stream()
+                .map(BlocklistDimensions::getDimensionId)
+                .collect(Collectors.toSet());
+    }
+
+}
diff --git a/metrics-proxy/src/test/java/ai/vespa/metricsproxy/http/application/ApplicationMetricsHandlerTest.java b/metrics-proxy/src/test/java/ai/vespa/metricsproxy/http/application/ApplicationMetricsHandlerTest.java
index d6b084acbb3..7cc51d7b46b 100644
--- a/metrics-proxy/src/test/java/ai/vespa/metricsproxy/http/application/ApplicationMetricsHandlerTest.java
+++ b/metrics-proxy/src/test/java/ai/vespa/metricsproxy/http/application/ApplicationMetricsHandlerTest.java
@@ -181,14 +181,14 @@ public class ApplicationMetricsHandlerTest {
 
         GenericService searchnode = jsonModel.nodes.get(0).services.get(0);
         Map<String, String> dimensions = searchnode.metrics.get(0).dimensions;
-        assertEquals(6, dimensions.size());
+        assertEquals(7, dimensions.size());
         assertEquals("music.default", dimensions.get(PublicDimensions.APPLICATION_ID));
         assertEquals("container/default", dimensions.get(PublicDimensions.CLUSTER_ID));
         assertEquals("us-west", dimensions.get(PublicDimensions.ZONE));
         assertEquals("search/", dimensions.get(PublicDimensions.API));
         assertEquals("music", dimensions.get(PublicDimensions.DOCUMENT_TYPE));
         assertEquals("default0", dimensions.get(PublicDimensions.SERVICE_ID));
-        assertFalse(dimensions.containsKey("non-public"));
+        assertFalse(dimensions.containsKey("clusterid"));
     }
 
     @Test
diff --git a/metrics-proxy/src/test/java/ai/vespa/metricsproxy/http/application/PublicDimensionsProcessorTest.java b/metrics-proxy/src/test/java/ai/vespa/metricsproxy/http/application/PublicDimensionsProcessorTest.java
index 68368d28fdf..e155b444580 100644
--- a/metrics-proxy/src/test/java/ai/vespa/metricsproxy/http/application/PublicDimensionsProcessorTest.java
+++ b/metrics-proxy/src/test/java/ai/vespa/metricsproxy/http/application/PublicDimensionsProcessorTest.java
@@ -4,11 +4,7 @@ package ai.vespa.metricsproxy.http.application;
 import ai.vespa.metricsproxy.metric.model.MetricsPacket;
 import org.junit.Test;
 
-import static ai.vespa.metricsproxy.http.application.PublicDimensionsProcessor.getPublicDimensions;
-import static ai.vespa.metricsproxy.http.application.PublicDimensionsProcessor.toDimensionIds;
 import static ai.vespa.metricsproxy.metric.dimensions.PublicDimensions.APPLICATION_ID;
-import static ai.vespa.metricsproxy.metric.dimensions.PublicDimensions.commonDimensions;
-import static ai.vespa.metricsproxy.metric.dimensions.PublicDimensions.publicDimensions;
 import static ai.vespa.metricsproxy.metric.model.DimensionId.toDimensionId;
 import static ai.vespa.metricsproxy.metric.model.ServiceId.toServiceId;
 import static org.junit.Assert.assertEquals;
@@ -20,11 +16,11 @@ import static org.junit.Assert.assertTrue;
 public class PublicDimensionsProcessorTest {
 
     @Test
-    public void non_public_dimensions_are_removed() {
+    public void blocklisted_dimensions_are_removed() {
         var builder = new MetricsPacket.Builder(toServiceId("foo"))
-                .putDimension(toDimensionId("a"), "");
+                .putDimension(toDimensionId("applicationName"), "");
 
-        var processor = new PublicDimensionsProcessor(10);
+        var processor = new PublicDimensionsProcessor();
         processor.process(builder);
         assertTrue(builder.getDimensionIds().isEmpty());
     }
@@ -34,27 +30,10 @@ public class PublicDimensionsProcessorTest {
         var builder = new MetricsPacket.Builder(toServiceId("foo"))
                 .putDimension(toDimensionId(APPLICATION_ID), "app");
 
-        var processor = new PublicDimensionsProcessor(10);
+        var processor = new PublicDimensionsProcessor();
         processor.process(builder);
         assertEquals(1, builder.getDimensionIds().size());
         assertEquals(toDimensionId(APPLICATION_ID), builder.getDimensionIds().iterator().next());
     }
 
-    @Test
-    public void common_dimensions_have_priority_when_there_are_too_many() {
-        var builder = new MetricsPacket.Builder(toServiceId("foo"));
-        getPublicDimensions()
-                .forEach(dimensionId -> builder.putDimension(dimensionId, dimensionId.id));
-        assertEquals(publicDimensions.size(), builder.getDimensionIds().size());
-
-        var processor = new PublicDimensionsProcessor(commonDimensions.size());
-        processor.process(builder);
-
-        var includedDimensions = builder.getDimensionIds();
-        assertEquals(commonDimensions.size(), includedDimensions.size());
-
-        toDimensionIds(commonDimensions).forEach(commonDimension ->
-                assertTrue(includedDimensions.contains(commonDimension)));
-    }
-
 }
diff --git a/metrics-proxy/src/test/resources/generic-sample.json b/metrics-proxy/src/test/resources/generic-sample.json
index 13302121507..b562ccb13d3 100644
--- a/metrics-proxy/src/test/resources/generic-sample.json
+++ b/metrics-proxy/src/test/resources/generic-sample.json
@@ -40,7 +40,7 @@
             "zone": "us-west",
             "api": "search/",
             "documenttype": "music",
-            "non-public": "will-be-removed"
+            "custom-dimension": "will-not-be-removed"
           }
         }
       ]