Skip to content

feat(metrics): [Trace Metrics 24] PR Feedback #5013

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.

Sign up for GitHub

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

Merged
adinauer merged 1 commit into from
Jan 14, 2026
Merged

feat(metrics): [Trace Metrics 24] PR Feedback #5013

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
2 changes: 1 addition & 1 deletion sentry-android-core/src/main/java/io/sentry/android/core/AndroidMetricsBatchProcessor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ public final class AndroidMetricsBatchProcessor extends MetricsBatchProcessor
implements AppState.AppStateListener {

public AndroidMetricsBatchProcessor(
@NotNull SentryOptions options, @NotNull ISentryClient client) {
final @NotNull SentryOptions options, final @NotNull ISentryClient client) {
super(options, client);
AppState.getInstance().addAppStateListener(this);
}
Expand Down
2 changes: 1 addition & 1 deletion ...ndroid-core/src/main/java/io/sentry/android/core/AndroidMetricsBatchProcessorFactory.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@
public final class AndroidMetricsBatchProcessorFactory implements IMetricsBatchProcessorFactory {
@Override
public @NotNull IMetricsBatchProcessor create(
@NotNull SentryOptions options, @NotNull SentryClient client) {
final @NotNull SentryOptions options, final @NotNull SentryClient client) {
return new AndroidMetricsBatchProcessor(options, client);
}
}
22 changes: 11 additions & 11 deletions sentry/src/main/java/io/sentry/SentryLogEvent.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ public void setTimestamp(final @NotNull Double timestamp) {
return body;
}

public void setBody(@NotNull String body) {
public void setBody(final @NotNull String body) {
this.body = body;
}

Expand Down Expand Up @@ -88,7 +88,7 @@ public void setAttribute(
return severityNumber;
}

public void setSeverityNumber(@Nullable Integer severityNumber) {
public void setSeverityNumber(final @Nullable Integer severityNumber) {
this.severityNumber = severityNumber;
}

Expand Down Expand Up @@ -120,7 +120,7 @@ public void serialize(final @NotNull ObjectWriter writer, final @NotNull ILogger

if (unknown != null) {
for (String key : unknown.keySet()) {
Object value = unknown.get(key);
final Object value = unknown.get(key);
writer.name(key).value(logger, value);
}
}
Expand Down Expand Up @@ -185,29 +185,29 @@ public static final class Deserializer implements JsonDeserializer<SentryLogEven
reader.endObject();

if (traceId == null) {
String message = "Missing required field \"" + JsonKeys.TRACE_ID + "\"";
Exception exception = new IllegalStateException(message);
final String message = "Missing required field \"" + JsonKeys.TRACE_ID + "\"";
final Exception exception = new IllegalStateException(message);
logger.log(SentryLevel.ERROR, message, exception);
throw exception;
}

if (timestamp == null) {
String message = "Missing required field \"" + JsonKeys.TIMESTAMP + "\"";
Exception exception = new IllegalStateException(message);
final String message = "Missing required field \"" + JsonKeys.TIMESTAMP + "\"";
final Exception exception = new IllegalStateException(message);
logger.log(SentryLevel.ERROR, message, exception);
throw exception;
}

if (body == null) {
String message = "Missing required field \"" + JsonKeys.BODY + "\"";
Exception exception = new IllegalStateException(message);
final String message = "Missing required field \"" + JsonKeys.BODY + "\"";
final Exception exception = new IllegalStateException(message);
logger.log(SentryLevel.ERROR, message, exception);
throw exception;
}

if (level == null) {
String message = "Missing required field \"" + JsonKeys.LEVEL + "\"";
Exception exception = new IllegalStateException(message);
final String message = "Missing required field \"" + JsonKeys.LEVEL + "\"";
final Exception exception = new IllegalStateException(message);
logger.log(SentryLevel.ERROR, message, exception);
throw exception;
}
Expand Down
4 changes: 2 additions & 2 deletions sentry/src/main/java/io/sentry/SentryLogEventAttributeValue.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,8 +99,8 @@ public static final class Deserializer implements JsonDeserializer<SentryLogEven
reader.endObject();

if (type == null) {
String message = "Missing required field \"" + JsonKeys.TYPE + "\"";
Exception exception = new IllegalStateException(message);
final String message = "Missing required field \"" + JsonKeys.TYPE + "\"";
final Exception exception = new IllegalStateException(message);
logger.log(SentryLevel.ERROR, message, exception);
throw exception;
}
Expand Down
4 changes: 2 additions & 2 deletions sentry/src/main/java/io/sentry/SentryLogEvents.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,8 +81,8 @@ public static final class Deserializer implements JsonDeserializer<SentryLogEven
reader.endObject();

if (items == null) {
String message = "Missing required field \"" + JsonKeys.ITEMS + "\"";
Exception exception = new IllegalStateException(message);
final String message = "Missing required field \"" + JsonKeys.ITEMS + "\"";
final Exception exception = new IllegalStateException(message);
logger.log(SentryLevel.ERROR, message, exception);
throw exception;
}
Expand Down
30 changes: 15 additions & 15 deletions sentry/src/main/java/io/sentry/SentryMetricsEvent.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,39 +79,39 @@ public void setTimestamp(final @NotNull Double timestamp) {
return name;
}

public void setName(@NotNull String name) {
public void setName(final @NotNull String name) {
this.name = name;
}

public @NotNull String getType() {
return type;
}

public void setType(@NotNull String type) {
public void setType(final @NotNull String type) {
this.type = type;
}

public @Nullable String getUnit() {
return unit;
}

public void setUnit(@Nullable String unit) {
public void setUnit(final @Nullable String unit) {
this.unit = unit;
}

public @Nullable SpanId getSpanId() {
return spanId;
}

public void setSpanId(@Nullable SpanId spanId) {
public void setSpanId(final @Nullable SpanId spanId) {
this.spanId = spanId;
}

public @NotNull Double getValue() {
return value;
}

public void setValue(@NotNull Double value) {
public void setValue(final @NotNull Double value) {
this.value = value;
}

Expand Down Expand Up @@ -241,36 +241,36 @@ public static final class Deserializer implements JsonDeserializer<SentryMetrics
reader.endObject();

if (traceId == null) {
String message = "Missing required field \"" + JsonKeys.TRACE_ID + "\"";
Exception exception = new IllegalStateException(message);
final String message = "Missing required field \"" + JsonKeys.TRACE_ID + "\"";
final Exception exception = new IllegalStateException(message);
logger.log(SentryLevel.ERROR, message, exception);
throw exception;
}

if (timestamp == null) {
String message = "Missing required field \"" + JsonKeys.TIMESTAMP + "\"";
Exception exception = new IllegalStateException(message);
final String message = "Missing required field \"" + JsonKeys.TIMESTAMP + "\"";
final Exception exception = new IllegalStateException(message);
logger.log(SentryLevel.ERROR, message, exception);
throw exception;
}

if (type == null) {
String message = "Missing required field \"" + JsonKeys.TYPE + "\"";
Exception exception = new IllegalStateException(message);
final String message = "Missing required field \"" + JsonKeys.TYPE + "\"";
final Exception exception = new IllegalStateException(message);
logger.log(SentryLevel.ERROR, message, exception);
throw exception;
}

if (name == null) {
String message = "Missing required field \"" + JsonKeys.NAME + "\"";
Exception exception = new IllegalStateException(message);
final String message = "Missing required field \"" + JsonKeys.NAME + "\"";
final Exception exception = new IllegalStateException(message);
logger.log(SentryLevel.ERROR, message, exception);
throw exception;
}

if (value == null) {
String message = "Missing required field \"" + JsonKeys.VALUE + "\"";
Exception exception = new IllegalStateException(message);
final String message = "Missing required field \"" + JsonKeys.VALUE + "\"";
final Exception exception = new IllegalStateException(message);
logger.log(SentryLevel.ERROR, message, exception);
throw exception;
}
Expand Down
4 changes: 2 additions & 2 deletions sentry/src/main/java/io/sentry/SentryMetricsEvents.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,8 +81,8 @@ public static final class Deserializer implements JsonDeserializer<SentryMetrics
reader.endObject();

if (items == null) {
String message = "Missing required field \"" + JsonKeys.ITEMS + "\"";
Exception exception = new IllegalStateException(message);
final String message = "Missing required field \"" + JsonKeys.ITEMS + "\"";
final Exception exception = new IllegalStateException(message);
logger.log(SentryLevel.ERROR, message, exception);
throw exception;
}
Expand Down
6 changes: 3 additions & 3 deletions sentry/src/main/java/io/sentry/SentryOptions.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3771,7 +3771,7 @@ public boolean isEnabled() {
*
* @param enableMetrics true if Sentry Metrics should be enabled
*/
public void setEnabled(boolean enableMetrics) {
public void setEnabled(final boolean enableMetrics) {
this.enable = enableMetrics;
}

Expand All @@ -3789,7 +3789,7 @@ public void setEnabled(boolean enableMetrics) {
*
* @param beforeSend the beforeSend callback for metrics
*/
public void setBeforeSend(@Nullable BeforeSendMetricCallback beforeSend) {
public void setBeforeSend(final @Nullable BeforeSendMetricCallback beforeSend) {
this.beforeSend = beforeSend;
}

Expand All @@ -3813,7 +3813,7 @@ public interface BeforeSendMetricCallback {
* @return the original metric, mutated metric or null if metric was dropped
*/
@Nullable
SentryMetricsEvent execute(@NotNull SentryMetricsEvent metric);
SentryMetricsEvent execute(final @NotNull SentryMetricsEvent metric);
}
}

Expand Down
2 changes: 1 addition & 1 deletion sentry/src/main/java/io/sentry/metrics/DefaultMetricsBatchProcessorFactory.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
public final class DefaultMetricsBatchProcessorFactory implements IMetricsBatchProcessorFactory {
@Override
public @NotNull IMetricsBatchProcessor create(
@NotNull SentryOptions options, @NotNull SentryClient client) {
final @NotNull SentryOptions options, final @NotNull SentryClient client) {
return new MetricsBatchProcessor(options, client);
}
}
2 changes: 1 addition & 1 deletion sentry/src/main/java/io/sentry/metrics/IMetricsBatchProcessor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ public interface IMetricsBatchProcessor {
void close(boolean isRestarting);

/**
* Flushes log events.
* Flushes metrics.
*
* @param timeoutMillis time in milliseconds
*/
Expand Down
5 changes: 5 additions & 0 deletions sentry/src/main/java/io/sentry/metrics/MetricsBatchProcessor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ public class MetricsBatchProcessor implements IMetricsBatchProcessor {
private static final @NotNull AutoClosableReentrantLock scheduleLock =
new AutoClosableReentrantLock();
private volatile boolean hasScheduled = false;
private volatile boolean isShuttingDown = false;

private final @NotNull ReusableCountLatch pendingCount = new ReusableCountLatch();

Expand All @@ -51,6 +52,9 @@ public MetricsBatchProcessor(

@Override
public void add(final @NotNull SentryMetricsEvent metricsEvent) {
if (isShuttingDown) {
return;
}
Comment on lines +55 to +57
Copy link

@sentry sentry bot Jan 14, 2026

Choose a reason for hiding this comment

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

Bug: A race condition in MetricsBatchProcessor.close(false) can cause metrics to be silently dropped during shutdown if a metric is added after the final flush has completed.
Severity: HIGH

Suggested Fix

To fix this, consider synchronizing the check for isShuttingDown and the addition to the queue within the add method. Alternatively, the shutdown logic could be revised to ensure that any items added to the queue after the executor is closed are still flushed, potentially by re-checking the queue in a synchronized block after the initial flush loop in close completes.

Prompt for AI Agent 
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.

Location: sentry/src/main/java/io/sentry/metrics/MetricsBatchProcessor.java#L55-L57

Potential issue: A race condition exists in the `close(false)` method of
`MetricsBatchProcessor`. A metric-producing thread can pass the initial `isShuttingDown`
check just before a separate shutdown thread sets the flag to true, closes the executor,
and drains the queue. If the metric-producing thread then adds its metric to the queue,
the metric will be stranded. The subsequent call to `maybeSchedule` will fail with a
`RejectedExecutionException` because the executor is closed, leading to silent metric
loss and an inconsistent `pendingCount`.

Did we get this right? 👍 / 👎 to inform future reviews.

if (pendingCount.getCount() >= MAX_QUEUE_SIZE) {
options
.getClientReportRecorder()
Expand All @@ -65,6 +69,7 @@ public void add(final @NotNull SentryMetricsEvent metricsEvent) {
@SuppressWarnings("FutureReturnValueIgnored")
@Override
public void close(final boolean isRestarting) {
isShuttingDown = true;
if (isRestarting) {
maybeSchedule(true, true);
executorService.submit(() -> executorService.close(options.getShutdownTimeoutMillis()));
Expand Down
Loading