-
-
Notifications
You must be signed in to change notification settings - Fork 461
refactor(distribution): Use interface pattern for distribution API #4731
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
Conversation
Performance metrics 🚀
|
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| c8125f3 | 397.65 ms | 485.14 ms | 87.49 ms |
| 3699cd5 | 423.60 ms | 495.52 ms | 71.92 ms |
| 23d6b12 | 354.10 ms | 408.38 ms | 54.28 ms |
| 674d437 | 355.28 ms | 504.18 ms | 148.90 ms |
| ee747ae | 382.73 ms | 435.41 ms | 52.68 ms |
| 17a0955 | 372.53 ms | 446.70 ms | 74.17 ms |
| ce0a49e | 532.00 ms | 609.96 ms | 77.96 ms |
| ee747ae | 374.71 ms | 455.18 ms | 80.47 ms |
| b750b96 | 408.98 ms | 480.32 ms | 71.34 ms |
| ee747ae | 405.43 ms | 485.70 ms | 80.28 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| c8125f3 | 1.58 MiB | 2.10 MiB | 532.32 KiB |
| 3699cd5 | 1.58 MiB | 2.10 MiB | 533.45 KiB |
| 23d6b12 | 1.58 MiB | 2.10 MiB | 532.31 KiB |
| 674d437 | 1.58 MiB | 2.10 MiB | 530.94 KiB |
| ee747ae | 1.58 MiB | 2.10 MiB | 530.95 KiB |
| 17a0955 | 1.58 MiB | 2.10 MiB | 533.20 KiB |
| ce0a49e | 1.58 MiB | 2.10 MiB | 532.94 KiB |
| ee747ae | 1.58 MiB | 2.10 MiB | 530.95 KiB |
| b750b96 | 1.58 MiB | 2.10 MiB | 533.19 KiB |
| ee747ae | 1.58 MiB | 2.10 MiB | 530.95 KiB |
Previous results on branch: no/refactor-distribution-api-interface
Startup times
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 6e95f6f | 414.78 ms | 434.59 ms | 19.82 ms |
| 1bc1dd2 | 368.46 ms | 429.98 ms | 61.52 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 6e95f6f | 1.58 MiB | 2.10 MiB | 532.96 KiB |
| 1bc1dd2 | 1.58 MiB | 2.10 MiB | 532.96 KiB |
| import org.jetbrains.annotations.NotNull; | ||
|
|
||
| /** Represents the result of checking for app updates. */ | ||
| public abstract class 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.
wondering if this can be used as a sealed class in Kotlin (i.e. doing a when over this)? I guess not, but this API is fine for me - just brainstorming if we should change it an enum to support when-statements (but then it wouldn't be easily typed)
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.
Yes, it was originally a sealed class but now that it is in the Sentry module I changed it to java. are we allowed to use Kotlin in the Sentry module??
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, let's try to init automatically in register as discussed internally
Address PR feedback from #4712: - Refactor Sentry.distribution() to follow existing replay() pattern - Create IDistributionApi interface with core methods (checkForUpdateBlocking, checkForUpdate, downloadUpdate) - Add distributionController to SentryOptions with NoOp default - Use getCurrentScopes().getScope().getOptions().getDistributionController() access pattern - Remove reflection-based implementation for type safety - Provide NoOpDistributionApi fallback when module not available This follows the established architecture pattern used by replay API, allowing distribution integrations to register real implementations via options.setDistributionController() during integration registration. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
- Update AndroidOptionsInitializer to use new DistributionIntegration constructor - Refactor DistributionIntegration to implement IDistributionApi methods directly - Remove internal package structure and DistributionInternal dependency - Update class names from Distribution to DistributionIntegration for clarity - Convert data classes to regular classes to match API requirements - Rename organizationSlug to orgSlug for consistency - Implement downloadUpdate using Android Intent system - Remove completed Distribution singleton approach This completes the distribution module implementation to work with the new interface-based API pattern from the previous commit. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
…odule - Move UpdateStatus and UpdateInfo from distribution module to core sentry module - Update IDistributionApi to use proper types instead of Object - Add UpdateCallback interface for type-safe async callbacks - Rename UpdateStatus.Error to UpdateError to avoid java.lang.Error clash - Update DistributionIntegration to implement IDistributionApi with proper types - Remove duplicate classes from distribution module - Regenerate API files with proper type signatures This provides full type safety for the distribution API while keeping the types accessible to all modules that might need them. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
…ion class path - Update ProGuard rules from `internal.DistributionIntegration` to `DistributionIntegration` - Fixes R8 missing class error in release builds - Ensures DistributionIntegration class is properly kept during code shrinking This addresses the build failure in Android integration tests where R8 was removing the DistributionIntegration class that is referenced by AndroidOptionsInitializer. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
- Update distribution module API file after refactoring to use interface pattern - Class renamed from Distribution to DistributionIntegration - Now implements IDistributionApi interface with proper method signatures - Removed UpdateInfo and UpdateStatus classes (moved to core sentry module) - Constructor now takes Context parameter - Method signatures use proper types instead of Object parameters 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
…resolve circular dependency - Move DistributionOptions from SentryAndroidOptions to SentryOptions to resolve circular dependency between sentry-android-core and sentry-android-distribution modules - Simplify DistributionIntegration.register() to work with SentryOptions directly instead of requiring SentryAndroidOptions - Remove separate DistributionOptions.kt file - Update API dumps 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
…ce in SentryAndroid
b169244 to
55f4f2b
Compare
Addresses architectural feedback from PR #4712 by refactoring the distribution API to follow established Sentry patterns.
Changes
IDistributionApiwithcheckForUpdateBlocking(),checkForUpdate(),downloadUpdate()methodsUpdateStatus,UpdateInfo, andUpdateCallbacktypes to core sentry module because they are part of the APIdistributionControllertoSentryOptionsfollowing replay API patternSentry.distribution()to usegetCurrentScopes().getScope().getOptions().getDistributionController()instead of reflectionDistributionIntegrationto implementIDistributionApidirectlyreplay()API#skip-changelog
Distribution integrations register real implementations during setup, with graceful NoOp fallback when the module is unavailable.