Skip to content

Commit 4f534a7

Browse files
shakuzenjonatan-ivanov
shakuzen
and
jonatan-ivanov
authored
Protect against concurrent reads/writes to Context keyvalues (#5739)
Using a ConcurrentHashMap for the Context keyvalues (low and high) avoids the issues with concurrent reads and writes. Related benchmarks and concurrency tests were added as well. See gh-4356 --------- Co-authored-by: Jonatan Ivanov <[email protected]>
1 parent 4a44430 commit 4f534a7

File tree

6 files changed

+171
-10
lines changed

6 files changed

+171
-10
lines changed

‎benchmarks/benchmarks-core/src/jmh/java/io/micrometer/benchmark/core/ObservationKeyValuesBenchmark.java

+93
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,93 @@
1+
/*
2+
* Copyright 2024 VMware, Inc.
3+
*
4+
* Licensed under the Apache License, Version 2.0 (the "License");
5+
* you may not use this file except in compliance with the License.
6+
* You may obtain a copy of the License at
7+
*
8+
* https://www.apache.org/licenses/LICENSE-2.0
9+
*
10+
* Unless required by applicable law or agreed to in writing, software
11+
* distributed under the License is distributed on an "AS IS" BASIS,
12+
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
13+
* See the License for the specific language governing permissions and
14+
* limitations under the License.
15+
*/
16+
17+
package io.micrometer.benchmark.core;
18+
19+
import io.micrometer.common.KeyValues;
20+
import io.micrometer.observation.Observation;
21+
import io.micrometer.observation.ObservationRegistry;
22+
import org.openjdk.jmh.annotations.*;
23+
import org.openjdk.jmh.runner.Runner;
24+
import org.openjdk.jmh.runner.RunnerException;
25+
import org.openjdk.jmh.runner.options.Options;
26+
import org.openjdk.jmh.runner.options.OptionsBuilder;
27+
28+
import java.util.concurrent.TimeUnit;
29+
30+
@State(Scope.Benchmark)
31+
@OutputTimeUnit(TimeUnit.NANOSECONDS)
32+
@Threads(4)
33+
public class ObservationKeyValuesBenchmark {
34+
35+
private static final KeyValues KEY_VALUES = KeyValues.of("key1", "value1", "key2", "value2", "key3", "value3",
36+
"key4", "value4", "key5", "value5");
37+
38+
private final ObservationRegistry registry = ObservationRegistry.create();
39+
40+
private final Observation.Context context = new TestContext();
41+
42+
private final Observation observation = Observation.createNotStarted("jmh", () -> context, registry);
43+
44+
@Benchmark
45+
@Group("contended")
46+
@GroupThreads(1)
47+
public Observation contendedWrite() {
48+
return write();
49+
}
50+
51+
@Benchmark
52+
@Group("contended")
53+
@GroupThreads(1)
54+
public KeyValues contendedRead() {
55+
return read();
56+
}
57+
58+
@Benchmark
59+
@Threads(1)
60+
public Observation uncontendedWrite() {
61+
return write();
62+
}
63+
64+
@Benchmark
65+
@Threads(1)
66+
public KeyValues uncontendedRead() {
67+
return read();
68+
}
69+
70+
private Observation write() {
71+
return observation.lowCardinalityKeyValues(KEY_VALUES);
72+
}
73+
74+
private KeyValues read() {
75+
return observation.getContext().getLowCardinalityKeyValues();
76+
}
77+
78+
public static void main(String[] args) throws RunnerException {
79+
Options options = new OptionsBuilder().include(ObservationKeyValuesBenchmark.class.getSimpleName())
80+
.warmupIterations(3)
81+
.measurementIterations(5)
82+
.mode(Mode.SampleTime)
83+
.forks(1)
84+
.build();
85+
86+
new Runner(options).run();
87+
}
88+
89+
static class TestContext extends Observation.Context {
90+
91+
}
92+
93+
}

‎concurrency-tests/build.gradle

-1
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,6 @@ plugins {
55
dependencies {
66
implementation project(":micrometer-core")
77
// implementation("io.micrometer:micrometer-core:1.12.4")
8-
implementation project(":micrometer-test")
98
implementation project(":micrometer-registry-prometheus")
109
runtimeOnly(libs.logbackLatest)
1110
}

‎concurrency-tests/src/jcstress/java/io/micrometer/concurrencytests/MeterRegistryConcurrencyTest.java

+1-2
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,6 @@
1515
*/
1616
package io.micrometer.concurrencytests;
1717

18-
import io.micrometer.core.Issue;
1918
import io.micrometer.core.instrument.Counter;
2019
import io.micrometer.core.instrument.MeterRegistry;
2120
import io.micrometer.core.instrument.simple.SimpleMeterRegistry;
@@ -154,7 +153,7 @@ public void arbiter(LL_Result r) {
154153
* iteration could happen at the same time a new meter is being registered, thus added
155154
* to the preFilterIdToMeterMap, modifying it while iterating over its KeySet.
156155
*/
157-
@Issue("gh-5489")
156+
// @Issue("gh-5489")
158157
@JCStressTest
159158
@Outcome(id = "OK", expect = Expect.ACCEPTABLE, desc = "No exception")
160159
@Outcome(expect = Expect.FORBIDDEN, desc = "Exception thrown")

‎concurrency-tests/src/jcstress/java/io/micrometer/concurrencytests/ObservationContextConcurrencyTest.java

+72
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,72 @@
1+
/*
2+
* Copyright 2024 VMware, Inc.
3+
*
4+
* Licensed under the Apache License, Version 2.0 (the "License");
5+
* you may not use this file except in compliance with the License.
6+
* You may obtain a copy of the License at
7+
*
8+
* https://www.apache.org/licenses/LICENSE-2.0
9+
*
10+
* Unless required by applicable law or agreed to in writing, software
11+
* distributed under the License is distributed on an "AS IS" BASIS,
12+
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
13+
* See the License for the specific language governing permissions and
14+
* limitations under the License.
15+
*/
16+
17+
package io.micrometer.concurrencytests;
18+
19+
import io.micrometer.common.KeyValue;
20+
import io.micrometer.observation.Observation;
21+
import org.openjdk.jcstress.annotations.Actor;
22+
import org.openjdk.jcstress.annotations.JCStressTest;
23+
import org.openjdk.jcstress.annotations.Outcome;
24+
import org.openjdk.jcstress.annotations.State;
25+
import org.openjdk.jcstress.infra.results.LL_Result;
26+
27+
import java.util.UUID;
28+
29+
import static org.openjdk.jcstress.annotations.Expect.ACCEPTABLE;
30+
import static org.openjdk.jcstress.annotations.Expect.FORBIDDEN;
31+
32+
public class ObservationContextConcurrencyTest {
33+
34+
@JCStressTest
35+
@State
36+
@Outcome(id = "No exception, No exception", expect = ACCEPTABLE)
37+
@Outcome(expect = FORBIDDEN)
38+
public static class ConsistentKeyValues {
39+
40+
private final Observation.Context context = new TestContext();
41+
42+
private final String uuid = UUID.randomUUID().toString();
43+
44+
@Actor
45+
public void read(LL_Result r) {
46+
try {
47+
context.getHighCardinalityKeyValues();
48+
r.r1 = "No exception";
49+
}
50+
catch (Exception e) {
51+
r.r1 = e.getClass();
52+
}
53+
}
54+
55+
@Actor
56+
public void write(LL_Result r) {
57+
try {
58+
context.addHighCardinalityKeyValue(KeyValue.of(uuid, uuid));
59+
r.r2 = "No exception";
60+
}
61+
catch (Exception e) {
62+
r.r2 = e.getClass();
63+
}
64+
}
65+
66+
}
67+
68+
static class TestContext extends Observation.Context {
69+
70+
}
71+
72+
}

‎concurrency-tests/src/jcstress/java/io/micrometer/concurrencytests/PrometheusMeterRegistryConcurrencyTest.java

+3-4
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,6 @@
1515
*/
1616
package io.micrometer.concurrencytests;
1717

18-
import io.micrometer.core.Issue;
1918
import io.micrometer.core.instrument.DistributionSummary;
2019
import io.micrometer.core.instrument.LongTaskTimer;
2120
import io.micrometer.core.instrument.LongTaskTimer.Sample;
@@ -38,7 +37,7 @@
3837
*/
3938
public class PrometheusMeterRegistryConcurrencyTest {
4039

41-
@Issue("#5193")
40+
// @Issue("#5193")
4241
@JCStressTest
4342
@State
4443
@Outcome(id = "true", expect = ACCEPTABLE, desc = "Successful scrape")
@@ -67,7 +66,7 @@ public void scrape(Z_Result r) {
6766

6867
}
6968

70-
@Issue("#5193")
69+
// @Issue("#5193")
7170
@JCStressTest
7271
@State
7372
@Outcome(id = "true", expect = ACCEPTABLE, desc = "Successful scrape")
@@ -96,7 +95,7 @@ public void scrape(Z_Result r) {
9695

9796
}
9897

99-
@Issue("#5193")
98+
// @Issue("#5193")
10099
@JCStressTest
101100
@State
102101
@Outcome(id = "true", expect = ACCEPTABLE, desc = "Successful scrape")

‎micrometer-observation/src/main/java/io/micrometer/observation/Observation.java

+2-3
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,6 @@
2121
import io.micrometer.common.lang.Nullable;
2222
import io.micrometer.common.util.internal.logging.InternalLoggerFactory;
2323

24-
import java.util.LinkedHashMap;
2524
import java.util.Map;
2625
import java.util.concurrent.Callable;
2726
import java.util.concurrent.ConcurrentHashMap;
@@ -925,9 +924,9 @@ class Context implements ContextView {
925924
@Nullable
926925
private ObservationView parentObservation;
927926

928-
private final Map<String, KeyValue> lowCardinalityKeyValues = new LinkedHashMap<>();
927+
private final Map<String, KeyValue> lowCardinalityKeyValues = new ConcurrentHashMap<>();
929928

930-
private final Map<String, KeyValue> highCardinalityKeyValues = new LinkedHashMap<>();
929+
private final Map<String, KeyValue> highCardinalityKeyValues = new ConcurrentHashMap<>();
931930

932931
/**
933932
* The observation name.

0 commit comments

Comments
 (0)