-
-
Notifications
You must be signed in to change notification settings - Fork 465
feat(metrics): [Trace Metrics 15] Android Metrics batch processor and factory #4994
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
base: 12-19-metrics_manifest_options_for_android
Are you sure you want to change the base?
Conversation
Instructions and example for changelogPlease add an entry to Example: ## Unreleased
### Features
- [Trace Metrics 15] Android Metrics batch processor and factory ([#4994](https://github.com/getsentry/sentry-java/pull/4994))If none of the above apply, you can opt out of this check by adding |
Performance metrics 🚀
|
6788ab9 to
8a6b00f
Compare
c3c0f3b to
0116a6b
Compare
8a6b00f to
870ba66
Compare
0116a6b to
56879b3
Compare
| } | ||
|
|
||
| @Override | ||
| public void onForeground() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: The SentryClient does not call close() or flush() on the new metricsBatchProcessor during shutdown, leading to memory leaks, data loss, and potential runtime errors.
Severity: CRITICAL
Suggested Fix
In SentryClient.java, add a call to metricsBatchProcessor.flush(timeoutMillis) within the flush() method and a call to metricsBatchProcessor.close(isRestarting) within the close() method. This will mirror the existing cleanup logic for loggerBatchProcessor.
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-android-core/src/main/java/io/sentry/android/core/AndroidMetricsBatchProcessor.java#L21
Potential issue: When metrics are enabled, the `SentryClient` initializes a
`metricsBatchProcessor` but fails to clean it up during shutdown. The
`SentryClient.close()` method does not call `metricsBatchProcessor.close()`, and
`SentryClient.flush()` does not call `metricsBatchProcessor.flush()`. This leads to
several issues on Android: a memory leak because the `AndroidMetricsBatchProcessor`
remains registered as an `AppStateListener`, potential runtime errors if the app
backgrounds after the client is closed, and data loss as queued metrics are never
flushed.
Did we get this right? 👍 / 👎 to inform future reviews.
|
cursor review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✅ Bugbot reviewed your changes and found no bugs!

📜 Description
💡 Motivation and Context
💚 How did you test it?
📝 Checklist
sendDefaultPIIis enabled.🔮 Next steps