Replace dimension allowlisting with blocklist in /metrics/v2

7 files changed, 88 insertions, 68 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..639f642778e 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,53 +13,23 @@ 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 = getBlocklistDimensions();
 
     @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> getBlocklistDimensions() {
+        return toDimensionIds(BlocklistDimensions.blocklistDimensions);
     }
 
     static Set<DimensionId> toDimensionIds(Collection<String> dimensionNames) {
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..5e72594759a
--- /dev/null
+++ b/metrics-proxy/src/main/java/ai/vespa/metricsproxy/metric/dimensions/BlocklistDimensions.java
@@ -0,0 +1,73 @@
+package ai.vespa.metricsproxy.metric.dimensions;
+
+import java.util.ArrayList;
+import java.util.List;
+
+/**
+ * @author olaa
+ */
+public class BlocklistDimensions {
+
+    private BlocklistDimensions() {}
+
+    /**
+     * Deployment related metrics - most of which are redundant
+     * E.g. app/applicationName/tenantName/instanceName is already included in applicationId
+     */
+    public static final String APP = "app";
+    public static final String APPLICATION_NAME = "applicationName";
+    public static final String CLUSTER_NAME = "clustername";
+    public static final String CLUSTER_ID = "clusterid";
+    public static final String CLUSTER_TYPE = "clustertype";
+    public static final String DEPLOYMENT_CLUSTER = "deploymentCluster";
+    public static final String GROUP_ID = "groupId";
+    public static final String INSTANCE = "instance";
+    public static final String INSTANCE_NAME = "instanceName";
+    public static final String TENANT_NAME = "tenantName";
+
+    /**
+     * State related dimensions - will always be the same value for a given snapshot
+     */
+    public static final String METRIC_TYPE = "metrictype";
+    public static final String ORCHESTRATOR_STATE = "orchestratorState";
+    public static final String ROLE = "role";
+    public static final String STATE = "state";
+    public static final String SYSTEM = "system";
+    public static final String VESPA_VERSION = "vespaVersion";
+
+    /**  Metric specific dimensions  **/
+    public static final String ARCHITECTURE = "arch";
+    public static final String AUTHZ_REQUIRED = "authz-required";
+    public static final String HOME = "home";
+    public static final String PORT = "port";
+    public static final String SCHEME = "scheme";
+    public static final String DRYRUN = "dryrun";
+    public static final String VERSION = "version";
+
+    public static final List<String> blocklistDimensions = List.of(
+            APP,
+            APPLICATION_NAME,
+            CLUSTER_NAME,
+            CLUSTER_ID,
+            CLUSTER_TYPE,
+            DEPLOYMENT_CLUSTER,
+            GROUP_ID,
+            INSTANCE,
+            INSTANCE_NAME,
+            TENANT_NAME,
+            METRIC_TYPE,
+            ORCHESTRATOR_STATE,
+            ROLE,
+            STATE,
+            SYSTEM,
+            VESPA_VERSION,
+            ARCHITECTURE,
+            AUTHZ_REQUIRED,
+            HOME,
+            PORT,
+            SCHEME,
+            DRYRUN,
+            VERSION
+    );
+
+}
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"
           }
         }
       ]