refactor before writing tests

open-telemetry · Nov 11, 2020 · d7ff8d4 · d7ff8d4
1 parent 9b59559
commit d7ff8d4
Show file tree

Hide file tree

Showing 3 changed files with 94 additions and 68 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
@@ -0,0 +1,83 @@
+/*
+ * Copyright The OpenTelemetry Authors
+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+package io.opentelemetry.sdk.metrics;
+
+import io.opentelemetry.sdk.metrics.view.AggregationConfiguration;
+import io.opentelemetry.sdk.metrics.view.Aggregations;
+import io.opentelemetry.sdk.metrics.view.InstrumentSelector;
+import java.util.Map;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.regex.Pattern;
+
+class AggregationChooser {
+  private static final AggregationConfiguration CUMULATIVE_SUM =
+      AggregationConfiguration.create(
+          Aggregations.sum(), AggregationConfiguration.Temporality.CUMULATIVE);
+  private static final AggregationConfiguration DELTA_SUMMARY =
+      AggregationConfiguration.create(
+          Aggregations.minMaxSumCount(), AggregationConfiguration.Temporality.DELTA);
+  private static final AggregationConfiguration CUMULATIVE_LAST_VALUE =
+      AggregationConfiguration.create(
+          Aggregations.lastValue(), AggregationConfiguration.Temporality.CUMULATIVE);
+  private static final AggregationConfiguration DELTA_LAST_VALUE =
+      AggregationConfiguration.create(
+          Aggregations.lastValue(), AggregationConfiguration.Temporality.DELTA);
+
+  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) {
+
+    for (Map.Entry<InstrumentSelector, AggregationConfiguration> entry : configuration.entrySet()) {
+      InstrumentSelector registeredSelector = entry.getKey();
+
+      if (matchesOnType(descriptor, registeredSelector)
+          && matchesOnName(descriptor, registeredSelector)) {
+        return entry.getValue();
+      }
+    }
+
+    // If none found, use the defaults:
+    return getDefaultSpecification(descriptor);
+  }
+
+  private static boolean matchesOnName(
+      InstrumentDescriptor descriptor, InstrumentSelector registeredSelector) {
+    Pattern pattern = registeredSelector.instrumentNamePattern();
+    if (pattern == null) {
+      return true;
+    }
+    return pattern.matcher(descriptor.getName()).matches();
+  }
+
+  private static AggregationConfiguration getDefaultSpecification(InstrumentDescriptor descriptor) {
+    switch (descriptor.getType()) {
+      case COUNTER:
+      case UP_DOWN_COUNTER:
+        return CUMULATIVE_SUM;
+      case VALUE_RECORDER:
+        return DELTA_SUMMARY;
+      case VALUE_OBSERVER:
+        return DELTA_LAST_VALUE;
+      case SUM_OBSERVER:
+      case UP_DOWN_SUM_OBSERVER:
+        return CUMULATIVE_LAST_VALUE;
+    }
+    throw new IllegalArgumentException("Unknown descriptor type: " + descriptor.getType());
+  }
+
+  void addView(InstrumentSelector selector, AggregationConfiguration specification) {
+    configuration.put(selector, specification);
+  }
+}
diff --git a/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/ViewRegistry.java b/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/ViewRegistry.java
@@ -8,11 +8,7 @@
 import io.opentelemetry.sdk.metrics.view.Aggregation;
 import io.opentelemetry.sdk.metrics.view.AggregationConfiguration;
 import io.opentelemetry.sdk.metrics.view.AggregationConfiguration.Temporality;
-import io.opentelemetry.sdk.metrics.view.Aggregations;
 import io.opentelemetry.sdk.metrics.view.InstrumentSelector;
-import java.util.Map;
-import java.util.concurrent.ConcurrentHashMap;
-import java.util.regex.Pattern;
 
 // notes:
 //  specify by pieces of the descriptor.
@@ -33,17 +29,16 @@
  */
 class ViewRegistry {
 
-  private static final AggregationConfiguration CUMULATIVE_SUM =
-      AggregationConfiguration.create(Aggregations.sum(), Temporality.CUMULATIVE);
-  private static final AggregationConfiguration DELTA_SUMMARY =
-      AggregationConfiguration.create(Aggregations.minMaxSumCount(), Temporality.DELTA);
-  private static final AggregationConfiguration CUMULATIVE_LAST_VALUE =
-      AggregationConfiguration.create(Aggregations.lastValue(), Temporality.CUMULATIVE);
-  private static final AggregationConfiguration DELTA_LAST_VALUE =
-      AggregationConfiguration.create(Aggregations.lastValue(), Temporality.DELTA);
+  private final AggregationChooser aggregationChooser;
 
-  private final Map<InstrumentSelector, AggregationConfiguration> configuration =
-      new ConcurrentHashMap<>();
+  ViewRegistry() {
+    this(new AggregationChooser());
+  }
+
+  // VisibleForTesting
+  ViewRegistry(AggregationChooser aggregationChooser) {
+    this.aggregationChooser = aggregationChooser;
+  }
 
   /**
    * Create a new {@link io.opentelemetry.sdk.metrics.Batcher} for use in metric recording
@@ -54,7 +49,7 @@ Batcher createBatcher(
       MeterSharedState meterSharedState,
       InstrumentDescriptor descriptor) {
 
-    AggregationConfiguration specification = findBestMatch(descriptor);
+    AggregationConfiguration specification = aggregationChooser.chooseAggregation(descriptor);
 
     Aggregation aggregation = specification.aggregation();
 
@@ -68,56 +63,7 @@ Batcher createBatcher(
     throw new IllegalStateException("unsupported Temporality: " + specification.temporality());
   }
 
-  // todo: consider moving this method to its own class, for more targeted testing.
-  private AggregationConfiguration findBestMatch(InstrumentDescriptor descriptor) {
-
-    for (Map.Entry<InstrumentSelector, AggregationConfiguration> entry : configuration.entrySet()) {
-      InstrumentSelector registeredSelector = entry.getKey();
-
-      if (matchesOnType(descriptor, registeredSelector)
-          && matchesOnName(descriptor, registeredSelector)) {
-        return entry.getValue();
-      }
-    }
-
-    // If none found, use the defaults:
-    return getDefaultSpecification(descriptor);
-  }
-
-  private static boolean matchesOnType(
-      InstrumentDescriptor descriptor, InstrumentSelector registeredSelector) {
-    if (registeredSelector.instrumentType() == null) {
-      return true;
-    }
-    return registeredSelector.instrumentType().equals(descriptor.getType());
-  }
-
-  private static boolean matchesOnName(
-      InstrumentDescriptor descriptor, InstrumentSelector registeredSelector) {
-    Pattern pattern = registeredSelector.instrumentNamePattern();
-    if (pattern == null) {
-      return true;
-    }
-    return pattern.matcher(descriptor.getName()).matches();
-  }
-
-  private static AggregationConfiguration getDefaultSpecification(InstrumentDescriptor descriptor) {
-    switch (descriptor.getType()) {
-      case COUNTER:
-      case UP_DOWN_COUNTER:
-        return CUMULATIVE_SUM;
-      case VALUE_RECORDER:
-        return DELTA_SUMMARY;
-      case VALUE_OBSERVER:
-        return DELTA_LAST_VALUE;
-      case SUM_OBSERVER:
-      case UP_DOWN_SUM_OBSERVER:
-        return CUMULATIVE_LAST_VALUE;
-    }
-    throw new IllegalArgumentException("Unknown descriptor type: " + descriptor.getType());
-  }
-
   void registerView(InstrumentSelector selector, AggregationConfiguration specification) {
-    configuration.put(selector, specification);
+    aggregationChooser.addView(selector, specification);
   }
 }
diff --git a/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/view/AggregationConfiguration.java b/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/view/AggregationConfiguration.java
@@ -7,7 +7,6 @@
 
 import com.google.auto.value.AutoValue;
 import io.opentelemetry.api.metrics.Instrument;
-import javax.annotation.Nullable;
 import javax.annotation.concurrent.Immutable;
 
 @AutoValue
@@ -19,11 +18,9 @@ public static AggregationConfiguration create(Aggregation aggregation, Temporali
   }
 
   /** Which {@link Aggregation} should be used for this View. */
-  @Nullable
   public abstract Aggregation aggregation();
 
   /** What {@link Temporality} should be used for this View (delta vs. cumulative). */
-  @Nullable
   public abstract Temporality temporality();
 
   /** An enumeration which describes the time period over which metrics should be aggregated. */