Add a lock around updating and accessing metrics.

jitsi · Mar 21, 2024 · ab41446 · ab41446
1 parent 350c58d
commit ab41446
Show file tree

Hide file tree

Showing 3 changed files with 39 additions and 6 deletions.
diff --git a/jvb/src/main/kotlin/org/jitsi/videobridge/metrics/Metrics.kt b/jvb/src/main/kotlin/org/jitsi/videobridge/metrics/Metrics.kt
@@ -18,15 +18,30 @@ package org.jitsi.videobridge.metrics
 import org.jitsi.config.JitsiConfig
 import org.jitsi.metaconfig.config
 import org.jitsi.metrics.MetricsUpdater
-import org.jitsi.videobridge.util.TaskPools
+import org.jitsi.utils.concurrent.CustomizableThreadFactory
 import java.time.Duration
+import java.util.concurrent.Executors
 
 object Metrics {
     private val interval: Duration by config {
         "videobridge.stats.interval".from(JitsiConfig.newConfig)
     }
-    val metricsUpdater = MetricsUpdater(TaskPools.SCHEDULED_POOL, interval)
+
+    /** Updating the metrics shouldn't block anywhere, but use a separate executor just in case. */
+    private val executor = Executors.newSingleThreadScheduledExecutor(
+        CustomizableThreadFactory("MetricsUpdater-scheduled", false))
+    val metricsUpdater = MetricsUpdater(executor, interval)
+
+    /**
+     * The lock which is used when metrics are updated or queried. The [MetricsUpdater] internally uses itself as the
+     * lock, so we reuse it here.
+     */
+    val lock: Any
+        get() = metricsUpdater
 
     fun start() = metricsUpdater.addUpdateTask { ThreadsMetric.update() }
-    fun stop() = metricsUpdater.stop()
+    fun stop() {
+        metricsUpdater.stop()
+        executor.shutdown()
+    }
 }
diff --git a/jvb/src/main/kotlin/org/jitsi/videobridge/metrics/VideobridgeMetricsContainer.kt b/jvb/src/main/kotlin/org/jitsi/videobridge/metrics/VideobridgeMetricsContainer.kt
@@ -18,11 +18,23 @@ package org.jitsi.videobridge.metrics
 import org.jitsi.metrics.MetricsContainer
 
 /**
- * `VideobridgeMetricsContainer` gathers and exports metrics
- * from a [Videobridge][org.jitsi.videobridge.Videobridge] instance.
+ * The [MetricsContainer] instance for jitsi-videobridge where all Prometheus metrics are registered.
  */
 class VideobridgeMetricsContainer private constructor() : MetricsContainer(namespace = "jitsi_jvb") {
 
+    /** Acquire Metrics.lock to make sure we don't race with the updater. */
+    override fun getPrometheusMetrics(contentType: String): String {
+        synchronized(Metrics.lock) {
+            return super.getPrometheusMetrics(contentType)
+        }
+    }
+
+    /** Acquire Metrics.lock to make sure we don't race with the updater. */
+    override val jsonString: String
+        get() = synchronized(Metrics.lock) {
+            super.jsonString
+        }
+
     companion object {
         /**
          * The singleton instance of `MetricsContainer`.

diff --git a/jvb/src/main/kotlin/org/jitsi/videobridge/stats/VideobridgeStatisticsShim.kt b/jvb/src/main/kotlin/org/jitsi/videobridge/stats/VideobridgeStatisticsShim.kt
@@ -20,6 +20,7 @@ import org.jitsi.videobridge.EndpointConnectionStatusMonitor
 import org.jitsi.videobridge.VersionConfig
 import org.jitsi.videobridge.health.JvbHealthChecker
 import org.jitsi.videobridge.load_management.JvbLoadManager
+import org.jitsi.videobridge.metrics.Metrics
 import org.jitsi.videobridge.metrics.ThreadsMetric
 import org.jitsi.videobridge.metrics.VideobridgeMetrics
 import org.jitsi.videobridge.metrics.VideobridgePeriodicMetrics
@@ -82,6 +83,11 @@ import java.text.SimpleDateFormat
 import java.util.Date
 import java.util.TimeZone
 
+/**
+ * A shim layer which translates the Prometheus metrics (from [VideobridgeMetricsContainer]) to the legacy
+ * [ColibriStatsExtension] suitable to be added to XMPP presence or converted to JSON for the response to the legacy
+ * /colibri/stats
+ */
 object VideobridgeStatisticsShim {
     fun getStatsJson() = JSONObject().apply {
         getStats().forEach { (k, v) ->
@@ -120,7 +126,7 @@ object VideobridgeStatisticsShim {
         timeZone = TimeZone.getTimeZone("UTC")
     }
 
-    private fun getStats(): Map<String, Any> {
+    private fun getStats(): Map<String, Any> = synchronized(Metrics.lock) {
         return buildMap {
             put("incoming_loss", VideobridgePeriodicMetrics.incomingLoss.get())
             put("outgoing_loss", VideobridgePeriodicMetrics.outgoingLoss.get())