Address review comment: check instrument compatibility before applying View enabled=false

Author: dashpole

sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/internal/view/ViewRegistry.java

@@ -111,14 +111,11 @@ public List<RegisteredView> findViews(
     // Find matching views for the instrument
     for (RegisteredView entry : registeredViews) {
       if (matchesSelector(entry.getInstrumentSelector(), descriptor, meterScope)) {
-        if (Boolean.FALSE.equals(entry.getEnabled())) {
-          result.add(applyDropAggregation(entry));
-          continue;
-        }
         AggregatorFactory viewAggregatorFactory =
             (AggregatorFactory) entry.getView().getAggregation();
         if (viewAggregatorFactory.isCompatibleWithInstrument(descriptor)) {
-          if (descriptor.getAdvice().isOptIn() && !Boolean.TRUE.equals(entry.getEnabled())) {
+          if (Boolean.FALSE.equals(entry.getEnabled())
+              || (descriptor.getAdvice().isOptIn() && !Boolean.TRUE.equals(entry.getEnabled()))) {
             result.add(applyDropAggregation(entry));
           } else if (Boolean.TRUE.equals(entry.getEnabled())
               && descriptor.getAdvice().hasAttributes()

sdk/metrics/src/test/java/io/opentelemetry/sdk/metrics/internal/view/ViewRegistryTest.java

@@ -530,6 +530,7 @@ void findViews_ApplyAdvice() {
   }
 
   @Test
+  @SuppressLogger(ViewRegistry.class)
   void findViews_OptInAndViewEnabled() {
     ViewBuilder enabledViewBuilder = View.builder().setDescription("enabled view");
     SdkMeterProviderUtil.setEnabled(enabledViewBuilder, /* enabled= */ true);
@@ -547,6 +548,15 @@ void findViews_OptInAndViewEnabled() {
             disabledViewBuilder.build(),
             /* enabled= */ false);
 
+    ViewBuilder incompatibleDisabledViewBuilder =
+        View.builder().setAggregation(Aggregation.explicitBucketHistogram());
+    SdkMeterProviderUtil.setEnabled(incompatibleDisabledViewBuilder, /* enabled= */ false);
+    RegisteredView incompatibleDisabledView =
+        registeredView(
+            InstrumentSelector.builder().setName("incompatible_disabled").build(),
+            incompatibleDisabledViewBuilder.build(),
+            /* enabled= */ false);
+
     RegisteredView unsetEnabledView =
         registeredView(
             InstrumentSelector.builder().setName("opt_in_unset_view").build(),
@@ -557,7 +567,7 @@ void findViews_OptInAndViewEnabled() {
         ViewRegistry.create(
             DefaultAggregationSelector.getDefault(),
             CardinalityLimitSelector.defaultCardinalityLimitSelector(),
-            Arrays.asList(enabledView, disabledView, unsetEnabledView));
+            Arrays.asList(enabledView, disabledView, incompatibleDisabledView, unsetEnabledView));
 
     // OptIn instrument with no matching view gets DROP_REGISTERED_VIEW
     assertThat(
@@ -615,6 +625,21 @@ void findViews_OptInAndViewEnabled() {
         .hasSize(1)
         .element(0)
         .satisfies(rv -> assertThat(rv.getView().getAggregation()).isEqualTo(Aggregation.drop()));
+
+    // Incompatible view with enabled=false still logs a warning and is ignored
+    assertThat(
+            viewRegistry.findViews(
+                InstrumentDescriptor.create(
+                    "incompatible_disabled",
+                    "",
+                    "",
+                    InstrumentType.OBSERVABLE_COUNTER,
+                    InstrumentValueType.LONG,
+                    Advice.empty()),
+                INSTRUMENTATION_SCOPE_INFO))
+        .isEqualTo(Collections.singletonList(DEFAULT_REGISTERED_VIEW));
+    logs.assertContains(
+        "View aggregation explicit_bucket_histogram is incompatible with instrument incompatible_disabled of type OBSERVABLE_COUNTER");
   }
 
   private static void assertAdviceFilter(RegisteredView view, AttributeKey<?>... expectedKeys) {

sdk/metrics/src/testIncubating/java/io/opentelemetry/sdk/metrics/OptInAdviceTest.java

@@ -170,6 +170,24 @@ void optInCounterNotEnabledByViewWithAggregationOnly() {
   void nonOptInInstrumentDisabledByViewEnabledFalse(
       InstrumentFactory instrumentFactory, PointsAssert<AbstractPointAssert<?, ?>> unused) {
     InMemoryMetricReader reader = InMemoryMetricReader.create();
+    ViewBuilder viewBuilder = View.builder();
+    SdkMeterProviderUtil.setEnabled(viewBuilder, /* enabled= */ false);
+    meterProvider =
+        SdkMeterProvider.builder()
+            .registerMetricReader(reader)
+            .registerView(InstrumentSelector.builder().setName("test").build(), viewBuilder.build())
+            .build();
+
+    TestInstrument instrument = instrumentFactory.create(meterProvider, "test", /* optIn= */ false);
+    assertThat(instrument.isEnabled()).isFalse();
+    instrument.record(1, ATTRIBUTES);
+
+    assertThat(reader.collectAllMetrics()).isEmpty();
+  }
+
+  @Test
+  void nonOptInCounterDisabledByViewEnabledFalseOverridesAggregation() {
+    InMemoryMetricReader reader = InMemoryMetricReader.create();
     ViewBuilder viewBuilder =
         View.builder()
             .setAggregation(
@@ -179,12 +197,17 @@ void nonOptInInstrumentDisabledByViewEnabledFalse(
     meterProvider =
         SdkMeterProvider.builder()
             .registerMetricReader(reader)
-            .registerView(InstrumentSelector.builder().setName("test").build(), viewBuilder.build())
+            .registerView(
+                InstrumentSelector.builder().setName("test_counter").build(), viewBuilder.build())
             .build();
 
-    TestInstrument instrument = instrumentFactory.create(meterProvider, "test", /* optIn= */ false);
-    assertThat(instrument.isEnabled()).isFalse();
-    instrument.record(1, ATTRIBUTES);
+    ExtendedLongCounter counter =
+        (ExtendedLongCounter)
+            ((ExtendedLongCounterBuilder) meterProvider.get("meter").counterBuilder("test_counter"))
+                .setOptInAdvice(false)
+                .build();
+    assertThat(counter.isEnabled()).isFalse();
+    counter.add(1, ATTRIBUTES);
 
     assertThat(reader.collectAllMetrics()).isEmpty();
   }