Make SimpleMetrics thread-safe #56

Activity

1. sspaink

   added this to the Metrics milestone on Jun 22, 2026
2. sspaink

   added the

   Bug

   issue type on Jun 22, 2026
3. github-project-automation

   added this to Java SDK Roadmap 2026on Jun 22, 2026
4. github-project-automation

   moved this to Todo in Java SDK Roadmap 2026on Jun 22, 2026
5. sspaink

   added

   evaluator

   metricsMetrics API and instrumentationMetrics API and instrumentation

   on Jun 22, 2026
6. c879873067877881111 commented on Sep 3, 2026

   

   c879873067877881111

   on Sep 3, 2026

   Contributor

   More actions

   Measured on macOS 14.8.4, Apple Silicon, 8 cores, OpenJDK 17.0.19.

   I have this working locally, but I ran into something the second bullet doesn't cover and I'd rather agree on the semantics before opening a PR.

   Adding synchronized is not sufficient — value() can still return a negative Duration.

   SimpleTimer reports end - start, and with a shared timer those two fields can come from different threads. Thread A runs start() then stop(); thread B runs start() before A reaches value(). Now end is A's stop timestamp and start is B's later start timestamp, so the subtraction goes negative. Locking makes the reads coherent, it does not make them come from the same interval.

   I measured it. I applied exactly what the bullets ask for — timer map to ConcurrentHashMap with computeIfAbsent, and start/stop/value all synchronized, keeping end - start — then ran 8 threads × 500 iterations of start(); stop(); value(); against one shared timer, ten times:

   RUN  1: FAIL  timer reported a negative duration: PT-0.000000209S
   RUN  2: FAIL  timer reported a negative duration: PT-0.000001084S
   RUN  3: FAIL  timer reported a negative duration: PT-0.000000208S
   RUN  4: FAIL  timer reported a negative duration: PT-0.000000333S
   RUN  5: FAIL  timer reported a negative duration: PT-0.000000834S
   RUN  6: FAIL  timer reported a negative duration: PT-0.000000750S
   RUN  7: FAIL  timer reported a negative duration: PT-0.000000625S
   RUN  8: FAIL  timer reported a negative duration: PT-0.000000667S
   RUN  9: FAIL  timer reported a negative duration: PT-0.000000583S
   RUN 10: FAIL  timer reported a negative duration: PT-0.000000375S
   

   between -208ns and -1084ns.

   Swapping SimpleTimer to the accumulate semantics v1/metrics uses in Go — Stop() adds time.Since(start) to a running total and resets start, Value() returns that total — the same test passes ten out of ten, because the total only ever grows by a non-negative delta.

   The question: accumulating is a behaviour change beyond "make it thread-safe". Single-threaded start(); stop(); start(); stop(); value() goes from "the last interval" to "the sum of both". Nothing in the suite pins the current reading — MetricsPrinterTest only covers never-started timers, and MetricsIntegrationTest asserts the duration is non-negative — so both variants are green apart from the new test. (preparedQuery_repeatedEvaluations_accumulateTimerCallsButRetainNamesOnce reads like accumulation was the original intent, though it only asserts the key set.)

   Which would you prefer?

   1. Fix Make SimpleMetrics thread-safe #56 with the accumulate semantics, matching Go, with the behaviour change called out in the changelog.
   2. Keep Make SimpleMetrics thread-safe #56 to locking plus ConcurrentHashMap, and track the negative duration separately — accepting that this ships a SimpleTimer which is race-free but can still report a negative duration.

   I have (1) written and tested and can open the PR for either. Happy to take (2) if you'd rather keep the scope tight.