refactor: Improve tests cases and adhere to style guide

- Move config test to resource files
- Adhere to contribution style guide
- On CEL expression syntax error include return the error message
- Enhance README to clarify the configuration usage

Author: dol

samplers/README.md

@@ -50,6 +50,7 @@ tracer_provider:
         rule_based_routing:
           # Fallback to the always_on sampler if the criteria is not met.
           fallback_sampler:
+            # A fallback must be specified.
             always_on:
           # Only apply to SERVER spans.
           span_kind: SERVER
@@ -77,6 +78,7 @@ Schema for `cel_based` sampler:
 fallback_sampler:
   always_on:
 # List of CEL expressions to evaluate. Expressions are evaluated in order.
+# The first expression that evaluates to true has its action taken, and subsequent expressions are not evaluated.
 expressions:
   # The action to take when the expression evaluates to true. Must be one of: DROP, RECORD_AND_SAMPLE.
   - action: DROP

samplers/src/main/java/io/opentelemetry/contrib/sampler/CelBasedSampler.java

@@ -26,15 +26,14 @@
 import java.util.Map;
 import java.util.logging.Level;
 import java.util.logging.Logger;
-import java.util.regex.Pattern;
 
 /**
  * This sampler accepts a list of {@link CelBasedSamplingExpression}s and tries to match every
  * proposed span against those rules. Every rule describes a span's attribute, a pattern against
  * which to match attribute's value, and a sampler that will make a decision about given span if
  * match was successful.
  *
- * <p>Matching is performed by {@link Pattern}.
+ * <p>Matching is performed by CEL expression evaluation.
  *
  * <p>Provided span kind is checked first and if differs from the one given to {@link
  * #builder(Sampler)}, the default fallback sampler will make a decision.
@@ -48,7 +47,7 @@ public final class CelBasedSampler implements Sampler {
 
   private static final Logger logger = Logger.getLogger(CelBasedSampler.class.getName());
 
-  public static final CelCompiler celCompiler =
+  static final CelCompiler celCompiler =
       CelCompilerFactory.standardCelCompilerBuilder()
           .addVar("name", SimpleType.STRING)
           .addVar("traceId", SimpleType.STRING)
@@ -57,24 +56,35 @@ public final class CelBasedSampler implements Sampler {
           .setResultType(SimpleType.BOOL)
           .build();
 
-  final CelRuntime celRuntime;
-
+  private final CelRuntime celRuntime;
   private final List<CelBasedSamplingExpression> expressions;
   private final Sampler fallback;
 
+  /**
+   * Creates a new CEL-based sampler.
+   *
+   * @param expressions The list of CEL expressions to evaluate
+   * @param fallback The fallback sampler to use when no expressions match
+   */
   public CelBasedSampler(List<CelBasedSamplingExpression> expressions, Sampler fallback) {
     this.expressions = requireNonNull(expressions, "expressions must not be null");
     this.expressions.forEach(
         expr -> {
-          if (!expr.abstractSyntaxTree.isChecked()) {
+          if (!expr.getAbstractSyntaxTree().isChecked()) {
             throw new IllegalArgumentException(
-                "Expression and its AST is not checked: " + expr.expression);
+                "Expression and its AST is not checked: " + expr.getExpression());
           }
         });
-    this.fallback = requireNonNull(fallback);
+    this.fallback = requireNonNull(fallback, "fallback must not be null");
     this.celRuntime = CelRuntimeFactory.standardCelRuntimeBuilder().build();
   }
 
+  /**
+   * Creates a new builder for CEL-based sampler.
+   *
+   * @param fallback The fallback sampler to use when no expressions match
+   * @return A new builder instance
+   */
   public static CelBasedSamplerBuilder builder(Sampler fallback) {
     return new CelBasedSamplerBuilder(
         requireNonNull(fallback, "fallback sampler must not be null"), celCompiler);
@@ -98,34 +108,40 @@ public SamplingResult shouldSample(
 
     for (CelBasedSamplingExpression expression : expressions) {
       try {
-        CelRuntime.Program program = celRuntime.createProgram(expression.abstractSyntaxTree);
+        CelRuntime.Program program = celRuntime.createProgram(expression.getAbstractSyntaxTree());
         Object result = program.eval(evaluationContext);
         // Happy path: Perform sampling based on the boolean result
         if (result instanceof Boolean && ((Boolean) result)) {
-          return expression.delegate.shouldSample(
-              parentContext, traceId, name, spanKind, attributes, parentLinks);
+          return expression
+              .getDelegate()
+              .shouldSample(parentContext, traceId, name, spanKind, attributes, parentLinks);
         }
         // If result is not boolean, treat as false
         logger.log(
             Level.FINE,
-            "Expression '" + expression.expression + "' returned non-boolean result: " + result);
+            "Expression '"
+                + expression.getExpression()
+                + "' returned non-boolean result: "
+                + result);
       } catch (CelEvaluationException e) {
         logger.log(
             Level.FINE,
-            "Expression '" + expression.expression + "' evaluation error: " + e.getMessage());
+            "Expression '" + expression.getExpression() + "' evaluation error: " + e.getMessage());
       }
     }
 
     return fallback.shouldSample(parentContext, traceId, name, spanKind, attributes, parentLinks);
   }
 
-  /** Convert OpenTelemetry Attributes to a Map that CEL can work with */
+  /**
+   * Convert OpenTelemetry Attributes to a Map that CEL can work with.
+   *
+   * @param attributes The OpenTelemetry attributes
+   * @return A map representation of the attributes
+   */
   private static Map<String, Object> convertAttributesToMap(Attributes attributes) {
     Map<String, Object> map = new HashMap<>();
-    attributes.forEach(
-        (key, value) -> {
-          map.put(key.getKey(), value);
-        });
+    attributes.forEach((key, value) -> map.put(key.getKey(), value));
     return map;
   }
 

samplers/src/main/java/io/opentelemetry/contrib/sampler/CelBasedSamplerBuilder.java

@@ -11,24 +11,40 @@
 import dev.cel.common.CelAbstractSyntaxTree;
 import dev.cel.common.CelValidationException;
 import dev.cel.compiler.CelCompiler;
-import io.opentelemetry.api.common.AttributeKey;
 import io.opentelemetry.sdk.trace.samplers.Sampler;
 import java.util.ArrayList;
 import java.util.List;
 
+/**
+ * Builder for {@link CelBasedSampler}.
+ *
+ * <p>This builder allows configuring CEL expressions with their associated sampling actions. Each
+ * expression is evaluated in order, and the first matching expression determines the sampling
+ * decision for a span.
+ */
 public final class CelBasedSamplerBuilder {
   private final CelCompiler celCompiler;
   private final List<CelBasedSamplingExpression> expressions = new ArrayList<>();
   private final Sampler defaultDelegate;
 
+  /**
+   * Creates a new builder with the specified fallback sampler and CEL compiler.
+   *
+   * @param defaultDelegate The fallback sampler to use when no expressions match
+   * @param celCompiler The CEL compiler for compiling expressions
+   */
   CelBasedSamplerBuilder(Sampler defaultDelegate, CelCompiler celCompiler) {
     this.defaultDelegate = defaultDelegate;
     this.celCompiler = celCompiler;
   }
 
   /**
-   * Use the provided sampler when the value of the provided {@link AttributeKey} matches the
-   * provided pattern.
+   * Use the provided sampler when the CEL expression evaluates to true.
+   *
+   * @param expression The CEL expression to evaluate
+   * @param sampler The sampler to use when the expression matches
+   * @return This builder instance for method chaining
+   * @throws CelValidationException if the expression cannot be compiled
    */
   @CanIgnoreReturnValue
   public CelBasedSamplerBuilder customize(String expression, Sampler sampler)
@@ -44,24 +60,34 @@ public CelBasedSamplerBuilder customize(String expression, Sampler sampler)
   }
 
   /**
-   * Drop all spans when the value of the provided {@link AttributeKey} matches the provided
-   * pattern.
+   * Drop all spans when the CEL expression evaluates to true.
+   *
+   * @param expression The CEL expression to evaluate
+   * @return This builder instance for method chaining
+   * @throws CelValidationException if the expression cannot be compiled
    */
   @CanIgnoreReturnValue
   public CelBasedSamplerBuilder drop(String expression) throws CelValidationException {
     return customize(expression, Sampler.alwaysOff());
   }
 
   /**
-   * Record and sample all spans when the value of the provided {@link AttributeKey} matches the
-   * provided pattern.
+   * Record and sample all spans when the CEL expression evaluates to true.
+   *
+   * @param expression The CEL expression to evaluate
+   * @return This builder instance for method chaining
+   * @throws CelValidationException if the expression cannot be compiled
    */
   @CanIgnoreReturnValue
   public CelBasedSamplerBuilder recordAndSample(String expression) throws CelValidationException {
     return customize(expression, Sampler.alwaysOn());
   }
 
-  /** Build the sampler based on the rules provided. */
+  /**
+   * Build the sampler based on the configured expressions.
+   *
+   * @return a new {@link CelBasedSampler} instance
+   */
   public CelBasedSampler build() {
     return new CelBasedSampler(expressions, defaultDelegate);
   }

samplers/src/main/java/io/opentelemetry/contrib/sampler/CelBasedSamplingExpression.java

@@ -12,15 +12,56 @@
 import java.util.Objects;
 import javax.annotation.Nullable;
 
-public class CelBasedSamplingExpression {
-  final CelAbstractSyntaxTree abstractSyntaxTree;
-  final String expression;
-  final Sampler delegate;
+/**
+ * Represents a CEL-based sampling expression that contains a compiled CEL expression and its
+ * associated sampler delegate.
+ *
+ * <p>This class is used internally by {@link CelBasedSampler} to store and evaluate CEL expressions
+ * for sampling decisions.
+ */
+public final class CelBasedSamplingExpression {
+  private final CelAbstractSyntaxTree abstractSyntaxTree;
+  private final String expression;
+  private final Sampler delegate;
 
+  /**
+   * Creates a new CEL-based sampling expression.
+   *
+   * @param abstractSyntaxTree The compiled CEL abstract syntax tree
+   * @param delegate The sampler to use when this expression evaluates to true
+   */
   CelBasedSamplingExpression(CelAbstractSyntaxTree abstractSyntaxTree, Sampler delegate) {
-    this.abstractSyntaxTree = requireNonNull(abstractSyntaxTree);
+    this.abstractSyntaxTree =
+        requireNonNull(abstractSyntaxTree, "abstractSyntaxTree must not be null");
     this.expression = abstractSyntaxTree.getSource().getContent().toString();
-    this.delegate = requireNonNull(delegate);
+    this.delegate = requireNonNull(delegate, "delegate must not be null");
+  }
+
+  /**
+   * Returns the compiled CEL abstract syntax tree.
+   *
+   * @return The abstract syntax tree
+   */
+  CelAbstractSyntaxTree getAbstractSyntaxTree() {
+    return abstractSyntaxTree;
+  }
+
+  /**
+   * Returns the string representation of the CEL expression.
+   *
+   * @return The expression string
+   */
+  String getExpression() {
+    return expression;
+  }
+
+  /**
+   * Returns the sampler delegate.
+   *
+   * @return The sampler delegate
+   */
+  Sampler getDelegate() {
+    return delegate;
   }
 
   @Override

samplers/src/main/java/io/opentelemetry/contrib/sampler/internal/CelBasedSamplerComponentProvider.java

@@ -21,7 +21,7 @@
  * <p>This class is internal and is hence not for public use. Its APIs are unstable and can change
  * at any time.
  */
-public class CelBasedSamplerComponentProvider implements ComponentProvider<Sampler> {
+public final class CelBasedSamplerComponentProvider implements ComponentProvider<Sampler> {
 
   private static final String ACTION_RECORD_AND_SAMPLE = "RECORD_AND_SAMPLE";
   private static final String ACTION_DROP = "DROP";
@@ -38,25 +38,12 @@ public String getName() {
 
   @Override
   public Sampler create(DeclarativeConfigProperties config) {
-    DeclarativeConfigProperties fallbackModel = config.getStructured("fallback_sampler");
-    if (fallbackModel == null) {
-      throw new DeclarativeConfigException(
-          "cel_based sampler .fallback_sampler is required but is null");
-    }
-    Sampler fallbackSampler;
-    try {
-      fallbackSampler = DeclarativeConfiguration.createSampler(fallbackModel);
-    } catch (DeclarativeConfigException e) {
-      throw new DeclarativeConfigException(
-          "cel_based sampler failed to create .fallback_sampler sampler", e);
-    }
-
     List<DeclarativeConfigProperties> expressions = config.getStructuredList("expressions");
     if (expressions == null || expressions.isEmpty()) {
       throw new DeclarativeConfigException("cel_based sampler .expressions is required");
     }
 
-    CelBasedSamplerBuilder builder = CelBasedSampler.builder(fallbackSampler);
+    CelBasedSamplerBuilder builder = CelBasedSampler.builder(getFallbackSampler(config));
 
     for (DeclarativeConfigProperties expressionConfig : expressions) {
       String expression = expressionConfig.getString("expression");
@@ -83,9 +70,27 @@ public Sampler create(DeclarativeConfigProperties config) {
                   + ACTION_DROP);
         }
       } catch (CelValidationException e) {
-        throw new DeclarativeConfigException("Failed to compile CEL expression: " + expression, e);
+        throw new DeclarativeConfigException(
+            "Failed to compile CEL expression: '" + expression + "'. CEL error: " + e.getMessage(),
+            e);
       }
     }
     return builder.build();
   }
+
+  private static Sampler getFallbackSampler(DeclarativeConfigProperties config) {
+    DeclarativeConfigProperties fallbackModel = config.getStructured("fallback_sampler");
+    if (fallbackModel == null) {
+      throw new DeclarativeConfigException(
+          "cel_based sampler .fallback_sampler is required but is null");
+    }
+    Sampler fallbackSampler;
+    try {
+      fallbackSampler = DeclarativeConfiguration.createSampler(fallbackModel);
+    } catch (DeclarativeConfigException e) {
+      throw new DeclarativeConfigException(
+          "cel_based sampler failed to create .fallback_sampler sampler", e);
+    }
+    return fallbackSampler;
+  }
 }