Use CHM for Context keyvalues

This avoids the issues with concurrent reads and writes of the keyvalues on the Context. Reverts the previous change to KeyValues as it is not needed with CHM.
micrometer-metrics · Dec 10, 2024 · a6b2eb1 · a6b2eb1
1 parent f29b5d1
commit a6b2eb1
Show file tree

Hide file tree

Showing 7 changed files with 49 additions and 36 deletions.
diff --git a/...chmarks-core/src/jmh/java/io/micrometer/benchmark/core/ObservationKeyValuesBenchmark.java b/...chmarks-core/src/jmh/java/io/micrometer/benchmark/core/ObservationKeyValuesBenchmark.java
@@ -16,9 +16,9 @@
 
 package io.micrometer.benchmark.core;
 
-import io.micrometer.common.KeyValue;
 import io.micrometer.common.KeyValues;
 import io.micrometer.observation.Observation;
+import io.micrometer.observation.ObservationRegistry;
 import org.openjdk.jmh.annotations.*;
 import org.openjdk.jmh.runner.Runner;
 import org.openjdk.jmh.runner.RunnerException;
@@ -28,37 +28,57 @@
 import java.util.concurrent.TimeUnit;
 
 @State(Scope.Benchmark)
-@OutputTimeUnit(TimeUnit.MICROSECONDS)
+@OutputTimeUnit(TimeUnit.NANOSECONDS)
+@Threads(4)
 public class ObservationKeyValuesBenchmark {
 
     private static final KeyValues KEY_VALUES = KeyValues.of("key1", "value1", "key2", "value2", "key3", "value3",
             "key4", "value4", "key5", "value5");
 
-    private static final KeyValue KEY_VALUE = KeyValue.of("testKey", "testValue");
+    private final ObservationRegistry registry = ObservationRegistry.create();
+
+    private final Observation.Context context = new TestContext();
+
+    private final Observation observation = Observation.createNotStarted("jmh", () -> context, registry);
 
     @Benchmark
-    public void noopBaseline() {
+    @Group("contended")
+    @GroupThreads(1)
+    public Observation contendedWrite() {
+        return write();
     }
 
     @Benchmark
-    public Observation.Context contextBaseline() {
-        return new TestContext().addLowCardinalityKeyValues(KEY_VALUES);
+    @Group("contended")
+    @GroupThreads(1)
+    public KeyValues contendedRead() {
+        return read();
     }
 
     @Benchmark
-    public Observation.Context putKeyValue() {
-        return new TestContext().addLowCardinalityKeyValues(KEY_VALUES).addLowCardinalityKeyValue(KEY_VALUE);
+    @Threads(1)
+    public Observation uncontendedWrite() {
+        return write();
     }
 
     @Benchmark
-    public KeyValues readKeyValues() {
-        return new TestContext().addLowCardinalityKeyValues(KEY_VALUES).getLowCardinalityKeyValues();
+    @Threads(1)
+    public KeyValues uncontendedRead() {
+        return read();
+    }
+
+    private Observation write() {
+        return observation.lowCardinalityKeyValues(KEY_VALUES);
+    }
+
+    private KeyValues read() {
+        return observation.getContext().getLowCardinalityKeyValues();
     }
 
     public static void main(String[] args) throws RunnerException {
         Options options = new OptionsBuilder().include(ObservationKeyValuesBenchmark.class.getSimpleName())
-            .warmupIterations(5)
-            .measurementIterations(10)
+            .warmupIterations(3)
+            .measurementIterations(5)
             .mode(Mode.SampleTime)
             .forks(1)
             .build();

diff --git a/concurrency-tests/build.gradle b/concurrency-tests/build.gradle
@@ -5,7 +5,6 @@ plugins {
 dependencies {
     implementation project(":micrometer-core")
 //    implementation("io.micrometer:micrometer-core:1.12.4")
-    implementation project(":micrometer-test")
     implementation project(":micrometer-registry-prometheus")
     runtimeOnly(libs.logbackLatest)
 }

diff --git a/...-tests/src/jcstress/java/io/micrometer/concurrencytests/MeterRegistryConcurrencyTest.java b/...-tests/src/jcstress/java/io/micrometer/concurrencytests/MeterRegistryConcurrencyTest.java
@@ -15,7 +15,6 @@
  */
 package io.micrometer.concurrencytests;
 
-import io.micrometer.core.Issue;
 import io.micrometer.core.instrument.Counter;
 import io.micrometer.core.instrument.MeterRegistry;
 import io.micrometer.core.instrument.simple.SimpleMeterRegistry;
@@ -154,7 +153,7 @@ public void arbiter(LL_Result r) {
      * iteration could happen at the same time a new meter is being registered, thus added
      * to the preFilterIdToMeterMap, modifying it while iterating over its KeySet.
      */
-    @Issue("gh-5489")
+    // @Issue("gh-5489")
     @JCStressTest
     @Outcome(id = "OK", expect = Expect.ACCEPTABLE, desc = "No exception")
     @Outcome(expect = Expect.FORBIDDEN, desc = "Exception thrown")

diff --git a/...s/src/jcstress/java/io/micrometer/concurrencytests/ObservationContextConcurrencyTest.java b/...s/src/jcstress/java/io/micrometer/concurrencytests/ObservationContextConcurrencyTest.java
@@ -22,7 +22,7 @@
 import org.openjdk.jcstress.annotations.JCStressTest;
 import org.openjdk.jcstress.annotations.Outcome;
 import org.openjdk.jcstress.annotations.State;
-import org.openjdk.jcstress.infra.results.Z_Result;
+import org.openjdk.jcstress.infra.results.LL_Result;
 
 import java.util.UUID;
 
@@ -33,32 +33,33 @@ public class ObservationContextConcurrencyTest {
 
     @JCStressTest
     @State
-    @Outcome(id = "true", expect = ACCEPTABLE)
+    @Outcome(id = "No exception, No exception", expect = ACCEPTABLE)
     @Outcome(expect = FORBIDDEN)
     public static class ConsistentKeyValues {
 
         private final Observation.Context context = new TestContext();
 
+        private final String uuid = UUID.randomUUID().toString();
+
         @Actor
-        public void read(Z_Result r) {
+        public void read(LL_Result r) {
             try {
                 context.getHighCardinalityKeyValues();
-                r.r1 = true;
+                r.r1 = "No exception";
             }
             catch (Exception e) {
-                r.r1 = false;
+                r.r1 = e.getClass();
             }
         }
 
         @Actor
-        public void write(Z_Result r) {
+        public void write(LL_Result r) {
             try {
-                String uuid = UUID.randomUUID().toString();
                 context.addHighCardinalityKeyValue(KeyValue.of(uuid, uuid));
-                r.r1 = true;
+                r.r2 = "No exception";
             }
             catch (Exception e) {
-                r.r1 = false;
+                r.r2 = e.getClass();
             }
         }
 

diff --git a/.../jcstress/java/io/micrometer/concurrencytests/PrometheusMeterRegistryConcurrencyTest.java b/.../jcstress/java/io/micrometer/concurrencytests/PrometheusMeterRegistryConcurrencyTest.java
@@ -15,7 +15,6 @@
  */
 package io.micrometer.concurrencytests;
 
-import io.micrometer.core.Issue;
 import io.micrometer.core.instrument.DistributionSummary;
 import io.micrometer.core.instrument.LongTaskTimer;
 import io.micrometer.core.instrument.LongTaskTimer.Sample;
@@ -38,7 +37,7 @@
  */
 public class PrometheusMeterRegistryConcurrencyTest {
 
-    @Issue("#5193")
+    // @Issue("#5193")
     @JCStressTest
     @State
     @Outcome(id = "true", expect = ACCEPTABLE, desc = "Successful scrape")
@@ -67,7 +66,7 @@ public void scrape(Z_Result r) {
 
     }
 
-    @Issue("#5193")
+    // @Issue("#5193")
     @JCStressTest
     @State
     @Outcome(id = "true", expect = ACCEPTABLE, desc = "Successful scrape")
@@ -96,7 +95,7 @@ public void scrape(Z_Result r) {
 
     }
 
-    @Issue("#5193")
+    // @Issue("#5193")
     @JCStressTest
     @State
     @Outcome(id = "true", expect = ACCEPTABLE, desc = "Successful scrape")

diff --git a/micrometer-commons/src/main/java/io/micrometer/common/KeyValues.java b/micrometer-commons/src/main/java/io/micrometer/common/KeyValues.java
@@ -371,12 +371,7 @@ else if (keyValues instanceof KeyValues) {
         }
         else if (keyValues instanceof Collection) {
             Collection<? extends KeyValue> keyValuesCollection = (Collection<? extends KeyValue>) keyValues;
-            try {
-                return toKeyValues(keyValuesCollection.toArray(EMPTY_KEY_VALUE_ARRAY));
-            }
-            catch (ArrayIndexOutOfBoundsException exception) {
-                return of(keyValues);
-            }
+            return toKeyValues(keyValuesCollection.toArray(EMPTY_KEY_VALUE_ARRAY));
         }
         else {
             return toKeyValues(StreamSupport.stream(keyValues.spliterator(), false).toArray(KeyValue[]::new));

diff --git a/micrometer-observation/src/main/java/io/micrometer/observation/Observation.java b/micrometer-observation/src/main/java/io/micrometer/observation/Observation.java
@@ -925,9 +925,9 @@ class Context implements ContextView {
         @Nullable
         private ObservationView parentObservation;
 
-        private final Map<String, KeyValue> lowCardinalityKeyValues = new LinkedHashMap<>();
+        private final Map<String, KeyValue> lowCardinalityKeyValues = new ConcurrentHashMap<>();
 
-        private final Map<String, KeyValue> highCardinalityKeyValues = new LinkedHashMap<>();
+        private final Map<String, KeyValue> highCardinalityKeyValues = new ConcurrentHashMap<>();
 
         /**
          * The observation name.