From 2fc6953d606a900b59ec2c9d287fb80142c09ad6 Mon Sep 17 00:00:00 2001
From: Christopher Kolstad <chriswk@getunleash.io>
Date: Tue, 24 Oct 2023 12:04:55 +0200
Subject: [PATCH] fix: Move bucket metrics to ConcurrentHashMaps

Since we're  continuously logging metrics, we could happen to log while
serializing the metrics bucket. Moving to a concurrenthashmap will allow
us to insert into the map while reading from the iterator.

fixes: #59
---
 .../io/getunleash/metrics/MetricsReporter.kt  |  7 +++---
 .../io/getunleash/metrics/MetricsTest.kt      | 23 +++++++++++++------
 2 files changed, 20 insertions(+), 10 deletions(-)

diff --git a/src/main/kotlin/io/getunleash/metrics/MetricsReporter.kt b/src/main/kotlin/io/getunleash/metrics/MetricsReporter.kt
index 2d6e4a7..949b650 100644
--- a/src/main/kotlin/io/getunleash/metrics/MetricsReporter.kt
+++ b/src/main/kotlin/io/getunleash/metrics/MetricsReporter.kt
@@ -17,6 +17,7 @@ import org.slf4j.LoggerFactory
 import java.io.Closeable
 import java.io.IOException
 import java.util.Date
+import java.util.concurrent.ConcurrentHashMap
 import java.util.concurrent.TimeUnit
 
 interface MetricsReporter {
@@ -56,11 +57,11 @@ class NonReporter : MetricsReporter {
     }
 }
 
-data class EvaluationCount(var yes: Int, var no: Int, val variants: MutableMap<String, Int> = mutableMapOf())
+data class EvaluationCount(var yes: Int, var no: Int, val variants: ConcurrentHashMap<String, Int> = ConcurrentHashMap())
 data class Bucket(
     val start: Date,
     var stop: Date? = null,
-    val toggles: MutableMap<String, EvaluationCount> = mutableMapOf()
+    val toggles: ConcurrentHashMap<String, EvaluationCount> = ConcurrentHashMap()
 )
 
 data class Report(val appName: String, val environment: String, val instanceId: String, val bucket: Bucket)
@@ -80,7 +81,7 @@ class HttpMetricsReporter(val config: UnleashConfig, val started: Date = Date())
             appName = config.appName ?: "unknown",
             instanceId = config.instanceId ?: "not-set",
             environment = config.environment ?: "not-set",
-            bucket = bucket.copy(stop = Date())
+            bucket = bucket
         )
         val request = Request.Builder().header("Authorization", config.clientKey).url(metricsUrl).post(
             Parser.jackson.writeValueAsString(report).toRequestBody("application/json".toMediaType())
diff --git a/src/test/kotlin/io/getunleash/metrics/MetricsTest.kt b/src/test/kotlin/io/getunleash/metrics/MetricsTest.kt
index e9329a8..9afef09 100644
--- a/src/test/kotlin/io/getunleash/metrics/MetricsTest.kt
+++ b/src/test/kotlin/io/getunleash/metrics/MetricsTest.kt
@@ -17,7 +17,15 @@ import java.io.File
 import java.time.ZoneOffset
 import java.time.ZonedDateTime
 import java.util.Date
+import java.util.concurrent.ConcurrentHashMap
 
+fun <K, V> concurrentHashMapOf(vararg pairs: Pair<K, V>): ConcurrentHashMap<K, V> {
+    val map: ConcurrentHashMap<K, V> = ConcurrentHashMap()
+    pairs.forEach { (key, value) ->
+        map[key] = value
+    }
+    return map
+}
 class MetricsTest {
 
     lateinit var server: MockWebServer
@@ -118,11 +126,13 @@ class MetricsTest {
         }
         val toggles = reporter.getToggles()
 
-        assertThat(toggles).containsAllEntriesOf(mutableMapOf(
-            "some-non-existing-toggle" to EvaluationCount(0, 100, mutableMapOf("disabled" to 100)),
-            "asdasd" to EvaluationCount(100, 0, mutableMapOf("123" to 100)),
-            "cache.buster" to EvaluationCount(100, 0, mutableMapOf("disabled" to 100)),
-        ))
+        assertThat(toggles).containsAllEntriesOf(
+            concurrentHashMapOf(
+            "some-non-existing-toggle" to EvaluationCount(0, 100, concurrentHashMapOf("disabled" to 100)),
+            "asdasd" to EvaluationCount(100, 0, concurrentHashMapOf("123" to 100)),
+            "cache.buster" to EvaluationCount(100, 0, concurrentHashMapOf("disabled" to 100)),
+        )
+        )
     }
 
     @Test
@@ -169,7 +179,7 @@ class MetricsTest {
             "unleash-android-proxy-sdk" to EvaluationCount(0, 100),
             "non-existing-toggle" to EvaluationCount(0, 100),
             "Test_release" to EvaluationCount(100, 0),
-            "demoApp.step4" to EvaluationCount(100, 0, mutableMapOf("red" to 100))
+            "demoApp.step4" to EvaluationCount(100, 0, concurrentHashMapOf("red" to 100))
         ))
         server.enqueue(MockResponse())
         // No activity since last report, bucket should be empty
@@ -187,5 +197,4 @@ class MetricsTest {
         val output = Parser.jackson.writeValueAsString(Date.from(ZonedDateTime.of(2021, 6, 1, 15, 0, 0, 456000000, ZoneOffset.UTC).toInstant()))
         assertThat(output).isEqualTo("\"2021-06-01T15:00:00.456+00:00\"")
     }
-
 }