Add support for reading distribution options from properties #4784
Conversation
Performance metrics 🚀
|
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| d217708 | 411.22 ms | 430.86 ms | 19.63 ms |
| 7314dbe | 437.83 ms | 505.64 ms | 67.81 ms |
| 14ff5ee | 419.75 ms | 495.73 ms | 75.98 ms |
| b3d8889 | 371.84 ms | 447.49 ms | 75.65 ms |
| b750b96 | 421.25 ms | 444.09 ms | 22.84 ms |
| c8125f3 | 397.65 ms | 485.14 ms | 87.49 ms |
| 3998a95 | 415.94 ms | 478.54 ms | 62.60 ms |
| ee747ae | 386.94 ms | 431.43 ms | 44.49 ms |
| d217708 | 355.34 ms | 381.39 ms | 26.05 ms |
| 3699cd5 | 423.60 ms | 495.52 ms | 71.92 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| d217708 | 1.58 MiB | 2.10 MiB | 532.97 KiB |
| 7314dbe | 1.58 MiB | 2.10 MiB | 533.45 KiB |
| 14ff5ee | 1.58 MiB | 2.10 MiB | 535.08 KiB |
| b3d8889 | 1.58 MiB | 2.10 MiB | 535.07 KiB |
| b750b96 | 1.58 MiB | 2.10 MiB | 533.20 KiB |
| c8125f3 | 1.58 MiB | 2.10 MiB | 532.32 KiB |
| 3998a95 | 1.58 MiB | 2.10 MiB | 532.96 KiB |
| ee747ae | 1.58 MiB | 2.10 MiB | 530.95 KiB |
| d217708 | 1.58 MiB | 2.10 MiB | 532.97 KiB |
| 3699cd5 | 1.58 MiB | 2.10 MiB | 533.45 KiB |
runningcode
force-pushed
the
no/eme-397-set-sentryoptionsdistributionoptions-from-gradle-or
branch
from
bc2d596 to
37e6a46
Compare
…roperties Extends DebugMetaPropertiesApplier to read and apply distribution configuration from properties files. This allows the Gradle plugin to populate distribution options (orgSlug, projectSlug, orgAuthToken, buildConfiguration) that will be automatically loaded when the SDK initializes. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
runningcode
force-pushed
the
no/eme-397-set-sentryoptionsdistributionoptions-from-gradle-or
branch
from
37e6a46 to
47bad3a
Compare
runningcode
requested review from
adinauer,
romtsn,
stefanosiano,
markushi and
lcian
as code owners
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
sentry/src/main/java/io/sentry/util/DebugMetaPropertiesApplier.java
Outdated
Show resolved
Hide resolved
| distributionOptions.buildConfiguration = buildConfiguration; | ||
| } | ||
|
|
||
| break; |
There was a problem hiding this comment.
I'm confused about this break; do we not need to go though all properties?
There was a problem hiding this comment.
This is a good question. I did this this to keep it consistent with the other functions in this file which also have a break. Example
This does the following:
- We iterate through a list of properties files (there can be multiple sentry-debug-meta.properties files from different sources)
- For each file, we check if it contains any distribution-related properties
- If we find a file with distribution properties, we:
- Apply those properties (only the non-empty ones that aren't already set)
- Then break out of the loop immediately - This means we only process the first properties file that contains distribution options
I'll add a comment here that explains this behavior. I'm happy to debate this behavior though! I think it would be confusing if the properties were overridden from different files.
| fun `applies distribution options from first properties file with values`() { | ||
| val properties1 = Properties() | ||
| val properties2 = Properties() | ||
| properties2.setProperty("io.sentry.distribution.org-slug", "org-from-second") |
There was a problem hiding this comment.
You have properties2 twice here and you want properties1 for one of them I think.
There was a problem hiding this comment.
Good point. Actually this is intentional, we only want to apply properties from the first file with values (which is why we have the break). We don't want to merge files. Although I'm happy to debate this behavior!
- Include buildConfiguration in initial property check to fix bug where buildConfiguration-only properties would be skipped - Add isEmpty() checks for all property values before applying - Add comment explaining break statement (only process first properties file) - Add test for buildConfiguration-only scenario - Add test for empty string values 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
| // to maintain consistency with other properties like proguardUuid | ||
| break; | ||
| } | ||
| } |
There was a problem hiding this comment.
Bug: Loop Exits Early on Distribution Property
The applyDistributionOptions method prematurely exits its loop over sentry-debug-meta.properties files. It breaks after encountering the first file with any non-null distribution property, even if that property is an empty string or if the option is already set. This prevents valid distribution options from being applied if they are defined in subsequent properties files.
Summary
Extends
DebugMetaPropertiesApplierto read and apply distribution configuration from properties files bundled in the APK. This is part of EME-397 to allow the Sentry Android Gradle Plugin to automatically populateSentryOptions.DistributionOptionsfields.Integration
This allows the Gradle plugin (implemented here) to generate these properties at build time and bundle them into the APK. The SDK will automatically load them when initializing, making distribution configuration seamless for developers.
There are probably many ways of passing these options to the client but using properties files doesn't require the client to recompile resources or code and there is already an existing mechanism in the codebase to do this so I just piggy-backed on that mechanism.
Notes
#skip-changelog This feature is not released or published yet.
🤖 Generated with Claude Code