Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Metrics clean #3403

Merged
merged 3 commits into from
May 3, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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))

Expand Down
3 changes: 1 addition & 2 deletions sentry/api/sentry.api
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}

Expand Down Expand Up @@ -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 {
Expand Down
12 changes: 8 additions & 4 deletions sentry/src/main/java/io/sentry/MetricsAggregator.java
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
}

Expand Down Expand Up @@ -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();
Expand Down Expand Up @@ -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);
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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);

Expand Down
15 changes: 11 additions & 4 deletions sentry/src/main/java/io/sentry/metrics/MetricType.java
Original file line number Diff line number Diff line change
@@ -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;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: I separated MetricType from Statsd on purpose: If we ever decide to use a different protocol, we would only need to refactor some helpers and transport specific code. But I'm still fine with doing it this way, as that's what other SDKs are doing as well.

}
}
21 changes: 3 additions & 18 deletions sentry/src/main/java/io/sentry/metrics/MetricsHelper.java
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -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(
Expand Down Expand Up @@ -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) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
26 changes: 18 additions & 8 deletions sentry/src/test/java/io/sentry/MetricsAggregatorTest.kt
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -338,8 +343,7 @@ class MetricsAggregatorTest {
key,
value,
unit,
tags,
timestamp
tags
)
}

Expand Down Expand Up @@ -373,8 +377,7 @@ class MetricsAggregatorTest {
key,
1.0,
unit,
tags,
timestamp
tags
)

// if the same set metric is emitted again
Expand All @@ -394,8 +397,7 @@ class MetricsAggregatorTest {
key,
0.0,
unit,
tags,
timestamp
tags
)
}

Expand Down Expand Up @@ -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())
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@ class LocalMetricsAggregatorTest {
val tags0 = mapOf(
"tag" to "value0"
)
val timestamp = 0L

// when a metric is emitted
aggregator.add(
Expand All @@ -24,8 +23,7 @@ class LocalMetricsAggregatorTest {
key,
1.0,
unit,
tags0,
timestamp
tags0
)

// and the same metric is emitted with different tags
Expand All @@ -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
Expand Down Expand Up @@ -70,8 +67,7 @@ class LocalMetricsAggregatorTest {
key,
1.0,
unit,
tags,
timestamp
tags
)

aggregator.add(
Expand All @@ -80,8 +76,7 @@ class LocalMetricsAggregatorTest {
key,
2.0,
unit,
tags,
timestamp
tags
)

val metric = aggregator.summaries.values.first()[0]
Expand Down
8 changes: 4 additions & 4 deletions sentry/src/test/java/io/sentry/metrics/MetricsHelperTest.kt
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Loading