Make the ViewRegistry immutable and remove the mutating deprecated me…

…thod. (#3149) * remove a method deprecated in 1.1.0 * Make the ViewRegistry immutable. * set all the defaults view config via loop, rather than explicitly per instrument type.
open-telemetry · Apr 20, 2021 · 7e2c34a · 7e2c34a
1 parent ea3d795
commit 7e2c34a
Show file tree

Hide file tree

Showing 8 changed files with 138 additions and 128 deletions.
diff --git a/sdk/metrics/src/jmh/java/io/opentelemetry/sdk/metrics/TestSdk.java b/sdk/metrics/src/jmh/java/io/opentelemetry/sdk/metrics/TestSdk.java
@@ -9,7 +9,12 @@
 import io.opentelemetry.api.metrics.MeterProvider;
 import io.opentelemetry.sdk.common.InstrumentationLibraryInfo;
 import io.opentelemetry.sdk.internal.SystemClock;
+import io.opentelemetry.sdk.metrics.common.InstrumentType;
+import io.opentelemetry.sdk.metrics.view.View;
 import io.opentelemetry.sdk.resources.Resource;
+import java.util.EnumMap;
+import java.util.LinkedHashMap;
+import java.util.regex.Pattern;
 
 @SuppressWarnings("ImmutableEnumChecker")
 public enum TestSdk {
@@ -25,7 +30,12 @@ Meter build() {
         @Override
         Meter build() {
           MeterProviderSharedState meterProviderSharedState =
-              MeterProviderSharedState.create(SystemClock.getInstance(), Resource.empty());
+              MeterProviderSharedState.create(
+                  SystemClock.getInstance(),
+                  Resource.empty(),
+                  new ViewRegistry(
+                      new EnumMap<InstrumentType, LinkedHashMap<Pattern, View>>(
+                          InstrumentType.class)));
           InstrumentationLibraryInfo instrumentationLibraryInfo =
               InstrumentationLibraryInfo.create("io.opentelemetry.sdk.metrics", null);
 

diff --git a/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/MeterProviderSharedState.java b/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/MeterProviderSharedState.java
@@ -13,8 +13,9 @@
 @AutoValue
 @Immutable
 abstract class MeterProviderSharedState {
-  static MeterProviderSharedState create(Clock clock, Resource resource) {
-    return new AutoValue_MeterProviderSharedState(clock, resource, new ViewRegistry(), clock.now());
+  static MeterProviderSharedState create(
+      Clock clock, Resource resource, ViewRegistry viewRegistry) {
+    return new AutoValue_MeterProviderSharedState(clock, resource, viewRegistry, clock.now());
   }
 
   abstract Clock getClock();

diff --git a/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/SdkMeterProvider.java b/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/SdkMeterProvider.java
@@ -12,14 +12,11 @@
 import io.opentelemetry.sdk.internal.ComponentRegistry;
 import io.opentelemetry.sdk.metrics.data.MetricData;
 import io.opentelemetry.sdk.metrics.export.MetricProducer;
-import io.opentelemetry.sdk.metrics.view.InstrumentSelector;
-import io.opentelemetry.sdk.metrics.view.View;
 import io.opentelemetry.sdk.resources.Resource;
 import java.util.ArrayList;
 import java.util.Collection;
 import java.util.Collections;
 import java.util.List;
-import java.util.Map;
 import java.util.logging.Logger;
 import javax.annotation.Nullable;
 
@@ -41,11 +38,8 @@ public final class SdkMeterProvider implements MeterProvider, MetricProducer {
   private final ComponentRegistry<SdkMeter> registry;
   private final MeterProviderSharedState sharedState;
 
-  SdkMeterProvider(
-      Clock clock, Resource resource, Map<InstrumentSelector, View> instrumentSelectorViews) {
-    this.sharedState = MeterProviderSharedState.create(clock, resource);
-    instrumentSelectorViews.forEach(
-        (selector, view) -> this.sharedState.getViewRegistry().registerView(selector, view));
+  SdkMeterProvider(Clock clock, Resource resource, ViewRegistry viewRegistry) {
+    this.sharedState = MeterProviderSharedState.create(clock, resource, viewRegistry);
     this.registry =
         new ComponentRegistry<>(
             instrumentationLibraryInfo -> new SdkMeter(sharedState, instrumentationLibraryInfo));
@@ -84,33 +78,4 @@ public Collection<MetricData> collectAllMetrics() {
   public static SdkMeterProviderBuilder builder() {
     return new SdkMeterProviderBuilder();
   }
-
-  /**
-   * Register a view with the given {@link InstrumentSelector}.
-   *
-   * <p>Example on how to register a view:
-   *
-   * <pre>{@code
-   * // get a handle to the MeterSdkProvider
-   * SdkMeterProvider meterProvider = SdkMeterProvider.builder().build();
-   *
-   * // create a selector to select which instruments to customize:
-   * InstrumentSelector instrumentSelector = InstrumentSelector.builder()
-   *   .setInstrumentType(InstrumentType.COUNTER)
-   *   .build();
-   *
-   * // create a specification of how you want the metrics aggregated:
-   * AggregatorFactory aggregatorFactory = AggregatorFactory.minMaxSumCount();
-   *
-   * //register the view with the MeterSdkProvider
-   * meterProvider.registerView(instrumentSelector, View.builder()
-   *   .setAggregatorFactory(aggregatorFactory).build());
-   * }</pre>
-   *
-   * @deprecated Use {@link SdkMeterProviderBuilder#registerView(InstrumentSelector, View)}
-   */
-  @Deprecated
-  public void registerView(InstrumentSelector selector, View view) {
-    sharedState.getViewRegistry().registerView(selector, view);
-  }
 }
diff --git a/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/SdkMeterProviderBuilder.java b/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/SdkMeterProviderBuilder.java
@@ -106,6 +106,9 @@ public SdkMeterProvider buildAndRegisterGlobal() {
    * @see GlobalMeterProvider
    */
   public SdkMeterProvider build() {
-    return new SdkMeterProvider(clock, resource, instrumentSelectorViews);
+    ViewRegistryBuilder viewRegistryBuilder = ViewRegistry.builder();
+    instrumentSelectorViews.forEach(viewRegistryBuilder::addView);
+    ViewRegistry viewRegistry = viewRegistryBuilder.build();
+    return new SdkMeterProvider(clock, resource, viewRegistry);
   }
 }
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
@@ -9,23 +9,19 @@
 import io.opentelemetry.sdk.metrics.common.InstrumentDescriptor;
 import io.opentelemetry.sdk.metrics.common.InstrumentType;
 import io.opentelemetry.sdk.metrics.data.AggregationTemporality;
-import io.opentelemetry.sdk.metrics.view.InstrumentSelector;
 import io.opentelemetry.sdk.metrics.view.View;
 import java.util.EnumMap;
 import java.util.LinkedHashMap;
 import java.util.Map;
-import java.util.concurrent.locks.ReentrantLock;
 import java.util.regex.Pattern;
+import javax.annotation.concurrent.Immutable;
 
 /**
- * Central location for Views to be registered. Registration of a view should eventually be done via
- * the {@link SdkMeterProvider}.
- *
- * <p>This class uses copy-on-write for the registered views to ensure that reading threads get
- * never blocked.
+ * Central location for Views to be registered. Registration of a view is done via the {@link
+ * SdkMeterProviderBuilder}.
  */
+@Immutable
 final class ViewRegistry {
-  private static final LinkedHashMap<Pattern, View> EMPTY_CONFIG = new LinkedHashMap<>();
   static final View CUMULATIVE_SUM =
       View.builder()
           .setAggregatorFactory(AggregatorFactory.sum(AggregationTemporality.CUMULATIVE))
@@ -35,35 +31,18 @@ final class ViewRegistry {
   static final View LAST_VALUE =
       View.builder().setAggregatorFactory(AggregatorFactory.lastValue()).build();
 
-  // The lock is used to ensure only one updated to the configuration happens at any moment.
-  private final ReentrantLock lock = new ReentrantLock();
-  private volatile EnumMap<InstrumentType, LinkedHashMap<Pattern, View>> configuration;
+  private final EnumMap<InstrumentType, LinkedHashMap<Pattern, View>> configuration;
 
-  ViewRegistry() {
+  ViewRegistry(EnumMap<InstrumentType, LinkedHashMap<Pattern, View>> configuration) {
     this.configuration = new EnumMap<>(InstrumentType.class);
-    configuration.put(InstrumentType.COUNTER, EMPTY_CONFIG);
-    configuration.put(InstrumentType.UP_DOWN_COUNTER, EMPTY_CONFIG);
-    configuration.put(InstrumentType.VALUE_RECORDER, EMPTY_CONFIG);
-    configuration.put(InstrumentType.SUM_OBSERVER, EMPTY_CONFIG);
-    configuration.put(InstrumentType.UP_DOWN_SUM_OBSERVER, EMPTY_CONFIG);
-    configuration.put(InstrumentType.VALUE_OBSERVER, EMPTY_CONFIG);
+    // make a copy for safety
+    configuration.forEach(
+        (instrumentType, patternViewLinkedHashMap) ->
+            this.configuration.put(instrumentType, new LinkedHashMap<>(patternViewLinkedHashMap)));
   }
 
-  void registerView(InstrumentSelector selector, View view) {
-    lock.lock();
-    try {
-      EnumMap<InstrumentType, LinkedHashMap<Pattern, View>> newConfiguration =
-          new EnumMap<>(configuration);
-      newConfiguration.put(
-          selector.getInstrumentType(),
-          newLinkedHashMap(
-              selector.getInstrumentNamePattern(),
-              view,
-              newConfiguration.get(selector.getInstrumentType())));
-      configuration = newConfiguration;
-    } finally {
-      lock.unlock();
-    }
+  static ViewRegistryBuilder builder() {
+    return new ViewRegistryBuilder();
   }
 
   View findView(InstrumentDescriptor descriptor) {
@@ -91,12 +70,4 @@ private static View getDefaultSpecification(InstrumentDescriptor descriptor) {
     }
     throw new IllegalArgumentException("Unknown descriptor type: " + descriptor.getType());
   }
-
-  private static LinkedHashMap<Pattern, View> newLinkedHashMap(
-      Pattern pattern, View view, LinkedHashMap<Pattern, View> parentConfiguration) {
-    LinkedHashMap<Pattern, View> result = new LinkedHashMap<>();
-    result.put(pattern, view);
-    result.putAll(parentConfiguration);
-    return result;
-  }
 }
diff --git a/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/ViewRegistryBuilder.java b/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/ViewRegistryBuilder.java
@@ -0,0 +1,46 @@
+/*
+ * Copyright The OpenTelemetry Authors
+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+package io.opentelemetry.sdk.metrics;
+
+import io.opentelemetry.sdk.metrics.common.InstrumentType;
+import io.opentelemetry.sdk.metrics.view.InstrumentSelector;
+import io.opentelemetry.sdk.metrics.view.View;
+import java.util.EnumMap;
+import java.util.LinkedHashMap;
+import java.util.regex.Pattern;
+
+class ViewRegistryBuilder {
+  private final EnumMap<InstrumentType, LinkedHashMap<Pattern, View>> configuration =
+      new EnumMap<>(InstrumentType.class);
+  private static final LinkedHashMap<Pattern, View> EMPTY_CONFIG = new LinkedHashMap<>();
+
+  ViewRegistryBuilder() {
+    for (InstrumentType type : InstrumentType.values()) {
+      configuration.put(type, EMPTY_CONFIG);
+    }
+  }
+
+  ViewRegistry build() {
+    return new ViewRegistry(configuration);
+  }
+
+  ViewRegistryBuilder addView(InstrumentSelector selector, View view) {
+    LinkedHashMap<Pattern, View> parentConfiguration =
+        configuration.get(selector.getInstrumentType());
+    configuration.put(
+        selector.getInstrumentType(),
+        newLinkedHashMap(selector.getInstrumentNamePattern(), view, parentConfiguration));
+    return this;
+  }
+
+  private static LinkedHashMap<Pattern, View> newLinkedHashMap(
+      Pattern pattern, View view, LinkedHashMap<Pattern, View> parentConfiguration) {
+    LinkedHashMap<Pattern, View> result = new LinkedHashMap<>();
+    result.put(pattern, view);
+    result.putAll(parentConfiguration);
+    return result;
+  }
+}
diff --git a/...ics/src/test/java/io/opentelemetry/sdk/metrics/AsynchronousInstrumentAccumulatorTest.java b/...ics/src/test/java/io/opentelemetry/sdk/metrics/AsynchronousInstrumentAccumulatorTest.java
@@ -23,8 +23,7 @@
 
 public class AsynchronousInstrumentAccumulatorTest {
   private final TestClock testClock = TestClock.create();
-  private final MeterProviderSharedState meterProviderSharedState =
-      MeterProviderSharedState.create(testClock, Resource.empty());
+  private MeterProviderSharedState meterProviderSharedState;
   private final MeterSharedState meterSharedState =
       MeterSharedState.create(InstrumentationLibraryInfo.empty());
   private LabelsProcessor spyLabelProcessor;
@@ -33,21 +32,28 @@ public class AsynchronousInstrumentAccumulatorTest {
   void setup() {
     spyLabelProcessor =
         Mockito.spy(
+            // note: can't convert to a lambda here because Mockito gets grumpy
             new LabelsProcessor() {
               @Override
               public Labels onLabelsBound(Context ctx, Labels labels) {
                 return labels.toBuilder().build();
               }
             });
-    meterProviderSharedState
-        .getViewRegistry()
-        .registerView(
-            InstrumentSelector.builder().setInstrumentType(InstrumentType.VALUE_OBSERVER).build(),
-            View.builder()
-                .setAggregatorFactory(AggregatorFactory.lastValue())
-                .setLabelsProcessorFactory(
-                    (resource, instrumentationLibraryInfo, descriptor) -> spyLabelProcessor)
-                .build());
+    ViewRegistry viewRegistry =
+        ViewRegistry.builder()
+            .addView(
+                InstrumentSelector.builder()
+                    .setInstrumentType(InstrumentType.VALUE_OBSERVER)
+                    .build(),
+                View.builder()
+                    .setAggregatorFactory(AggregatorFactory.lastValue())
+                    .setLabelsProcessorFactory(
+                        (resource, instrumentationLibraryInfo, descriptor) -> spyLabelProcessor)
+                    .build())
+            .build();
+
+    meterProviderSharedState =
+        MeterProviderSharedState.create(testClock, Resource.empty(), viewRegistry);
   }
 
   @Test