From 7e2c34a18f0459e26523dac5c6c05f7411202033 Mon Sep 17 00:00:00 2001 From: John Watson Date: Tue, 20 Apr 2021 10:10:40 -0700 Subject: [PATCH] Make the ViewRegistry immutable and remove the mutating deprecated method. (#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. --- .../io/opentelemetry/sdk/metrics/TestSdk.java | 12 ++- .../sdk/metrics/MeterProviderSharedState.java | 5 +- .../sdk/metrics/SdkMeterProvider.java | 39 +--------- .../sdk/metrics/SdkMeterProviderBuilder.java | 5 +- .../sdk/metrics/ViewRegistry.java | 53 +++---------- .../sdk/metrics/ViewRegistryBuilder.java | 46 +++++++++++ ...AsynchronousInstrumentAccumulatorTest.java | 28 ++++--- .../sdk/metrics/ViewRegistryTest.java | 78 ++++++++++--------- 8 files changed, 138 insertions(+), 128 deletions(-) create mode 100644 sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/ViewRegistryBuilder.java 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 index 972557e6f79..04e36aaa410 100644 --- 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.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 index 14b69818b8c..cf2182840e1 100644 --- 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 index 4e1a68d09f6..a420b865f88 100644 --- 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 registry; private final MeterProviderSharedState sharedState; - SdkMeterProvider( - Clock clock, Resource resource, Map 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 collectAllMetrics() { public static SdkMeterProviderBuilder builder() { return new SdkMeterProviderBuilder(); } - - /** - * Register a view with the given {@link InstrumentSelector}. - * - *

Example on how to register a view: - * - * 

{@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());
-   * }
- * - * @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 index dd7946e2c0d..723bcbc9c7e 100644 --- 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 index 2bd49f1186b..5f778fbd1e3 100644 --- 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}. - * - *

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 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> configuration; + private final EnumMap> configuration; - ViewRegistry() { + ViewRegistry(EnumMap> 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> 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 newLinkedHashMap( - Pattern pattern, View view, LinkedHashMap parentConfiguration) { - LinkedHashMap 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 new file mode 100644 index 00000000000..3131cc08c2a --- /dev/null +++ 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> configuration = + new EnumMap<>(InstrumentType.class); + private static final LinkedHashMap 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 parentConfiguration = + configuration.get(selector.getInstrumentType()); + configuration.put( + selector.getInstrumentType(), + newLinkedHashMap(selector.getInstrumentNamePattern(), view, parentConfiguration)); + return this; + } + + private static LinkedHashMap newLinkedHashMap( + Pattern pattern, View view, LinkedHashMap parentConfiguration) { + LinkedHashMap result = new LinkedHashMap<>(); + result.put(pattern, view); + result.putAll(parentConfiguration); + return result; + } +} diff --git a/sdk/metrics/src/test/java/io/opentelemetry/sdk/metrics/AsynchronousInstrumentAccumulatorTest.java b/sdk/metrics/src/test/java/io/opentelemetry/sdk/metrics/AsynchronousInstrumentAccumulatorTest.java index 18c7f88f369..8bd661782e6 100644 --- a/sdk/metrics/src/test/java/io/opentelemetry/sdk/metrics/AsynchronousInstrumentAccumulatorTest.java +++ b/sdk/metrics/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 diff --git a/sdk/metrics/src/test/java/io/opentelemetry/sdk/metrics/ViewRegistryTest.java b/sdk/metrics/src/test/java/io/opentelemetry/sdk/metrics/ViewRegistryTest.java index 33e4da5b8b9..008aff8821d 100644 --- a/sdk/metrics/src/test/java/io/opentelemetry/sdk/metrics/ViewRegistryTest.java +++ b/sdk/metrics/src/test/java/io/opentelemetry/sdk/metrics/ViewRegistryTest.java @@ -21,13 +21,15 @@ void selection_onType() { AggregatorFactory factory = AggregatorFactory.lastValue(); View view = View.builder().setAggregatorFactory(factory).build(); - ViewRegistry viewRegistry = new ViewRegistry(); - viewRegistry.registerView( - InstrumentSelector.builder() - .setInstrumentType(InstrumentType.COUNTER) - .setInstrumentNameRegex(".*") - .build(), - view); + ViewRegistry viewRegistry = + ViewRegistry.builder() + .addView( + InstrumentSelector.builder() + .setInstrumentType(InstrumentType.COUNTER) + .setInstrumentNameRegex(".*") + .build(), + view) + .build(); assertThat( viewRegistry.findView( InstrumentDescriptor.create( @@ -46,13 +48,15 @@ void selection_onName() { AggregatorFactory factory = AggregatorFactory.lastValue(); View view = View.builder().setAggregatorFactory(factory).build(); - ViewRegistry viewRegistry = new ViewRegistry(); - viewRegistry.registerView( - InstrumentSelector.builder() - .setInstrumentType(InstrumentType.COUNTER) - .setInstrumentNameRegex("overridden") - .build(), - view); + ViewRegistry viewRegistry = + ViewRegistry.builder() + .addView( + InstrumentSelector.builder() + .setInstrumentType(InstrumentType.COUNTER) + .setInstrumentNameRegex("overridden") + .build(), + view) + .build(); assertThat( viewRegistry.findView( InstrumentDescriptor.create( @@ -73,19 +77,21 @@ void selection_LastAddedViewWins() { AggregatorFactory factory2 = AggregatorFactory.minMaxSumCount(); View view2 = View.builder().setAggregatorFactory(factory2).build(); - ViewRegistry viewRegistry = new ViewRegistry(); - viewRegistry.registerView( - InstrumentSelector.builder() - .setInstrumentType(InstrumentType.COUNTER) - .setInstrumentNameRegex(".*") - .build(), - view1); - viewRegistry.registerView( - InstrumentSelector.builder() - .setInstrumentType(InstrumentType.COUNTER) - .setInstrumentNameRegex("overridden") - .build(), - view2); + ViewRegistry viewRegistry = + ViewRegistry.builder() + .addView( + InstrumentSelector.builder() + .setInstrumentType(InstrumentType.COUNTER) + .setInstrumentNameRegex(".*") + .build(), + view1) + .addView( + InstrumentSelector.builder() + .setInstrumentType(InstrumentType.COUNTER) + .setInstrumentNameRegex("overridden") + .build(), + view2) + .build(); assertThat( viewRegistry.findView( @@ -104,13 +110,15 @@ void selection_regex() { AggregatorFactory factory = AggregatorFactory.lastValue(); View view = View.builder().setAggregatorFactory(factory).build(); - ViewRegistry viewRegistry = new ViewRegistry(); - viewRegistry.registerView( - InstrumentSelector.builder() - .setInstrumentNameRegex("overrid(es|den)") - .setInstrumentType(InstrumentType.COUNTER) - .build(), - view); + ViewRegistry viewRegistry = + ViewRegistry.builder() + .addView( + InstrumentSelector.builder() + .setInstrumentNameRegex("overrid(es|den)") + .setInstrumentType(InstrumentType.COUNTER) + .build(), + view) + .build(); assertThat( viewRegistry.findView( @@ -132,7 +140,7 @@ void selection_regex() { @Test void defaults() { - ViewRegistry viewRegistry = new ViewRegistry(); + ViewRegistry viewRegistry = ViewRegistry.builder().build(); assertThat( viewRegistry.findView( InstrumentDescriptor.create(