-
-
Notifications
You must be signed in to change notification settings - Fork 456
feat(android-distribution): Add module foundation with compilation stubs #4712
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: main
Are you sure you want to change the base?
Conversation
|
* Provides functionality to check for app updates and download new versions from Sentry's preprod | ||
* artifacts system. | ||
*/ | ||
public object Distribution { |
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.
This is almost exactly the same API as in the Emerge Build Distribution.
public fun downloadUpdate(context: Context, info: UpdateInfo) { | ||
val browserIntent = Intent(Intent.ACTION_VIEW, Uri.parse(info.downloadUrl)) | ||
browserIntent.addFlags(Intent.FLAG_ACTIVITY_NEW_TASK) | ||
context.startActivity(browserIntent) | ||
} |
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.
Potential bug: The downloadUpdate
method calls startActivity
without handling a potential ActivityNotFoundException
, which can crash the app if no browser is installed.
-
Description: The
downloadUpdate
method, part of the public SDK API, initiates a download by callingstartActivity
with anACTION_VIEW
intent. However, it lacks error handling forActivityNotFoundException
. This exception is thrown if no application on the device can handle the HTTP/HTTPS URL, such as when no web browser is installed. Because this is an unhandled exception in a public SDK method, it will propagate up and crash the host application. -
Suggested fix: Wrap the
startActivity(intent)
call in atry-catch
block to handleActivityNotFoundException
. Alternatively, check if an activity can handle the intent by callingcontext.packageManager.resolveActivity(intent, 0)
before callingstartActivity
.
severity: 0.6, confidence: 0.9
Did we get this right? 👍 / 👎 to inform future reviews.
Performance metrics 🚀
|
Revision | Plain | With Sentry | Diff |
---|---|---|---|
ee747ae | 405.43 ms | 485.70 ms | 80.28 ms |
ee747ae | 357.79 ms | 421.84 ms | 64.05 ms |
85d7417 | 347.21 ms | 394.35 ms | 47.15 ms |
b750b96 | 408.98 ms | 480.32 ms | 71.34 ms |
ee747ae | 358.21 ms | 389.41 ms | 31.20 ms |
ee747ae | 415.92 ms | 470.15 ms | 54.23 ms |
b750b96 | 421.25 ms | 444.09 ms | 22.84 ms |
c8125f3 | 383.82 ms | 441.66 ms | 57.84 ms |
ee747ae | 554.98 ms | 611.50 ms | 56.52 ms |
7314dbe | 437.83 ms | 505.64 ms | 67.81 ms |
App size
Revision | Plain | With Sentry | Diff |
---|---|---|---|
ee747ae | 1.58 MiB | 2.10 MiB | 530.95 KiB |
ee747ae | 1.58 MiB | 2.10 MiB | 530.95 KiB |
85d7417 | 1.58 MiB | 2.10 MiB | 533.44 KiB |
b750b96 | 1.58 MiB | 2.10 MiB | 533.19 KiB |
ee747ae | 1.58 MiB | 2.10 MiB | 530.95 KiB |
ee747ae | 1.58 MiB | 2.10 MiB | 530.95 KiB |
b750b96 | 1.58 MiB | 2.10 MiB | 533.20 KiB |
c8125f3 | 1.58 MiB | 2.10 MiB | 532.32 KiB |
ee747ae | 1.58 MiB | 2.10 MiB | 530.95 KiB |
7314dbe | 1.58 MiB | 2.10 MiB | 533.45 KiB |
Previous results on branch: no/distribution-module-foundation
Startup times
Revision | Plain | With Sentry | Diff |
---|---|---|---|
de2fe17 | 388.06 ms | 432.06 ms | 44.00 ms |
c6cf0b8 | 366.29 ms | 420.61 ms | 54.32 ms |
a18e682 | 395.94 ms | 443.64 ms | 47.70 ms |
3ea8703 | 376.34 ms | 437.15 ms | 60.81 ms |
0ed1688 | 397.64 ms | 447.40 ms | 49.76 ms |
App size
Revision | Plain | With Sentry | Diff |
---|---|---|---|
de2fe17 | 1.58 MiB | 2.10 MiB | 532.30 KiB |
c6cf0b8 | 1.58 MiB | 2.10 MiB | 532.37 KiB |
a18e682 | 1.58 MiB | 2.10 MiB | 532.50 KiB |
3ea8703 | 1.58 MiB | 2.10 MiB | 532.37 KiB |
0ed1688 | 1.58 MiB | 2.10 MiB | 532.30 KiB |
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.
Boilerplate lgtm! I think it would be good to get a thumbs from SDK team just on the API surface before landing this.
<application> | ||
<provider | ||
android:name="io.sentry.android.distribution.DistributionContentProvider" | ||
android:authorities="${applicationId}.sentry-distribution-provider" |
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.
Looks like we can make this a bit shorter:
android:name=".SentryPerformanceProvider" |
- Just
.SentryDistributionProvider
for name ${applicationId}.SentryDistributionProvider
for android:authorities to match prtg
@@ -0,0 +1,12 @@ | |||
<?xml version="1.0" encoding="utf-8"?> | |||
<manifest xmlns:android="http://schemas.android.com/apk/res/android"> | |||
<uses-permission android:name="android.permission.INTERNET" /> |
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.
I think it's fine to keep this either way but I was wondering - the requirement is you have to have the main Sentry SDK right? You won't be able to have just distro.
* @param context Android context | ||
* @param options Configuration options for build distribution | ||
*/ | ||
public fun init(context: Context, options: DistributionOptions) { |
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.
I don't know how well the API surface matches the rest of the Sentry SDK but if it looks good to one of the Sentry folks then fine by me.
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.
For sentry android we also use the following pattern:
SentryAndroid.init(context) { config ->
// config.xyz =
}
In pure Java uses cases this is a bit more readable and easier to maintain when options change a lot, at least when compared against a plain new DistributionOptions()
ctor, as you need to name the properties. But with named args in Kotlin, we can use this approach too.
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.
Left a few nits, but looks quite good to me!
* the Distribution SDK is available without requiring manual initialization in | ||
* Application.onCreate(). | ||
*/ | ||
public class DistributionContentProvider : ContentProvider() { |
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.
It might be worth having a look at EmptySecureContentProvider
, I remember we had some security issue reports around default content providers.
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.
Good idea!
* @param sentryBaseUrl Base URL for Sentry API (defaults to https://sentry.io) | ||
* @param buildConfiguration Optional build configuration name for filtering | ||
*/ | ||
public data class DistributionOptions( |
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.
It's kinda discouraged (e.g. see this article) to use data classes for public APIs, I'm not sure how often this changes - but maybe makes sense to not use it here too.
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.
Good point!
* @param context Android context | ||
* @param options Configuration options for build distribution | ||
*/ | ||
public fun init(context: Context, options: DistributionOptions) { |
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.
For sentry android we also use the following pattern:
SentryAndroid.init(context) { config ->
// config.xyz =
}
In pure Java uses cases this is a bit more readable and easier to maintain when options change a lot, at least when compared against a plain new DistributionOptions()
ctor, as you need to name the properties. But with named args in Kotlin, we can use this approach too.
* @param context Android context | ||
* @return UpdateStatus indicating if an update is available, up to date, or error | ||
*/ | ||
public fun checkForUpdate(context: Context): UpdateStatus { |
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.
nit: I'd make this super clear that this is a blocking call
public fun checkForUpdate(context: Context): UpdateStatus { | |
public fun checkForUpdateBlocking(context: Context): UpdateStatus { |
* @param context Android context | ||
* @return CompletableFuture with UpdateStatus result | ||
*/ | ||
public fun checkForUpdateCompletableFuture(context: Context): CompletableFuture<UpdateStatus> { |
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.
nit: As a beginner I would prefer a simple callback mechanism instead.
public fun checkForUpdateCompletableFuture(context: Context): CompletableFuture<UpdateStatus> { | |
public fun checkForUpdate(context: Context, onUpdateStatusLoaded: (UpdateStatus) -> Unit) { |
* @param context Android context | ||
* @return UpdateStatus indicating if an update is available, up to date, or error | ||
*/ | ||
public fun checkForUpdate(context: Context): UpdateStatus { |
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.
Right now triggering the update check and getting the latest update status is one operation. I'm wondering what the ideal scenario would look like, and if it could make sense to split those two.
- If the ContentProvider would trigger an update check automatically during init, it could make sense to have a
registerUpdateListener()
API, which emits the latest status (if any) + and future triggered checks - If you expect people to add "check for update" buttons to their app the existing APIs would better suit this
val organizationSlug: String, | ||
val projectSlug: String, | ||
val sentryBaseUrl: String = "https://sentry.io", | ||
val buildConfiguration: String? = null, |
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.
What could be an example value for this?
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.
buildConfiguration is like debug
or release
. You can only check for updates with the same build configuration. You can’t be on a debug
build and pull a release
update.
* Provides functionality to check for app updates and download new versions from Sentry's preprod | ||
* artifacts system. | ||
*/ | ||
public object Distribution { |
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.
For new-ish features like logging and replay, we're exposing top level entry points under the Sentry class. E.g. Sentry.logger()
and Sentry.replay()
. It could make sense to do the same for distribution as well.
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.
This is a good point, I looked in to it and it seems like a bigger change, I will add this in a future PR.
This PR establishes the foundational structure for the sentry-android-distribution module with compilation stubs that enable parallel development of individual components. ### Changes - Android module configuration with necessary dependencies - AndroidManifest.xml with ContentProvider for auto-initialization - Distribution object with init(), isEnabled(), checkForUpdate() methods - DistributionOptions data class for configuration - UpdateStatus sealed class for result types - UpdateInfo data class for update details - Internal stub implementations that compile successfully ### Implementation Strategy - All methods return placeholder errors ("Implementation coming in future PR") - Follows zero-dependency design (only depends on sentry module) - Enables parallel development of binary identifier, HTTP client, API models, and core logic 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
- Fix ActivityNotFoundException in downloadUpdate method - Update AndroidManifest provider to use shorter naming convention - Add EmptySecureContentProvider for security - Convert DistributionOptions from data class to regular class - Add initOrder comment explaining initialization sequence 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
- Add lambda-based init pattern matching SentryAndroid.init - Rename checkForUpdate to checkForUpdateBlocking for clarity - Replace CompletableFuture with simple callback approach - Convert DistributionOptions to mutable builder pattern - Add example for buildConfiguration parameter 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
- Add automatic distribution module detection in SentryAndroid.java - Create DistributionIntegration for seamless auto-enablement when module present - Add Sentry.distribution() top-level API using reflection for build-time safety - Remove ContentProvider approach in favor of Integration pattern - Update Distribution API to use callback-based async methods - Fix ActivityNotFoundException handling in downloadUpdate method Follows existing patterns from replay/timber/fragment integrations for consistency. Module works automatically when included, provides compile errors when not. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
c1d435c
to
b68cb00
Compare
…irect instantiation - Remove sentry-android-core dependency from distribution module (only needs sentry module) - Add distribution as compileOnly dependency in sentry-android-core - Use direct DistributionIntegration instantiation instead of reflection - Eliminates circular dependency and follows same pattern as other integrations The distribution module only needs Integration/IScopes/SentryOptions from core sentry, not anything from sentry-android-core, making the architecture cleaner. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
No longer needed since this is a single PR implementation. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
* | ||
* @return The distribution API object that provides update checking functionality | ||
*/ | ||
public static @Nullable Object distribution() { |
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.
This method uses reflection whereas the replay()
and logger()
methods work by calling no-op APIs when the classes aren’t available. To save the extra boilerplate of creating the interfaces and no-op classes, I just used reflection. Thoughts?
Added the missing isDistributionAvailable parameter (set to false) to installDefaultIntegrations method calls in test files to fix compilation errors. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
Update sentry.api to include the new distribution() method signature to fix apiCheck failure. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
…ionIntegration Added consumer ProGuard rule in sentry-android-core to handle missing DistributionIntegration class when the distribution module is not included. This follows the same pattern used for other optional integrations like Replay and Timber.
f5e46f7
to
5dce724
Compare
🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
Summary
Adds Sentry Android Distribution module with automatic integration and
Sentry.distribution()
API.Key Changes
Sentry.distribution()
methodUsage
#skip-changelog
🤖 Generated with Claude Code