tests for the AggregationChooser and a bugfix they uncovered

open-telemetry · Nov 12, 2020 · 9ff19d1 · 9ff19d1
1 parent d7ff8d4
commit 9ff19d1
Show file tree

Hide file tree

Showing 4 changed files with 248 additions and 15 deletions.
diff --git a/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/AggregationChooser.java b/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/AggregationChooser.java
@@ -8,6 +8,8 @@
 import io.opentelemetry.sdk.metrics.view.AggregationConfiguration;
 import io.opentelemetry.sdk.metrics.view.Aggregations;
 import io.opentelemetry.sdk.metrics.view.InstrumentSelector;
+import java.util.ArrayList;
+import java.util.List;
 import java.util.Map;
 import java.util.concurrent.ConcurrentHashMap;
 import java.util.regex.Pattern;
@@ -29,34 +31,53 @@ class AggregationChooser {
   private final Map<InstrumentSelector, AggregationConfiguration> configuration =
       new ConcurrentHashMap<>();
 
-  private static boolean matchesOnType(
-      InstrumentDescriptor descriptor, InstrumentSelector registeredSelector) {
-    if (registeredSelector.instrumentType() == null) {
-      return true;
-    }
-    return registeredSelector.instrumentType().equals(descriptor.getType());
-  }
-
   AggregationConfiguration chooseAggregation(InstrumentDescriptor descriptor) {
-
+    List<Map.Entry<InstrumentSelector, AggregationConfiguration>> possibleMatches =
+        new ArrayList<>();
     for (Map.Entry<InstrumentSelector, AggregationConfiguration> entry : configuration.entrySet()) {
       InstrumentSelector registeredSelector = entry.getKey();
-
+      // if it matches everything, return it right away...
       if (matchesOnType(descriptor, registeredSelector)
           && matchesOnName(descriptor, registeredSelector)) {
         return entry.getValue();
       }
+      // otherwise throw it into a bucket of possible matches if it matches one of the criteria
+      if (matchesOne(descriptor, registeredSelector)) {
+        possibleMatches.add(entry);
+      }
+    }
+
+    if (possibleMatches.isEmpty()) {
+      return getDefaultSpecification(descriptor);
     }
 
-    // If none found, use the defaults:
-    return getDefaultSpecification(descriptor);
+    // If no exact matches found, pick the first one that matches something:
+    return possibleMatches.get(0).getValue();
+  }
+
+  private boolean matchesOne(InstrumentDescriptor descriptor, InstrumentSelector selector) {
+    if (selector.hasNameRegex() && !matchesOnName(descriptor, selector)) {
+      return false;
+    }
+    if (selector.hasType() && !matchesOnType(descriptor, selector)) {
+      return false;
+    }
+    return true;
+  }
+
+  private static boolean matchesOnType(
+      InstrumentDescriptor descriptor, InstrumentSelector selector) {
+    if (selector.instrumentType() == null) {
+      return false;
+    }
+    return selector.instrumentType().equals(descriptor.getType());
   }
 
   private static boolean matchesOnName(
       InstrumentDescriptor descriptor, InstrumentSelector registeredSelector) {
     Pattern pattern = registeredSelector.instrumentNamePattern();
     if (pattern == null) {
-      return true;
+      return false;
     }
     return pattern.matcher(descriptor.getName()).matches();
   }

diff --git a/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/view/Aggregations.java b/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/view/Aggregations.java
@@ -99,6 +99,11 @@ public boolean availableForInstrument(InstrumentType instrumentType) {
       return instrumentType == InstrumentType.VALUE_OBSERVER
           || instrumentType == InstrumentType.VALUE_RECORDER;
     }
+
+    @Override
+    public String toString() {
+      return getClass().getSimpleName();
+    }
   }
 
   @Immutable
@@ -142,6 +147,11 @@ public boolean availableForInstrument(InstrumentType instrumentType) {
       // Available for all instruments.
       return true;
     }
+
+    @Override
+    public String toString() {
+      return getClass().getSimpleName();
+    }
   }
 
   @Immutable
@@ -170,6 +180,11 @@ public boolean availableForInstrument(InstrumentType instrumentType) {
       // Available for all instruments.
       return true;
     }
+
+    @Override
+    public String toString() {
+      return getClass().getSimpleName();
+    }
   }
 
   @Immutable
@@ -201,6 +216,11 @@ public String getUnit(String initialUnit) {
     public boolean availableForInstrument(InstrumentType instrumentType) {
       throw new UnsupportedOperationException("Implement this");
     }
+
+    @Override
+    public String toString() {
+      return getClass().getSimpleName();
+    }
   }
 
   @Immutable
@@ -248,6 +268,11 @@ public boolean availableForInstrument(InstrumentType instrumentType) {
       return instrumentType == InstrumentType.SUM_OBSERVER
           || instrumentType == InstrumentType.UP_DOWN_SUM_OBSERVER;
     }
+
+    @Override
+    public String toString() {
+      return getClass().getSimpleName();
+    }
   }
 
   private Aggregations() {}

diff --git a/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/view/InstrumentSelector.java b/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/view/InstrumentSelector.java
@@ -15,7 +15,6 @@
 @AutoValue
 @Immutable
 public abstract class InstrumentSelector {
-
   public static Builder newBuilder() {
     return new AutoValue_InstrumentSelector.Builder();
   }
@@ -26,12 +25,20 @@ public static Builder newBuilder() {
   @Nullable
   public abstract String instrumentNameRegex();
 
-  @Memoized
   @Nullable
+  @Memoized
   public Pattern instrumentNamePattern() {
     return instrumentNameRegex() == null ? null : Pattern.compile(instrumentNameRegex());
   }
 
+  public boolean hasType() {
+    return instrumentType() != null;
+  }
+
+  public boolean hasNameRegex() {
+    return instrumentNameRegex() != null;
+  }
+
   @AutoValue.Builder
   public interface Builder {
     Builder instrumentType(InstrumentType instrumentType);

diff --git a/sdk/metrics/src/test/java/io/opentelemetry/sdk/metrics/AggregationChooserTest.java b/sdk/metrics/src/test/java/io/opentelemetry/sdk/metrics/AggregationChooserTest.java
@@ -0,0 +1,180 @@
+/*
+ * Copyright The OpenTelemetry Authors
+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+package io.opentelemetry.sdk.metrics;
+
+import static org.assertj.core.api.Assertions.assertThat;
+
+import io.opentelemetry.sdk.metrics.common.InstrumentType;
+import io.opentelemetry.sdk.metrics.common.InstrumentValueType;
+import io.opentelemetry.sdk.metrics.view.AggregationConfiguration;
+import io.opentelemetry.sdk.metrics.view.Aggregations;
+import io.opentelemetry.sdk.metrics.view.InstrumentSelector;
+import org.junit.jupiter.api.Test;
+
+class AggregationChooserTest {
+
+  @Test
+  void selection_onType() {
+    AggregationConfiguration configuration =
+        AggregationConfiguration.create(
+            Aggregations.sum(), AggregationConfiguration.Temporality.DELTA);
+
+    AggregationChooser aggregationChooser = new AggregationChooser();
+    aggregationChooser.addView(
+        InstrumentSelector.newBuilder().instrumentType(InstrumentType.COUNTER).build(),
+        configuration);
+    assertThat(
+            aggregationChooser.chooseAggregation(
+                InstrumentDescriptor.create(
+                    "", "", "", InstrumentType.COUNTER, InstrumentValueType.LONG)))
+        .isEqualTo(configuration);
+    // this one hasn't been configured, so it gets the default still..
+    assertThat(
+            aggregationChooser.chooseAggregation(
+                InstrumentDescriptor.create(
+                    "", "", "", InstrumentType.UP_DOWN_COUNTER, InstrumentValueType.LONG)))
+        .isEqualTo(
+            AggregationConfiguration.create(
+                Aggregations.sum(), AggregationConfiguration.Temporality.CUMULATIVE));
+  }
+
+  @Test
+  void selection_onName() {
+    AggregationConfiguration configuration =
+        AggregationConfiguration.create(
+            Aggregations.sum(), AggregationConfiguration.Temporality.DELTA);
+
+    AggregationChooser aggregationChooser = new AggregationChooser();
+    aggregationChooser.addView(
+        InstrumentSelector.newBuilder().instrumentNameRegex("overridden").build(), configuration);
+    assertThat(
+            aggregationChooser.chooseAggregation(
+                InstrumentDescriptor.create(
+                    "overridden", "", "", InstrumentType.COUNTER, InstrumentValueType.LONG)))
+        .isEqualTo(configuration);
+    // this one hasn't been configured, so it gets the default still..
+    assertThat(
+            aggregationChooser.chooseAggregation(
+                InstrumentDescriptor.create(
+                    "default", "", "", InstrumentType.UP_DOWN_COUNTER, InstrumentValueType.LONG)))
+        .isEqualTo(
+            AggregationConfiguration.create(
+                Aggregations.sum(), AggregationConfiguration.Temporality.CUMULATIVE));
+  }
+
+  @Test
+  void selection_moreSpecificWins() {
+    AggregationConfiguration configuration1 =
+        AggregationConfiguration.create(
+            Aggregations.sum(), AggregationConfiguration.Temporality.DELTA);
+    AggregationConfiguration configuration2 =
+        AggregationConfiguration.create(
+            Aggregations.count(), AggregationConfiguration.Temporality.DELTA);
+
+    AggregationChooser aggregationChooser = new AggregationChooser();
+    aggregationChooser.addView(
+        InstrumentSelector.newBuilder()
+            .instrumentNameRegex("overridden")
+            .instrumentType(InstrumentType.COUNTER)
+            .build(),
+        configuration2);
+    aggregationChooser.addView(
+        InstrumentSelector.newBuilder().instrumentType(InstrumentType.COUNTER).build(),
+        configuration1);
+
+    assertThat(
+            aggregationChooser.chooseAggregation(
+                InstrumentDescriptor.create(
+                    "overridden", "", "", InstrumentType.COUNTER, InstrumentValueType.LONG)))
+        .isEqualTo(configuration2);
+    assertThat(
+            aggregationChooser.chooseAggregation(
+                InstrumentDescriptor.create(
+                    "default", "", "", InstrumentType.COUNTER, InstrumentValueType.LONG)))
+        .isEqualTo(configuration1);
+  }
+
+  @Test
+  void selection_regex() {
+    AggregationConfiguration configuration1 =
+        AggregationConfiguration.create(
+            Aggregations.sum(), AggregationConfiguration.Temporality.DELTA);
+
+    AggregationChooser aggregationChooser = new AggregationChooser();
+    aggregationChooser.addView(
+        InstrumentSelector.newBuilder()
+            .instrumentNameRegex("overrid(es|den)")
+            .instrumentType(InstrumentType.COUNTER)
+            .build(),
+        configuration1);
+
+    assertThat(
+            aggregationChooser.chooseAggregation(
+                InstrumentDescriptor.create(
+                    "overridden", "", "", InstrumentType.COUNTER, InstrumentValueType.LONG)))
+        .isEqualTo(configuration1);
+    assertThat(
+            aggregationChooser.chooseAggregation(
+                InstrumentDescriptor.create(
+                    "overrides", "", "", InstrumentType.COUNTER, InstrumentValueType.LONG)))
+        .isEqualTo(configuration1);
+    // this one hasn't been configured, so it gets the default still..
+    assertThat(
+            aggregationChooser.chooseAggregation(
+                InstrumentDescriptor.create(
+                    "default", "", "", InstrumentType.UP_DOWN_COUNTER, InstrumentValueType.LONG)))
+        .isEqualTo(
+            AggregationConfiguration.create(
+                Aggregations.sum(), AggregationConfiguration.Temporality.CUMULATIVE));
+  }
+
+  @Test
+  void defaults() {
+    AggregationChooser aggregationChooser = new AggregationChooser();
+    assertThat(
+            aggregationChooser.chooseAggregation(
+                InstrumentDescriptor.create(
+                    "", "", "", InstrumentType.COUNTER, InstrumentValueType.LONG)))
+        .isEqualTo(
+            AggregationConfiguration.create(
+                Aggregations.sum(), AggregationConfiguration.Temporality.CUMULATIVE));
+    assertThat(
+            aggregationChooser.chooseAggregation(
+                InstrumentDescriptor.create(
+                    "", "", "", InstrumentType.UP_DOWN_COUNTER, InstrumentValueType.LONG)))
+        .isEqualTo(
+            AggregationConfiguration.create(
+                Aggregations.sum(), AggregationConfiguration.Temporality.CUMULATIVE));
+    assertThat(
+            aggregationChooser.chooseAggregation(
+                InstrumentDescriptor.create(
+                    "", "", "", InstrumentType.VALUE_RECORDER, InstrumentValueType.LONG)))
+        .isEqualTo(
+            AggregationConfiguration.create(
+                Aggregations.minMaxSumCount(), AggregationConfiguration.Temporality.DELTA));
+    assertThat(
+            aggregationChooser.chooseAggregation(
+                InstrumentDescriptor.create(
+                    "", "", "", InstrumentType.SUM_OBSERVER, InstrumentValueType.LONG)))
+        .isEqualTo(
+            AggregationConfiguration.create(
+                Aggregations.lastValue(), AggregationConfiguration.Temporality.CUMULATIVE));
+    assertThat(
+            aggregationChooser.chooseAggregation(
+                InstrumentDescriptor.create(
+                    "", "", "", InstrumentType.VALUE_OBSERVER, InstrumentValueType.LONG)))
+        .isEqualTo(
+            AggregationConfiguration.create(
+                Aggregations.lastValue(), AggregationConfiguration.Temporality.DELTA));
+    assertThat(
+            aggregationChooser.chooseAggregation(
+                InstrumentDescriptor.create(
+                    "", "", "", InstrumentType.UP_DOWN_SUM_OBSERVER, InstrumentValueType.LONG)))
+        .isEqualTo(
+            AggregationConfiguration.create(
+                Aggregations.lastValue(), AggregationConfiguration.Temporality.CUMULATIVE));
+  }
+}