Metrics clean (#3403)

* Remove `timestampMs` from `LocalMetricsAggregator` * put `toStatsdType` in enum directly * beforeMetric wrapped in try catch * flush check if there are metrics before scheduling again
getsentry · May 3, 2024 · 8d236f4 · 8d236f4
1 parent 8045023
commit 8d236f4
Show file tree

Hide file tree

Showing 10 changed files with 52 additions and 52 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -9,6 +9,7 @@
 
 ### Fixes
 
+- (Internal) Metrics code cleanup ([#3403](https://github.com/getsentry/sentry-java/pull/3403))
 - Fix Frame measurements in app start transactions ([#3382](https://github.com/getsentry/sentry-java/pull/3382))
 - Fix timing metric value different from span duration ([#3368](https://github.com/getsentry/sentry-java/pull/3368))
 

diff --git a/sentry/api/sentry.api b/sentry/api/sentry.api
@@ -3469,7 +3469,7 @@ public abstract interface class io/sentry/metrics/IMetricsClient {
 
 public final class io/sentry/metrics/LocalMetricsAggregator {
 	public fun <init> ()V
-	public fun add (Ljava/lang/String;Lio/sentry/metrics/MetricType;Ljava/lang/String;DLio/sentry/MeasurementUnit;Ljava/util/Map;J)V
+	public fun add (Ljava/lang/String;Lio/sentry/metrics/MetricType;Ljava/lang/String;DLio/sentry/MeasurementUnit;Ljava/util/Map;)V
 	public fun getSummaries ()Ljava/util/Map;
 }
 
@@ -3544,7 +3544,6 @@ public final class io/sentry/metrics/MetricsHelper {
 	public static fun sanitizeTagValue (Ljava/lang/String;)Ljava/lang/String;
 	public static fun sanitizeUnit (Ljava/lang/String;)Ljava/lang/String;
 	public static fun setFlushShiftMs (J)V
-	public static fun toStatsdType (Lio/sentry/metrics/MetricType;)Ljava/lang/String;
 }
 
 public final class io/sentry/metrics/NoopMetricsAggregator : io/sentry/IMetricsAggregator, io/sentry/metrics/MetricsApi$IMetricsInterface {

diff --git a/sentry/src/main/java/io/sentry/MetricsAggregator.java b/sentry/src/main/java/io/sentry/MetricsAggregator.java
@@ -156,8 +156,12 @@ private void add(
     }
 
     if (beforeEmitCallback != null) {
-      if (!beforeEmitCallback.execute(key, tags)) {
-        return;
+      try {
+        if (!beforeEmitCallback.execute(key, tags)) {
+          return;
+        }
+      } catch (Throwable e) {
+        logger.log(SentryLevel.ERROR, "The beforeEmit callback threw an exception.", e);
       }
     }
 
@@ -206,7 +210,7 @@ private void add(
     // See develop docs: https://develop.sentry.dev/sdk/metrics/#sets
     if (localMetricsAggregator != null) {
       final double localValue = type == MetricType.Set ? addedWeight : value;
-      localMetricsAggregator.add(metricKey, type, key, localValue, unit, tags, timestampMs);
+      localMetricsAggregator.add(metricKey, type, key, localValue, unit, tags);
     }
 
     final boolean isOverWeight = isOverWeight();
@@ -324,7 +328,7 @@ public void run() {
     flush(false);
 
     synchronized (this) {
-      if (!isClosed) {
+      if (!isClosed && !buckets.isEmpty()) {
         executorService.schedule(this, MetricsHelper.FLUSHER_SLEEP_TIME_MS);
       }
     }

diff --git a/sentry/src/main/java/io/sentry/metrics/LocalMetricsAggregator.java b/sentry/src/main/java/io/sentry/metrics/LocalMetricsAggregator.java
@@ -28,8 +28,7 @@ public void add(
       final @NotNull String key,
       final double value,
       final @Nullable MeasurementUnit unit,
-      final @Nullable Map<String, String> tags,
-      final long timestampMs) {
+      final @Nullable Map<String, String> tags) {
 
     final @NotNull String exportKey = MetricsHelper.getExportKey(type, key, unit);
 

diff --git a/sentry/src/main/java/io/sentry/metrics/MetricType.java b/sentry/src/main/java/io/sentry/metrics/MetricType.java
@@ -1,12 +1,19 @@
 package io.sentry.metrics;
 
 import org.jetbrains.annotations.ApiStatus;
+import org.jetbrains.annotations.NotNull;
 
 /** The metric instrument type */
 @ApiStatus.Internal
 public enum MetricType {
-  Counter,
-  Gauge,
-  Distribution,
-  Set
+  Counter("c"),
+  Gauge("g"),
+  Distribution("d"),
+  Set("s");
+
+  final @NotNull String statsdCode;
+
+  MetricType(final @NotNull String statsdCode) {
+    this.statsdCode = statsdCode;
+  }
 }
diff --git a/sentry/src/main/java/io/sentry/metrics/MetricsHelper.java b/sentry/src/main/java/io/sentry/metrics/MetricsHelper.java
@@ -89,28 +89,13 @@ public static String sanitizeTagValue(final @NotNull String input) {
     return output.toString();
   }
 
-  public static @NotNull String toStatsdType(final @NotNull MetricType type) {
-    switch (type) {
-      case Counter:
-        return "c";
-      case Gauge:
-        return "g";
-      case Distribution:
-        return "d";
-      case Set:
-        return "s";
-      default:
-        throw new IllegalArgumentException("Invalid Metric Type: " + type.name());
-    }
-  }
-
   @NotNull
   public static String getMetricBucketKey(
       final @NotNull MetricType type,
       final @NotNull String metricKey,
       final @Nullable MeasurementUnit unit,
       final @Nullable Map<String, String> tags) {
-    final @NotNull String typePrefix = toStatsdType(type);
+    final @NotNull String typePrefix = type.statsdCode;
     final @NotNull String serializedTags = getTagsKey(tags);
 
     final @NotNull String unitName = getUnitName(unit);
@@ -176,7 +161,7 @@ public static String getExportKey(
       final @NotNull String key,
       final @Nullable MeasurementUnit unit) {
     final @NotNull String unitName = getUnitName(unit);
-    return String.format("%s:%s@%s", toStatsdType(type), key, unitName);
+    return String.format("%s:%s@%s", type.statsdCode, key, unitName);
   }
 
   public static double convertNanosTo(
@@ -234,7 +219,7 @@ public static void encodeMetrics(
       }
 
       writer.append("|");
-      writer.append(toStatsdType(metric.getType()));
+      writer.append(metric.getType().statsdCode);
 
       final @Nullable Map<String, String> tags = metric.getTags();
       if (tags != null) {

diff --git a/sentry/src/main/java/io/sentry/protocol/SentryTransaction.java b/sentry/src/main/java/io/sentry/protocol/SentryTransaction.java
@@ -225,7 +225,7 @@ public void serialize(final @NotNull ObjectWriter writer, final @NotNull ILogger
       writer.name(JsonKeys.MEASUREMENTS).value(logger, measurements);
     }
     if (metricSummaries != null && !metricSummaries.isEmpty()) {
-      writer.name(SentrySpan.JsonKeys.METRICS_SUMMARY).value(logger, metricSummaries);
+      writer.name(JsonKeys.METRICS_SUMMARY).value(logger, metricSummaries);
     }
     writer.name(JsonKeys.TRANSACTION_INFO).value(logger, transactionInfo);
     new SentryBaseEvent.Serializer().serialize(this, writer, logger);

diff --git a/sentry/src/test/java/io/sentry/MetricsAggregatorTest.kt b/sentry/src/test/java/io/sentry/MetricsAggregatorTest.kt
@@ -300,13 +300,18 @@ class MetricsAggregatorTest {
         // then a flush is scheduled
         assertTrue(fixture.executorService.hasScheduledRunnables())
 
+        // flush is executed, but there are other metric to capture and it's scheduled again
+        fixture.executorService.runAll()
+        verify(fixture.client, never()).captureMetrics(any())
+        assertTrue(fixture.executorService.hasScheduledRunnables())
+
         // after the flush is executed, the metric is captured
         fixture.currentTimeMillis = 31_000
         fixture.executorService.runAll()
         verify(fixture.client).captureMetrics(any())
 
-        // and flushing is scheduled again
-        assertTrue(fixture.executorService.hasScheduledRunnables())
+        // there is no other metric to capture, so flush is not scheduled again
+        assertFalse(fixture.executorService.hasScheduledRunnables())
     }
 
     @Test
@@ -338,8 +343,7 @@ class MetricsAggregatorTest {
             key,
             value,
             unit,
-            tags,
-            timestamp
+            tags
         )
     }
 
@@ -373,8 +377,7 @@ class MetricsAggregatorTest {
             key,
             1.0,
             unit,
-            tags,
-            timestamp
+            tags
         )
 
         // if the same set metric is emitted again
@@ -394,8 +397,7 @@ class MetricsAggregatorTest {
             key,
             0.0,
             unit,
-            tags,
-            timestamp
+            tags
         )
     }
 
@@ -506,4 +508,12 @@ class MetricsAggregatorTest {
         aggregator.flush(true)
         verify(fixture.client, never()).captureMetrics(any())
     }
+
+    @Test
+    fun `if before emit throws, metric is emitted`() {
+        val aggregator = fixture.getSut(beforeEmitMetricCallback = { key, tags -> throw RuntimeException() })
+        aggregator.increment("key", 1.0, null, null, 20_001, null)
+        aggregator.flush(true)
+        verify(fixture.client).captureMetrics(any())
+    }
 }
diff --git a/sentry/src/test/java/io/sentry/metrics/LocalMetricsAggregatorTest.kt b/sentry/src/test/java/io/sentry/metrics/LocalMetricsAggregatorTest.kt
@@ -15,7 +15,6 @@ class LocalMetricsAggregatorTest {
         val tags0 = mapOf(
             "tag" to "value0"
         )
-        val timestamp = 0L
 
         // when a metric is emitted
         aggregator.add(
@@ -24,8 +23,7 @@ class LocalMetricsAggregatorTest {
             key,
             1.0,
             unit,
-            tags0,
-            timestamp
+            tags0
         )
 
         // and the same metric is emitted with different tags
@@ -38,8 +36,7 @@ class LocalMetricsAggregatorTest {
             key,
             1.0,
             unit,
-            tags1,
-            timestamp
+            tags1
         )
 
         // then the summary contain a single top level group for the metric
@@ -70,8 +67,7 @@ class LocalMetricsAggregatorTest {
             key,
             1.0,
             unit,
-            tags,
-            timestamp
+            tags
         )
 
         aggregator.add(
@@ -80,8 +76,7 @@ class LocalMetricsAggregatorTest {
             key,
             2.0,
             unit,
-            tags,
-            timestamp
+            tags
         )
 
         val metric = aggregator.summaries.values.first()[0]

diff --git a/sentry/src/test/java/io/sentry/metrics/MetricsHelperTest.kt b/sentry/src/test/java/io/sentry/metrics/MetricsHelperTest.kt
@@ -177,10 +177,10 @@ class MetricsHelperTest {
 
     @Test
     fun toStatsdType() {
-        assertEquals("c", MetricsHelper.toStatsdType(MetricType.Counter))
-        assertEquals("g", MetricsHelper.toStatsdType(MetricType.Gauge))
-        assertEquals("s", MetricsHelper.toStatsdType(MetricType.Set))
-        assertEquals("d", MetricsHelper.toStatsdType(MetricType.Distribution))
+        assertEquals("c", MetricType.Counter.statsdCode)
+        assertEquals("g", MetricType.Gauge.statsdCode)
+        assertEquals("s", MetricType.Set.statsdCode)
+        assertEquals("d", MetricType.Distribution.statsdCode)
     }
 
     @Test