make include/exclude easier to use with empty but not null arguments (#8185)

Co-authored-by: Jack Berg <34418638+jack-berg@users.noreply.github.com>

Author: SylvainJuge

sdk-extensions/incubator/src/main/java/io/opentelemetry/sdk/extension/incubator/fileconfig/ResourceFactory.java

@@ -90,16 +90,7 @@ private static Predicate<String> detectorAttributeFilter(
     if (includedExcludeModel == null) {
       return ResourceFactory::matchAll;
     }
-    List<String> included = includedExcludeModel.getIncluded();
-    List<String> excluded = includedExcludeModel.getExcluded();
-    if (included == null && excluded == null) {
-      return ResourceFactory::matchAll;
-    }
-    // when "included" is omitted in configuration, we get an empty list
-    // in this context it should be interpreted as "include everything"
-    if (included != null && included.isEmpty()) {
-      included = null;
-    }
-    return IncludeExcludePredicate.createPatternMatching(included, excluded);
+    return IncludeExcludePredicate.createPatternMatching(
+        includedExcludeModel.getIncluded(), includedExcludeModel.getExcluded());
   }
 }

sdk-extensions/incubator/src/main/java/io/opentelemetry/sdk/extension/incubator/fileconfig/ViewFactory.java

@@ -35,9 +35,7 @@ public View create(ViewStreamModel model, DeclarativeConfigContext context) {
     if (attributeKeys != null) {
       List<String> included = attributeKeys.getIncluded();
       List<String> excluded = attributeKeys.getExcluded();
-      if (included != null || excluded != null) {
-        builder.setAttributeFilter(IncludeExcludePredicate.createExactMatching(included, excluded));
-      }
+      builder.setAttributeFilter(IncludeExcludePredicate.createExactMatching(included, excluded));
     }
     if (model.getAggregation() != null) {
       builder.setAggregation(

sdk-extensions/incubator/src/test/java/io/opentelemetry/sdk/extension/incubator/fileconfig/ComposableRuleBasedSamplerFactoryTest.java

@@ -162,7 +162,7 @@ private static Stream<Arguments> createTestCases() {
   private static final AttributeKey<String> HTTP_PATH = AttributeKey.stringKey("http.path");
 
   @ParameterizedTest
-  @MethodSource("declarativeCOnfigSamplingPredicateArgs")
+  @MethodSource("declarativeConfigSamplingPredicateArgs")
   void declarativeConfigSamplingPredicate(
       DeclarativeConfigSamplingPredicate predicate,
       Context context,
@@ -174,7 +174,7 @@ void declarativeConfigSamplingPredicate(
   }
 
   @SuppressWarnings("unused")
-  private static Stream<Arguments> declarativeCOnfigSamplingPredicateArgs() {
+  private static Stream<Arguments> declarativeConfigSamplingPredicateArgs() {
     DeclarativeConfigSamplingPredicate matchAll =
         new DeclarativeConfigSamplingPredicate(null, null, null, null);
     DeclarativeConfigSamplingPredicate valuesMatcher =

sdk/common/src/main/java/io/opentelemetry/sdk/common/internal/IncludeExcludePredicate.java

@@ -38,26 +38,28 @@ private IncludeExcludePredicate(
       @Nullable Collection<String> excluded,
       boolean globMatchingEnabled) {
     this.globMatchingEnabled = globMatchingEnabled;
-    this.included = included == null ? null : new LinkedHashSet<>(included);
-    this.excluded = excluded == null ? null : new LinkedHashSet<>(excluded);
+    this.included = copyIfNotEmpty(included);
+    this.excluded = copyIfNotEmpty(excluded);
     if (this.included != null && this.excluded != null) {
       this.predicate =
           includedPredicate(this.included, globMatchingEnabled)
               .and(excludedPredicate(this.excluded, globMatchingEnabled));
-    } else if (this.included == null && this.excluded != null) {
+    } else if (this.excluded != null) {
       this.predicate = excludedPredicate(this.excluded, globMatchingEnabled);
-    } else if (this.excluded == null && this.included != null) {
+    } else if (this.included != null) {
       this.predicate = includedPredicate(this.included, globMatchingEnabled);
     } else {
-      throw new IllegalArgumentException(
-          "At least one of includedPatterns or excludedPatterns must not be null");
+      // include everything by default if both included and excluded are null or empt
+      this.predicate = s -> true;
     }
   }
 
   /**
    * Create a (case-sensitive) exact matching include exclude predicate.
    *
-   * @throws IllegalArgumentException if {@code included} AND {@code excluded} are null.
+   * <p>When {@code included} is empty or {@literal null}, all values are included.
+   *
+   * <p>When {@code excluded} is empty or {@literal null}, no value are excluded.
    */
   public static Predicate<String> createExactMatching(
       @Nullable Collection<String> included, @Nullable Collection<String> excluded) {
@@ -67,9 +69,11 @@ public static Predicate<String> createExactMatching(
   /**
    * Create a pattern matching include exclude predicate.
    *
-   * <p>See {@link GlobUtil} for pattern matching details.
+   * <p>When {@code included} is empty or {@literal null}, all values are included by default.
+   *
+   * <p>When {@code excluded} is empty or {@literal null}, no value is excluded by default.
    *
-   * @throws IllegalArgumentException if {@code included} AND {@code excluded} are null.
+   * <p>See {@link GlobUtil} for pattern matching details.
    */
   public static Predicate<String> createPatternMatching(
       @Nullable Collection<String> included, @Nullable Collection<String> excluded) {
@@ -119,4 +123,9 @@ private static Predicate<String> excludedPredicate(
     }
     return result;
   }
+
+  @Nullable
+  private static Set<String> copyIfNotEmpty(@Nullable Collection<String> collection) {
+    return collection == null || collection.isEmpty() ? null : new LinkedHashSet<>(collection);
+  }
 }

sdk/common/src/test/java/io/opentelemetry/sdk/common/internal/IncludeExcludePredicateTest.java

@@ -9,9 +9,13 @@
 import static java.util.Collections.singletonList;
 import static org.assertj.core.api.Assertions.assertThat;
 
+import java.util.Arrays;
+import java.util.Collection;
 import java.util.Collections;
 import java.util.function.Predicate;
 import java.util.stream.Stream;
+import javax.annotation.Nullable;
+import org.junit.jupiter.api.Test;
 import org.junit.jupiter.params.ParameterizedTest;
 import org.junit.jupiter.params.provider.Arguments;
 import org.junit.jupiter.params.provider.MethodSource;
@@ -26,8 +30,6 @@ class IncludeExcludePredicateTest {
       IncludeExcludePredicate.createExactMatching(singletonList("foo"), singletonList("bar"));
   private static final Predicate<String> EXACT_MULTI =
       IncludeExcludePredicate.createExactMatching(asList("foo", "fooo"), asList("bar", "barr"));
-  public static final Predicate<String> EXACT_INCLUDE_NONE =
-      IncludeExcludePredicate.createExactMatching(Collections.emptyList(), null);
 
   private static final Predicate<String> PATTERN_INCLUDE =
       IncludeExcludePredicate.createPatternMatching(singletonList("f?o"), null);
@@ -37,8 +39,6 @@ class IncludeExcludePredicateTest {
       IncludeExcludePredicate.createPatternMatching(singletonList("f?o"), singletonList("b?r"));
   private static final Predicate<String> PATTERN_MULTI =
       IncludeExcludePredicate.createPatternMatching(asList("f?o", "f?oo"), asList("b?r", "b?rr"));
-  public static final Predicate<String> PATTERN_INCLUDE_NONE =
-      IncludeExcludePredicate.createPatternMatching(Collections.emptyList(), null);
 
   @ParameterizedTest
   @MethodSource("testArgs")
@@ -75,8 +75,6 @@ private static Stream<Arguments> testArgs() {
         Arguments.of(EXACT_MULTI, "bar", false),
         Arguments.of(EXACT_MULTI, "barr", false),
         Arguments.of(EXACT_MULTI, "baz", false),
-        // include none
-        Arguments.of(EXACT_INCLUDE_NONE, "foo", false),
         // pattern matching
         // include only
         Arguments.of(PATTERN_INCLUDE, "foo", true),
@@ -106,9 +104,7 @@ private static Stream<Arguments> testArgs() {
         Arguments.of(PATTERN_MULTI, "bAr", false),
         Arguments.of(PATTERN_MULTI, "barr", false),
         Arguments.of(PATTERN_MULTI, "bArr", false),
-        Arguments.of(PATTERN_MULTI, "baz", false),
-        // include none
-        Arguments.of(PATTERN_INCLUDE_NONE, "foo", false));
+        Arguments.of(PATTERN_MULTI, "baz", false));
   }
 
   @ParameterizedTest
@@ -140,4 +136,30 @@ private static Stream<Arguments> stringRepresentationArgs() {
             PATTERN_MULTI,
             "IncludeExcludePredicate{globMatchingEnabled=true, included=[f?o, f?oo], excluded=[b?r, b?rr]}"));
   }
+
+  @Test
+  void emptyOrNullShouldIncludeAll() {
+    shouldIncludeAll(null, null);
+    shouldIncludeAll(Collections.emptyList(), null);
+    shouldIncludeAll(null, Collections.emptyList());
+    shouldIncludeAll(Collections.emptyList(), Collections.emptyList());
+  }
+
+  private static void shouldIncludeAll(
+      @Nullable Collection<String> included, @Nullable Collection<String> excluded) {
+    Arrays.asList("foo", "fooo", "fOo", "FOO", "bar", "bAr", "barr", "bArr", "baz")
+        .forEach(
+            s -> {
+              Predicate<String> exactMatching =
+                  IncludeExcludePredicate.createExactMatching(included, excluded);
+              assertThat(exactMatching.test(s)).isTrue();
+              assertThat(exactMatching.toString())
+                  .isEqualTo("IncludeExcludePredicate{globMatchingEnabled=false}");
+              Predicate<String> patternMatching =
+                  IncludeExcludePredicate.createPatternMatching(included, excluded);
+              assertThat(patternMatching.test(s)).isTrue();
+              assertThat(patternMatching.toString())
+                  .isEqualTo("IncludeExcludePredicate{globMatchingEnabled=true}");
+            });
+  }
 }

sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/ViewBuilder.java

@@ -11,6 +11,7 @@
 import io.opentelemetry.sdk.metrics.internal.aggregator.AggregatorFactory;
 import io.opentelemetry.sdk.metrics.internal.state.MetricStorage;
 import io.opentelemetry.sdk.metrics.internal.view.AttributesProcessor;
+import java.util.Collections;
 import java.util.Objects;
 import java.util.Set;
 import java.util.function.Predicate;
@@ -74,6 +75,12 @@ public ViewBuilder setAggregation(Aggregation aggregation) {
    */
   public ViewBuilder setAttributeFilter(Set<String> keysToRetain) {
     Objects.requireNonNull(keysToRetain, "keysToRetain");
+    if (keysToRetain.isEmpty()) {
+      // include/exclude predicate requires to include or exclude at least one
+      // thus an empty list of keys is effectively equivalent to ignore all of them
+      return setAttributeFilter(
+          IncludeExcludePredicate.createPatternMatching(null, Collections.singletonList("*")));
+    }
     return setAttributeFilter(IncludeExcludePredicate.createExactMatching(keysToRetain, null));
   }