diff --git a/CHANGELOG.md b/CHANGELOG.md index f72b0fad7f..38989311d9 100644 --- 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 index 61cf316b6c..121ef1af76 100644 --- 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 ()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 index 9d5f277477..71e5a40f3f 100644 --- 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 index ee0f24eaae..2afbf8e19e 100644 --- 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 tags, - final long timestampMs) { + final @Nullable Map 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 index 56fb10ee1e..fc816bc25a 100644 --- 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 index f3a56c119e..46e22ca8ad 100644 --- 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 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 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 index 39e7704bd5..0ca789270e 100644 --- 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 index e757a52d61..1b2b2f36e1 100644 --- 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 index d5bee5d2f8..83c5d3864d 100644 --- 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 index a164b38d7b..8112170f68 100644 --- 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