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

[adinauer]

📜 Description

💡 Motivation and Context

💚 How did you test it?

📝 Checklist

• I added GH Issue ID & Linear ID
• I added tests to verify the changes.
• No new PII added or SDK only sends newly added PII if sendDefaultPII is enabled.
• I updated the docs if needed.
• I updated the wizard if needed.
• Review from the native team if needed.
• No breaking change or entry added to the changelog.
• No breaking change for hybrid SDKs or communicated to hybrid SDKs.

🔮 Next steps

[github-actions]

	Fails
🚫	Please consider adding a changelog entry for the next release.

	Messages
📖	Do not forget to update Sentry-docs with your feature once the pull request gets approved.

Instructions and example for changelog

Please add an entry to CHANGELOG.md to the "Unreleased" section. Make sure the entry includes this PR's number.

Example:

## Unreleased

### Features

- [Trace Metrics 24] PR Feedback ([#5013](https://github.com/getsentry/sentry-java/pull/5013))

If none of the above apply, you can opt out of this check by adding #skip-changelog to the PR description or adding a skip-changelog label.

Generated by 🚫 dangerJS against f502ffe

[adinauer]

• feat(metrics): [Trace Metrics 27] Automatically use SentryOptions.Metrics.BeforeSendMetricCallback Spring beans #5021 : 2 dependent PRs (#5022 , #5024 )
• feat(metrics): [Trace Metrics 26] SentryMetricsParameters create shortcut for attributes map #5020
• feat(metrics): [Trace Metrics 25] Add Hint to Metrics #5019
• feat(metrics): [Trace Metrics 24] PR Feedback #5013 👈 (View in Graphite)
• feat(metrics): [Trace Metrics 23] Add E2E tests for Metrics #5011
• feat(metrics): [Trace Metrics 22] Close and flush MetricsBatchProcessor from SentryClient #5010
• Use uv for managing python version #5009
• feat(metrics): [Trace Metrics 21] Add AI rules files for Metrics (and update feature flags rule) #5008
• feat(metrics): [Trace Metrics 20] Add device and OS attributes to metrics #5007
• feat(metrics): [Trace Metrics 19] Create SentryMetricsParameters for Metrics #5006
• feat(metrics): [Trace Metrics 18] Add unit tests for Metrics #5005
• feat(metrics): [Trace Metrics 17] Remove Experimental Annotation from Metrics Options #4996
• feat(metrics): [Trace Metrics 16] Only send user attributes if sendDefaultPii is true #4995
• feat(metrics): [Trace Metrics 15] Android Metrics batch processor and factory #4994
• feat(metrics): [Trace Metrics 14] Metrics Manifest options for Android #4993
• feat(metrics): [Trace Metrics 13] Metrics external options #4992
• feat(metrics): [Trace Metrics 12] Record client report for discarded metrics envelope item #4991
• feat(metrics): [Trace Metrics 11] Metrics Envelope item deserialization #4990
• feat(metrics): [Trace Metrics 10] Metrics Gauge API #4989
• feat(metrics): [Trace Metrics 9] Metrics Distribution API #4988
• feat(metrics): [Trace Metrics 8] Metrics Count API #4987
• feat(metrics): [Trace Metrics 7] Metrics counter basic API #4986
• feat(metrics): [Trace Metrics 6] Implement SentryClient for Metrics #4985
• feat(metrics): [Trace Metrics 5] Add batch processor for Metrics #4984
• feat(metrics): [Trace Metrics 4] Add transport types for metrics #4983
• feat(metrics): [Trace Metrics 3] Enable metrics by default #4982
• feat(metrics): [Trace Metrics 2] MetricsApi stub #4981
• feat(metrics): [Trace Metrics 1] Metrics Options #4980
• feat/trace-metrics

This stack of pull requests is managed by Graphite. Learn more about stacking.

[github-actions]

Performance metrics 🚀

	Plain	With Sentry	Diff
Startup time	327.59 ms	385.98 ms	58.39 ms
Size	1.58 MiB	2.20 MiB	635.32 KiB

Baseline results on branch: 01-08-add_e2e_tests_for_metrics

Startup times

Revision	Plain	With Sentry	Diff
527a476	352.67 ms	429.67 ms	77.00 ms

App size

Revision	Plain	With Sentry	Diff
527a476	1.58 MiB	2.13 MiB	561.00 KiB

[sentry]

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.}

[adinauer]

cursor review

[cursor]

✅ Bugbot reviewed your changes and found no bugs!