code review comments

open-telemetry · Aug 10, 2023 · e333bea · e333bea
1 parent ee699ec
commit e333bea
Show file tree

Hide file tree

Showing 3 changed files with 56 additions and 27 deletions.
diff --git a/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/internal/descriptor/Advice.java b/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/internal/descriptor/Advice.java
@@ -35,6 +35,10 @@ public static AdviceBuilder builder() {
   @Nullable
   public abstract List<AttributeKey<?>> getAttributes();
 
+  public boolean hasAttributes() {
+    return getAttributes() != null;
+  }
+
   @AutoValue.Builder
   public abstract static class AdviceBuilder {
 

diff --git a/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/internal/view/ViewRegistry.java b/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/internal/view/ViewRegistry.java
@@ -130,32 +130,30 @@ public List<RegisteredView> findViews(
     RegisteredView instrumentDefaultView =
         requireNonNull(instrumentDefaultRegisteredView.get(descriptor.getType()));
 
-    // if the user defined an advice, use it
-    if (shouldApplyAdvice(descriptor.getAdvice())) {
-      instrumentDefaultView =
-          applyAdviceToDefaultView(instrumentDefaultView, descriptor.getAdvice());
-    }
-
     AggregatorFactory viewAggregatorFactory =
         (AggregatorFactory) instrumentDefaultView.getView().getAggregation();
 
-    // If the aggregation from default aggregation selector is compatible with the instrument, use
-    // it
-    if (viewAggregatorFactory.isCompatibleWithInstrument(descriptor)) {
-      return Collections.singletonList(instrumentDefaultView);
+    if (!viewAggregatorFactory.isCompatibleWithInstrument(descriptor)) {
+      // The aggregation from default aggregation selector was incompatible with instrument, use
+      // default aggregation instead
+      logger.log(
+          Level.WARNING,
+          "Instrument default aggregation "
+              + AggregationUtil.aggregationName(instrumentDefaultView.getView().getAggregation())
+              + " is incompatible with instrument "
+              + descriptor.getName()
+              + " of type "
+              + descriptor.getType());
+      instrumentDefaultView = DEFAULT_REGISTERED_VIEW;
+    }
+
+    // if the user defined an attributes advice, use it
+    if (descriptor.getAdvice().hasAttributes()) {
+      instrumentDefaultView =
+          applyAdviceToDefaultView(instrumentDefaultView, descriptor.getAdvice());
     }
 
-    // The aggregation from default aggregation selector was incompatible with instrument, use
-    // default aggregation instead
-    logger.log(
-        Level.WARNING,
-        "Instrument default aggregation "
-            + AggregationUtil.aggregationName(instrumentDefaultView.getView().getAggregation())
-            + " is incompatible with instrument "
-            + descriptor.getName()
-            + " of type "
-            + descriptor.getType());
-    return Collections.singletonList(DEFAULT_REGISTERED_VIEW);
+    return Collections.singletonList(instrumentDefaultView);
   }
 
   // Matches an instrument selector against an instrument + meter.
@@ -255,10 +253,6 @@ private static Pattern toRegexPattern(String globPattern) {
     return Pattern.compile(patternBuilder.toString());
   }
 
-  private static boolean shouldApplyAdvice(Advice descriptor) {
-    return descriptor.getAttributes() != null;
-  }
-
   private static RegisteredView applyAdviceToDefaultView(
       RegisteredView instrumentDefaultView, Advice advice) {
     return RegisteredView.create(

diff --git a/sdk/metrics/src/test/java/io/opentelemetry/sdk/metrics/internal/view/ViewRegistryTest.java b/sdk/metrics/src/test/java/io/opentelemetry/sdk/metrics/internal/view/ViewRegistryTest.java
@@ -434,14 +434,21 @@ void defaults() {
   }
 
   @Test
-  void adviceAttributesProcessor() {
+  void findViews_ApplyAdvice() {
+    // use incompatible aggregation for histogram
+    DefaultAggregationSelector aggregationSelector =
+        instrumentType ->
+            instrumentType == InstrumentType.HISTOGRAM
+                ? Aggregation.lastValue()
+                : Aggregation.defaultAggregation();
+
     RegisteredView registeredView =
         registeredView(
             InstrumentSelector.builder().setName("test").build(),
             View.builder().setDescription("view applied").build());
     ViewRegistry viewRegistry =
         ViewRegistry.create(
-            DefaultAggregationSelector.getDefault(),
+            aggregationSelector,
             CardinalityLimitSelector.defaultCardinalityLimitSelector(),
             Collections.singletonList(registeredView));
 
@@ -483,6 +490,30 @@ void adviceAttributesProcessor() {
                     MetricStorage.DEFAULT_MAX_CARDINALITY,
                     SourceInfo.noSourceInfo())));
 
+    // If there is no matching view and attributes advice was defined, use it - incompatible
+    // aggregation case
+    assertThat(
+            viewRegistry.findViews(
+                InstrumentDescriptor.create(
+                    "histogram_advice",
+                    "",
+                    "",
+                    InstrumentType.HISTOGRAM,
+                    InstrumentValueType.DOUBLE,
+                    Advice.builder()
+                        .setAttributes(Arrays.asList(stringKey("key1"), stringKey("key2")))
+                        .build()),
+                INSTRUMENTATION_SCOPE_INFO))
+        .isEqualTo(
+            Collections.singletonList(
+                RegisteredView.create(
+                    DEFAULT_REGISTERED_VIEW.getInstrumentSelector(),
+                    DEFAULT_REGISTERED_VIEW.getView(),
+                    new AdviceAttributesProcessor(
+                        Arrays.asList(stringKey("key1"), stringKey("key2"))),
+                    DEFAULT_REGISTERED_VIEW.getCardinalityLimit(),
+                    DEFAULT_REGISTERED_VIEW.getViewSourceInfo())));
+
     // if advice is not defined, use the default view
     assertThat(
             viewRegistry.findViews(