feat(extend-app-start): [2/4] Extract AppStartExtension component#5606
feat(extend-app-start): [2/4] Extract AppStartExtension component#5606buenaflor wants to merge 23 commits into
Conversation
|
📲 Install BuildsAndroid
|
064d3f1 to
7a7d63f
Compare
fd450bb to
54be119
Compare
Performance metrics 🚀
|
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 3d04aae | 324.42 ms | 359.30 ms | 34.88 ms |
| 69d43cc | 380.70 ms | 424.90 ms | 44.20 ms |
| 2821a4d | 315.46 ms | 366.56 ms | 51.10 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 3d04aae | 0 B | 0 B | 0 B |
| 69d43cc | 0 B | 0 B | 0 B |
| 2821a4d | 0 B | 0 B | 0 B |
Previous results on branch: feat/app-start-extension-android
Startup times
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| ba0608b | 319.63 ms | 373.04 ms | 53.41 ms |
| 682e090 | 347.33 ms | 427.28 ms | 79.95 ms |
| db5be2f | 311.84 ms | 365.94 ms | 54.10 ms |
| 388ffc5 | 315.18 ms | 362.43 ms | 47.25 ms |
| ca0c0cfc14d958d067e640e132d8de863dfe8eb1 | 318.45 ms | 370.06 ms | 51.61 ms |
| b76203f | 336.02 ms | 403.42 ms | 67.40 ms |
| 201a6fd | 326.00 ms | 371.92 ms | 45.92 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| ba0608b | 0 B | 0 B | 0 B |
| 682e090 | 0 B | 0 B | 0 B |
| db5be2f | 0 B | 0 B | 0 B |
| 388ffc5 | 0 B | 0 B | 0 B |
| ca0c0cfc14d958d067e640e132d8de863dfe8eb1 | 0 B | 0 B | 0 B |
| b76203f | 0 B | 0 B | 0 B |
| 201a6fd | 0 B | 0 B | 0 B |
54be119 to
d2b96ad
Compare
3bc1643 to
45f6c0b
Compare
…ndroid extender Replaces the AppStartMetrics IAppStartExtender implementation and the deferred ExtendedAppStartSpan with a focused, lock-guarded AppStartExtension that owns the eager App Start transaction and extended span. AppStartMetrics now only holds the component and exposes isAppStartWindowOpen(). Inert until 3/4 registers the listener. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ead of static scope lookup Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… instead of a callback The listener now returns the created transaction+span (or null to decline) rather than calling back into onExtended() while extendAppStart() holds the lock. This removes the re-entrant lock acquisition and the A->B->A round trip, collapsing two public methods into one linear flow. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…o clear Drop comments that restate code or annotate omissions (region markers, getter javadoc, the reset call-site comment, the ExtendedAppStart/isActive docs). Rename AppStartExtension.reset() to clear() to match its owner AppStartMetrics.clear(). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ting it The extension logs only two warnings, both on rare guard paths in extendAppStart(). Inline Sentry.getCurrentScopes().getOptions().getLogger() at those sites instead of holding a logger field set at init, dropping the field, setLogger(), and the AndroidOptionsInitializer wiring. extendAppStart() runs post-init, so the lookup always yields the configured logger. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…tartExtension Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…mments Drop finishTransaction's javadoc (trivial body; it described caller context and waitForChildren behavior configured elsewhere) and reduce getExtendedEndTime's javadoc to a single inline note on the only non-obvious branch (deadline suppression). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…o finishExtendedAppStart Implements the renamed IAppStartExtender.finishExtendedAppStart(). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
2d5188e to
01b1dc4
Compare
…on extendAppStart Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…nd trim comments Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
| } | ||
| } | ||
|
|
||
| public void finishTransaction(final @NotNull SentryDate endTimestamp) { |
There was a problem hiding this comment.
this will be used in the next PR 3/4
There was a problem hiding this comment.
Pull request overview
This PR continues the “Extend App Start” stack by extracting the Android app-start extension behavior out of AppStartMetrics into a dedicated, testable AppStartExtension component, and wiring it into Android options so the core IAppStartExtender bridge can route to Android.
Changes:
- Introduces
AppStartExtension(@ApiStatus.Internal) implementingIAppStartExtender, encapsulating the extension lifecycle and extended txn/span references. - Updates
AppStartMetricsto own the component, expose anisAppStartWindowOpen()gate, and clear the extension state when spans are sent / metrics reset. - Registers the extender in
AndroidOptionsInitializerand adds/extends unit coverage for the new behavior and gates.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| sentry-android-core/src/main/java/io/sentry/android/core/AppStartExtension.java | New internal extender component with lock-guarded lifecycle and span/txn accessors. |
| sentry-android-core/src/main/java/io/sentry/android/core/performance/AppStartMetrics.java | Holds the extension component, adds isAppStartWindowOpen(), clears extension state on reset/send. |
| sentry-android-core/src/main/java/io/sentry/android/core/AndroidOptionsInitializer.java | Wires the Android extender implementation into SentryAndroidOptions. |
| sentry-android-core/src/test/java/io/sentry/android/core/AppStartExtensionTest.kt | New unit tests for extension activation/inert behavior, finish semantics, and end-time suppression. |
| sentry-android-core/src/test/java/io/sentry/android/core/performance/AppStartMetricsTest.kt | Adds coverage for the new “window open” gate and extension state reset paths. |
| sentry-android-core/api/sentry-android-core.api | API dump updates for newly added internal class and new AppStartMetrics methods. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…nished flag getExtendedEndTime() gated on span.isFinished(), but finishing the extended span completes the waitForChildren transaction and runs the event processor re-entrantly within finishExtendedAppStart(), before the span's finished flag is set. The processor then saw an unfinished span and dropped the app start measurement whenever the extension finished after the first frame. Read getFinishDate() (set before the finish callback) instead, which also keeps the extended end controllable in tests. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
a622ea2 to
d2c16a2
Compare
…pan end When the extended span finished after the timestamp passed to finishTransaction() but before that call ran (e.g. a synchronous extension in a headless start, where finishTransaction runs later at main-thread idle), waitForChildren had nothing left to wait for and the transaction kept the earlier passed timestamp. The extended span then ended after the transaction, and the app start vital exceeded the transaction duration. Finish at max(endTimestamp, extended span finish date) so the span is contained and the duration matches the vital. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The setter wrote the field without synchronization while extendAppStart() reads it under the lock, leaving no happens-before edge. Acquire the same lock in the setter, consistent with the rest of the class's mutable state. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit e26ee97. Configure here.
Finishing the extended span runs the event processor re-entrantly (via the waitForChildren transaction) before the span's isFinished() flag is set, while the finish timestamp is already in place. Reading isFinished() could therefore hand out a span that is already finishing. Switch to getFinishDate() == null, matching getExtendedEndTime(). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…endedAppStartSpan Replace the repeated reentrancy explanation with a cross-reference to getExtendedEndTime(), which holds the canonical version. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…length Single-line form fits the 100-char limit so spotless leaves it unwrapped. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…mment Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ppStart The predicate has a single consumer — the extend gate in AppStartExtension — so name it for that intent. Also drop the self-evident max-end comment in finishTransaction. Regenerated apiDump. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ed test After getExtendedAppStartSpan switched to getFinishDate(), the "after the span finished" case exercises the same branch as the reentrancy test (finishDate set -> NoOp); the isFinished stub was inert. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Move these component accessors into the component-extraction PR (they were introduced later in the wiring PR). Keeps the full AppStartExtension surface and its unit tests together here; the wiring PR only consumes them. Inert on its own. Regenerated apiDump. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
It is written without a lock in onAppStartSpansSent() and read across threads in canExtendAppStart() (via extendAppStart()), with no happens-before edge. volatile guarantees visibility, matching the AtomicInteger/AtomicBoolean siblings in the same check. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

PR Stack (Extend App Start)
📜 Description
Pulls the Android extender out of the already-large
AppStartMetricsinto a focused, testableAppStartExtensioncomponent.AppStartExtension(new)implements IAppStartExtender. Owns the lock-guarded extend lifecycle and holds the eager App StartITransaction+ its childISpan.AppStartMetricscanExtendAppStart().AndroidOptionsInitializerNotes
AppStartExtensionhas no scopes, so it can't create the transaction/span itself — the integration builds them and returns anExtendedAppStartfrom the listener.getExtendedEndTime()is deadline-aware:nullonDEADLINE_EXCEEDED, so the vital is suppressed rather than inflated.canExtendAppStart()gate = measurements not sent + no activity + first frame not drawn. The foreground check is ignored so headless starts can extend as well.[3/4], soextendAppStart()is a no-op andgetExtendedAppStartSpan()returnsNoOpSpan.💡 Motivation and Context
Part of the app start extension API stack (#5553). A dedicated component keeps the new concern out of
AppStartMetricsand is easy to isolate and test.💚 How did you test it?
Unit tests in
AppStartExtensionTest,AppStartMetricsTest, andAndroidOptionsInitializerTestcover the extend gate, listener firing, inert behavior, idempotent finish, deadline suppression, and wiring.apiCheck, spotless, and the:sentry-android-coreunit suite pass.📝 Checklist
sendDefaultPIIis enabled.🔮 Next steps
[3/4]registers the listener, eagerly creates the App Start transaction + child, and extends/suppresses the app start vital inPerformanceAndroidEventProcessor.#skip-changelog