copilot feedback

Author: jackshirazi

dynamic-control/src/main/java/io/opentelemetry/contrib/dynamic/policy/PolicyStore.java

@@ -7,6 +7,7 @@
 
 import java.util.ArrayList;
 import java.util.Collections;
+import java.util.LinkedHashSet;
 import java.util.List;
 import java.util.Objects;
 
@@ -16,20 +17,27 @@
  */
 public final class PolicyStore {
 
-  private volatile List<TelemetryPolicy> policies = Collections.emptyList();
+  private List<TelemetryPolicy> policies = Collections.emptyList();
 
   /**
-   * Replaces the stored policies when the new list is not equal to the current snapshot.
+   * Replaces the stored policies when the new snapshot is not equal to the current one.
    *
-   * @return {@code true} if the store was updated, {@code false} if the list was equal
+   * <p>Input lists are normalized to a set of distinct policies ({@link TelemetryPolicy#equals
+   * value equality}): duplicates are dropped and only the first occurrence of each policy is
+   * kept (insertion order). Change detection uses set equality, so list order does not matter.
+   * That matches telemetry policy semantics where the effective result does not depend on
+   * processing order (see the telemetry policy OTEP, commutativity / no user-defined ordering
+   * between policies).
+   *
+   * @return {@code true} if the store was updated, {@code false} if the snapshot was unchanged
    */
   public synchronized boolean updatePolicies(List<TelemetryPolicy> newPolicies) {
     Objects.requireNonNull(newPolicies, "newPolicies cannot be null");
-    List<TelemetryPolicy> copy = new ArrayList<>(newPolicies);
-    if (policies.equals(copy)) {
+    List<TelemetryPolicy> normalized = new ArrayList<>(new LinkedHashSet<>(newPolicies));
+    if (new LinkedHashSet<>(policies).equals(new LinkedHashSet<>(normalized))) {
       return false;
     }
-    policies = copy;
+    policies = normalized;
     return true;
   }
 

dynamic-control/src/main/java/io/opentelemetry/contrib/dynamic/policy/TelemetryPolicy.java

@@ -21,6 +21,14 @@
  * <p>Direct instantiation of this base class is intentionally supported for type-only policy
  * signals (for example, to indicate policy removal/reset without policy-specific values).
  *
+ * <p><b>Subclasses:</b> {@link #equals(Object)} on this class returns {@code false} when either
+ * operand is a typed subclass (not {@code TelemetryPolicy} itself), so a type-only instance never
+ * equals a value-carrying subclass with the same {@link #getType() type}. Each concrete subclass
+ * <b>must</b> override both {@code equals} and {@code hashCode} consistently with its fields, and
+ * obey the {@code equals}/{@code hashCode} contract. That is required for consumers such as {@link
+ * io.opentelemetry.contrib.dynamic.policy.PolicyStore} that deduplicate and detect changes using
+ * {@link Object#equals(Object)}.
+ *
  * @see io.opentelemetry.contrib.dynamic.policy
  */
 public class TelemetryPolicy {
@@ -53,7 +61,8 @@ public String getType() {
 
   /**
    * Type-only policies ({@link TelemetryPolicy} instances) do not equal typed subclasses that share
-   * the same {@link #getType() type} string.
+   * the same {@link #getType() type} string. Subclasses must override {@code equals} (and {@code
+   * hashCode}) for value-based equality; see the class Javadoc.
    */
   @Override
   public boolean equals(Object obj) {
@@ -72,6 +81,6 @@ public boolean equals(Object obj) {
 
   @Override
   public int hashCode() {
-    return Objects.hash(type);
+    return type.hashCode();
   }
 }

dynamic-control/src/main/java/io/opentelemetry/contrib/dynamic/policy/tracesampling/TraceSamplingRatePolicy.java

@@ -6,7 +6,6 @@
 package io.opentelemetry.contrib.dynamic.policy.tracesampling;
 
 import io.opentelemetry.contrib.dynamic.policy.TelemetryPolicy;
-import java.util.Objects;
 
 public final class TraceSamplingRatePolicy extends TelemetryPolicy {
   public static final String POLICY_TYPE = "trace-sampling";
@@ -39,6 +38,6 @@ public boolean equals(Object obj) {
 
   @Override
   public int hashCode() {
-    return Objects.hash(probability);
+    return Double.hashCode(probability);
   }
 }

dynamic-control/src/test/java/io/opentelemetry/contrib/dynamic/policy/PolicyStoreTest.java

@@ -41,16 +41,25 @@ void updatePoliciesReturnsTrueWhenProbabilityChanges() {
   }
 
   @Test
-  void updatePoliciesDetectsListOrderChange() {
+  void updatePoliciesReturnsFalseWhenOnlyOrderDiffers() {
     PolicyStore store = new PolicyStore();
     List<TelemetryPolicy> first =
         Arrays.asList(new TraceSamplingRatePolicy(0.1), new TraceSamplingRatePolicy(0.2));
     List<TelemetryPolicy> reordered =
         Arrays.asList(new TraceSamplingRatePolicy(0.2), new TraceSamplingRatePolicy(0.1));
 
     assertThat(store.updatePolicies(first)).isTrue();
-    assertThat(store.updatePolicies(reordered)).isTrue();
-    assertThat(store.getPolicies()).isEqualTo(reordered);
+    assertThat(store.updatePolicies(reordered)).isFalse();
+    assertThat(store.getPolicies()).isEqualTo(first);
+  }
+
+  @Test
+  void updatePoliciesIgnoresDuplicatePoliciesInInput() {
+    PolicyStore store = new PolicyStore();
+    TraceSamplingRatePolicy p = new TraceSamplingRatePolicy(0.5);
+    assertThat(store.updatePolicies(Arrays.asList(p, new TraceSamplingRatePolicy(0.5)))).isTrue();
+    assertThat(store.getPolicies()).containsExactly(p);
+    assertThat(store.updatePolicies(singletonList(new TraceSamplingRatePolicy(0.5)))).isFalse();
   }
 
   @Test