-
-
Notifications
You must be signed in to change notification settings - Fork 457
feat(android-distribution): add httpclient for checking for build distribution updates #4734
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
|
57a565d
to
3e93cc4
Compare
Performance metrics 🚀
|
Revision | Plain | With Sentry | Diff |
---|---|---|---|
23d6b12 | 354.10 ms | 408.38 ms | 54.28 ms |
674d437 | 355.28 ms | 504.18 ms | 148.90 ms |
7314dbe | 437.83 ms | 505.64 ms | 67.81 ms |
3d205d0 | 352.15 ms | 432.53 ms | 80.38 ms |
c8125f3 | 397.65 ms | 485.14 ms | 87.49 ms |
ee747ae | 554.98 ms | 611.50 ms | 56.52 ms |
3998a95 | 415.94 ms | 478.54 ms | 62.60 ms |
ee747ae | 415.92 ms | 470.15 ms | 54.23 ms |
ee747ae | 405.43 ms | 485.70 ms | 80.28 ms |
1df7eb6 | 397.04 ms | 429.64 ms | 32.60 ms |
App size
Revision | Plain | With Sentry | Diff |
---|---|---|---|
23d6b12 | 1.58 MiB | 2.10 MiB | 532.31 KiB |
674d437 | 1.58 MiB | 2.10 MiB | 530.94 KiB |
7314dbe | 1.58 MiB | 2.10 MiB | 533.45 KiB |
3d205d0 | 1.58 MiB | 2.10 MiB | 532.97 KiB |
c8125f3 | 1.58 MiB | 2.10 MiB | 532.32 KiB |
ee747ae | 1.58 MiB | 2.10 MiB | 530.95 KiB |
3998a95 | 1.58 MiB | 2.10 MiB | 532.96 KiB |
ee747ae | 1.58 MiB | 2.10 MiB | 530.95 KiB |
ee747ae | 1.58 MiB | 2.10 MiB | 530.95 KiB |
1df7eb6 | 1.58 MiB | 2.10 MiB | 532.97 KiB |
import javax.net.ssl.HttpsURLConnection | ||
|
||
/** HTTP client for making requests to Sentry's distribution API. */ | ||
internal class DistributionHttpClient(private val options: SentryOptions) { |
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 had claude generate an HttpClient for me. It works. I'm just not sure if it is a good idea to develop another client.
The other alternative is to expand the existing HttpConnection
to support our use case OR shadow oktthp in to our library. Open to thoughts!
import org.junit.Ignore | ||
import org.junit.Test | ||
|
||
class DistributionHttpClientTest { |
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 just a test class I used to test the integration. In a future, SAGP will inject the authToken and other things so we won't need to specify this manually. I'll also integrate distribution in to the sample app.
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.
Thanks!
...-android-distribution/src/main/java/io/sentry/android/distribution/DistributionHttpClient.kt
Outdated
Show resolved
Hide resolved
...-android-distribution/src/main/java/io/sentry/android/distribution/DistributionHttpClient.kt
Outdated
Show resolved
Hide resolved
...-android-distribution/src/main/java/io/sentry/android/distribution/DistributionHttpClient.kt
Outdated
Show resolved
Hide resolved
import org.json.JSONObject | ||
|
||
/** Parser for distribution API responses. */ | ||
internal class UpdateResponseParser(private val options: SentryOptions) { |
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 some tests for this also since it is nicely separated
UpdateStatus.UpdateError(e.message ?: "Configuration error") | ||
} catch (e: Exception) { | ||
sentryOptions.logger.log(SentryLevel.ERROR, e, "Failed to check for updates") | ||
UpdateStatus.UpdateError("Network error: ${e.message}") |
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 nice to be able to distinguish between 'not connected' and 'network error' in the return type if that is possible and easy.
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.
UnknownHostException
would mean there is possibly no network and SocketTimeoutException
could mean a bad network. I added those to distinguish here.
val appId = context.applicationInfo.packageName | ||
|
||
DistributionHttpClient.UpdateCheckParams( | ||
mainBinaryIdentifier = appId, // Using package name as binary identifier |
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.
Do we want 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.
nope, will remove
mainBinaryIdentifier = appId, // Using package name as binary identifier | ||
appId = appId, | ||
platform = "android", | ||
version = versionName, |
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 we're missing version code iirc (Android has both a human facing version and a machine facing version for apps, one is versionCode and the other versionNumber)
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 good point. I didn’t see versionCode as a parameter to the check for updates APi.
https://github.com/getsentry/sentry/blob/master/src/sentry/preprod/api/endpoints/project_preprod_check_for_updates.py#L62
Should I pass this in as versionName + versionCode or should we add an extra parameter to the API?
connection.setRequestProperty("Accept", "application/json") | ||
connection.setRequestProperty( | ||
"User-Agent", | ||
options.sentryClientName ?: "sentry-android-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.
We maybe should default to the same as here:
sentry-java/sentry-android-core/src/main/java/io/sentry/android/core/SentryAndroidOptions.java
Line 231 in d217708
setSentryClientName(BuildConfig.SENTRY_ANDROID_SDK_NAME + "/" + BuildConfig.VERSION_NAME); |
(or, since it seems like it is always set despite being nullable we could throw if it is unset)
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.
sounds good, i added a throw if unset
…ality Implements the checkForUpdateBlocking method in DistributionIntegration to check for app updates via Sentry's distribution API. ## Why not reuse existing HttpConnection? The existing `HttpConnection` class is designed specifically for Sentry event transport and is not suitable for distribution API calls: - Hardcoded for POST requests (we need GET) - Expects Sentry envelopes with gzip encoding (we need simple JSON) - Only considers status 200 successful (REST APIs use 200-299 range) - Includes Sentry-specific rate limiting logic ## Changes - **DistributionHttpClient**: New HTTP client for distribution API requests - Supports GET requests with query parameters (main_binary_identifier, app_id, platform, version) - Uses SentryOptions.DistributionOptions for configuration (orgSlug, projectSlug, orgAuthToken) - Handles SSL configuration, timeouts, and proper error handling - **UpdateResponseParser**: JSON response parser for API responses - Parses API responses into UpdateStatus objects (UpToDate, NewRelease, UpdateError) - Handles various HTTP status codes with appropriate error messages - Validates required fields in update information - **DistributionIntegration**: Updated to use new classes - Automatically extracts app information (package name, version) from Android context - Clean separation of concerns with HTTP client and response parser - Comprehensive error handling and logging - **Tests**: Added unit test for DistributionHttpClient with real API integration 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
- Remove unnecessary Claude-style comments from DistributionHttpClient - Replace manual URL building with Android Uri.Builder for safer parameter encoding - Add comprehensive tests for UpdateResponseParser with 11 test cases - Improve error handling to distinguish between network connection vs server issues - Add clarifying comments about which exceptions indicate network connectivity problems - Fix null value handling in JSON parsing to properly validate "null" strings - Remove unclear comment about package name usage 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
Replace custom fallback "sentry-android-distribution" with error throw when sentryClientName is null, following the pattern used throughout the codebase where sentryClientName is expected to always be set. Addresses PR review feedback about reusing consistent user agent. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
Change "check connection speed" to "check network connection" to be more general and align with the goal of distinguishing network connectivity issues. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
6465b7d
to
061d6b5
Compare
Summary
Implements the
checkForUpdateBlocking
method inDistributionIntegration
to check for app updates via Sentry's distribution API.Why not reuse existing HttpConnection?
The existing
HttpConnection
class is designed specifically for Sentry event transport and is not suitable for distribution API calls:Changes
DistributionHttpClient: New HTTP client for distribution API requests
UpdateResponseParser: JSON response parser for API responses
#skip-changelog
🤖 Generated with Claude Code