-
-
Notifications
You must be signed in to change notification settings - Fork 465
Add autoTransactionDeadlineTimeoutMillis option #4555
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
Add autoTransactionDeadlineTimeoutMillis option #4555
Conversation
Co-authored-by: stefano.siano <stefano.siano@sentry.io>
Co-authored-by: stefano.siano <stefano.siano@sentry.io>
small cleanup updated changelog
Performance metrics 🚀
|
|
@sentry review |
PR DescriptionThis pull request introduces a new configuration option, Click to see moreKey Technical Changes
Architecture DecisionsThe decision was made to add a new option to Dependencies and InteractionsThis change primarily affects the transaction creation process within the Android SDK. It interacts with Risk ConsiderationsA potential risk is that setting a very long or infinite deadline timeout (by setting the option to 0 or a negative value) could lead to transactions remaining open indefinitely, potentially impacting performance and resource usage. Users should be aware of this when configuring the option. Another consideration is ensuring that the new option is properly documented and communicated to users to avoid confusion. Notable Implementation DetailsThe implementation handles zero and negative values for |
|
|
||
| // Set deadline timeout based on configured option | ||
| val deadlineTimeoutMillis = scopes.options.autoTransactionDeadlineTimeoutMillis | ||
| // No deadline when zero or negative value is set | ||
| it.deadlineTimeout = if (deadlineTimeoutMillis <= 0) null else deadlineTimeoutMillis |
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.
The Kotlin implementation follows the same pattern as the Java implementations, which is good for consistency. However, consider extracting the deadline timeout logic into a utility method to reduce code duplication across ActivityLifecycleIntegration, SentryGestureListener, and SentryNavigationListener.
| // Set deadline timeout based on configured option | |
| val deadlineTimeoutMillis = scopes.options.autoTransactionDeadlineTimeoutMillis | |
| // No deadline when zero or negative value is set | |
| it.deadlineTimeout = if (deadlineTimeoutMillis <= 0) null else deadlineTimeoutMillis | |
| // Consider creating a utility method like: | |
| // private fun getDeadlineTimeoutFromOptions(options: SentryOptions): Long? { | |
| // val deadlineTimeoutMillis = options.autoTransactionDeadlineTimeoutMillis | |
| // return if (deadlineTimeoutMillis <= 0) null else deadlineTimeoutMillis | |
| // } |
Did we get this right? 👍 / 👎 to inform future reviews.
sentry-android-core/src/test/java/io/sentry/android/core/ActivityLifecycleIntegrationTest.kt
Show resolved
Hide resolved
sentry-android-core/src/test/java/io/sentry/android/core/ManifestMetadataReaderTest.kt
Outdated
Show resolved
Hide resolved
sentry-android-core/src/main/java/io/sentry/android/core/ManifestMetadataReader.java
Outdated
Show resolved
Hide resolved
romtsn
left a comment
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.
lgtm, besides the naming.
📜 Description
Adds
autoTransactionDeadlineTimeoutMillistoSentryOptionsto control the deadline for automatic transactions (Activity, Gesture, Navigation).It defaults to 30 seconds, and a value
<= 0disables the deadline entirely.💡 Motivation and Context
Fixes #4251
💚 How did you test it?
Unit tests
📝 Checklist
sendDefaultPIIis enabled.🔮 Next steps