Conversation
Add a source-agnostic hook so a screen that is neither an Activity nor a Fragment can be reported as visible, and wire it up from the Compose Navigation instrumentation's OnDestinationChangedListener. A reported destination takes precedence over the last resumed fragment and then the last resumed activity, still falling back to "unknown". Clearing is conditional on the given name still being the recorded one, matching the compareAndSet pattern already used by activityPaused and fragmentPaused, so a nested or sibling controller leaving the composition cannot discard a destination another controller has since recorded. The public API delta is two methods on VisibleScreenTracker: navigationDestinationChanged(String) and navigationDestinationCleared(String). Everything else added here is internal. No attribute or semantic convention changes are included. Which attribute is stamped, and the longer-term staleness model, remain open questions on open-telemetry#1909. See open-telemetry#1909 Signed-off-by: Hitesh Kalluru <kalluruhitesh3@gmail.com>
There was a problem hiding this comment.
Pull request overview
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Enables VisibleScreenTracker to treat Compose Navigation destinations as the “currently visible screen”, and wires Compose Navigation instrumentation to report/clear destination names via a small reporter helper.
Changes:
- Added destination-first precedence to
VisibleScreenTrackerand new methods to set/clear a navigation destination name. - Wired Compose Navigation
NavController.OnDestinationChangedListenerto report the resolved route pattern toVisibleScreenTracker, and clear it on disposal. - Added/updated unit + Robolectric/Compose tests and updated the API dump and changelog.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| services/src/main/java/io/opentelemetry/android/internal/services/visiblescreen/VisibleScreenTrackerImpl.kt | Adds an AtomicReference slot for current navigation destination and destination-first precedence. |
| services/src/main/java/io/opentelemetry/android/internal/services/visiblescreen/VisibleScreenTracker.kt | Adds new interface methods + KDoc describing destination precedence/clearing semantics. |
| services/api/services.api | Updates API surface to include the two new methods. |
| services/src/test/java/io/opentelemetry/android/internal/services/visiblescreen/VisibleScreenTrackerTest.kt | Adds test cases for destination precedence and conditional clearing behavior. |
| instrumentation/compose/navigation/src/main/kotlin/io/opentelemetry/instrumentation/compose/navigation/NavControllerExtensions.kt | Fetches VisibleScreenTracker, reports destination name on changes, clears on dispose, and updates attach seam. |
| instrumentation/compose/navigation/src/main/kotlin/io/opentelemetry/instrumentation/compose/navigation/NavigationDestinationReporter.kt | Introduces helper to remember last reported destination per controller and clear it safely. |
| instrumentation/compose/navigation/src/test/kotlin/io/opentelemetry/instrumentation/compose/navigation/NavigationVisibleScreenComposeTest.kt | Adds Compose/Robolectric tests verifying report and disposal-clear behavior. |
| instrumentation/compose/navigation/src/test/kotlin/io/opentelemetry/instrumentation/compose/navigation/NavigationDestinationReporterTest.kt | Adds unit tests for reporter behavior (clear semantics and idempotency). |
| instrumentation/compose/navigation/src/test/kotlin/io/opentelemetry/instrumentation/compose/navigation/NavControllerExtensionsTest.kt | Updates tests for the new attachOpenTelemetry(..., reporter) parameter and verifies reporting. |
| instrumentation/compose/navigation/build.gradle.kts | Adds dependency on :services to access VisibleScreenTracker. |
| CHANGELOG.md | Documents the new destination-aware visible screen behavior. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| } | ||
|
|
||
| override fun navigationDestinationCleared(destinationName: String) { | ||
| currentNavigationDestination.compareAndSet(destinationName, null) |
| @Before | ||
| fun setup() { | ||
| val otel = OpenTelemetryRule.create() | ||
| rum = | ||
| mockk<OpenTelemetryRum> { | ||
| every { openTelemetry } returns otel.openTelemetry | ||
| } | ||
| visibleScreenTracker = mockk(relaxed = true) | ||
| Services.set( | ||
| mockk<Services> { | ||
| every { this@mockk.visibleScreenTracker } returns this@NavigationVisibleScreenComposeTest.visibleScreenTracker | ||
| }, | ||
| ) |
| import androidx.test.ext.junit.runners.AndroidJUnit4 | ||
| import io.mockk.mockk | ||
| import io.mockk.verify | ||
| import io.opentelemetry.android.internal.services.visiblescreen.VisibleScreenTracker | ||
| import org.junit.Test | ||
| import org.junit.runner.RunWith | ||
|
|
||
| @RunWith(AndroidJUnit4::class) |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2011 +/- ##
==========================================
- Coverage 65.67% 64.77% -0.90%
==========================================
Files 172 174 +2
Lines 3918 3960 +42
Branches 442 474 +32
==========================================
- Hits 2573 2565 -8
- Misses 1213 1233 +20
- Partials 132 162 +30 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
|
||
| override val currentlyVisibleScreen: String | ||
| get() { | ||
| val destination = currentNavigationDestination.get() |
There was a problem hiding this comment.
What clears this destination when its host Activity pauses and another Activity comes to the foreground? The first Activity composition can remain alive, so onDispose may not run; because the destination always wins here, spans and logs from Activity B can still get Activity A route. Could we scope it to the host Activity lifecycle and add an A route -> B Activity test?
There was a problem hiding this comment.
Confirmed, and thanks — this is a real hole. When Activity A is stopped rather than
destroyed its composition stays alive, so onDispose never runs and the slot keeps A's
route. activityResumed(B) updates lastResumedActivity, but the destination outranks it
in currentlyVisibleScreen, so every span and log on Activity B is attributed to A's
route until the user navigates back.
I'll scope it to the host Activity. navigationDestinationChanged can capture the
resumed Activity at report time, and currentlyVisibleScreen returns the destination
only while that host is still the resumed Activity. That has a nice side effect: the
route is restored when the user returns to A, without the composition needing to
re-report.
One pre-existing limitation worth naming rather than hiding: the tracker identifies
Activities by javaClass.simpleName, so two instances of the same Activity class are
indistinguishable. I don't propose to change that here.
Adding the A route -> B Activity test.
| * The clear only applies if [destinationName] is still the recorded destination. A source that | ||
| * has since been superseded by another one therefore cannot discard the newer destination. | ||
| */ | ||
| fun navigationDestinationCleared(destinationName: String) |
There was a problem hiding this comment.
Could this clear identify the reporting controller rather than only its route text? Two controllers can both report home; if the newer one is active and the older one disposes, clear("home") can remove the newer controller value. An opaque source token or registration handle would preserve the conditional-clear guarantee.
There was a problem hiding this comment.
Good catch — name equality isn't identity, so this defeats the point of the
conditional clear. Two controllers reporting "home" means the older one's
clear("home") satisfies the compareAndSet and removes the newer controller's value.
Switching to an opaque owner token. The compose module already creates one
NavigationDestinationReporter per attached controller, so it can pass itself:
navigationDestinationChanged(owner, name) records the pair, and
navigationDestinationCleared(owner) clears only when the recorded owner is
identical. Route text stops participating in the clear decision entirely.
This composes with your other comment — the slot ends up holding (owner, host
activity, destination name), where the owner governs clears and the host governs
reads. Adding a test for two controllers reporting the same route.
|
@breedx-splk please review |
|
@hitkall there are quite a few open comments from Copilot and another project member - if you can address these first then one of the project maintainers/approvers will take a look. |
fractalwrench
added
the
needs author feedback
|
This has been automatically marked as stale because it has been marked as needing author feedback and has not had any activity for 21 days. It will be closed automatically if there is no response from the author within 14 additional days from this comment. |
What this does
Gives
VisibleScreenTrackera source-agnostic way to be told about a screen that is neither anActivity nor a Fragment, and wires it up from the Compose Navigation instrumentation's
NavController.OnDestinationChangedListener.AtomicReferenceslot onVisibleScreenTrackerImplfor a navigation destination name.currentlyVisibleScreenbecomes: destination → fragment → activity →"unknown".:instrumentation:compose:navigationgainsimplementation(project(":services")). This edge isprecedented:
:instrumentation:compose:clickalready depends on:services.NavigationDestinationReporter, and theDisposableEffect'sonDisposeclears it.Clearing is conditional:
navigationDestinationCleared(name)only clears whennameis stillthe recorded destination, via
compareAndSet, matching the pattern already used byactivityPausedand
fragmentPaused. Each attached controller owns its own reporter, so a nested or siblingcontroller leaving the composition cannot discard a destination another controller has since
recorded. Without this, teardown ordering between nested controllers could blank a destination the
parent had just recorded and silently drop attribution back to the Activity name.
What this deliberately does not do
ScreenAttributesSpanProcessororScreenAttributesLogRecordProcessor.app.navigation.completeevent and itsapp.navigation.destination.nameattribute behave exactly as before.previouslyVisibleScreenis untouched, solast.screen.namekeeps its Activity/Fragment-onlymeaning. There is a test pinning this.
One consequence worth stating plainly, because it is the point of the change rather than a side
effect: no attribute key changes, but for apps that attach this instrumentation the value of
app.screen.namewill now be the Compose destination name rather than the host Activity name. Theinstrumentation is manual and opt-in — it is not auto-discovered — so only apps that call
withOpenTelemetryorrememberObservedNavControllerare affected.Public API delta — Tier 4
Two methods on
VisibleScreenTracker, and nothing else:navigation.apiis unchanged —attachOpenTelemetryandNavigationDestinationReporterareinternal. Per CONTRIBUTING this is Tier 4: 2 approvals and a 2-day waiting period.Known consequence: DialogFragment
Under destination-first precedence, a
DialogFragmentshown over a Compose screen is no longerreflected in
currentlyVisibleScreen— the recorded destination outranks it. This follows directlyfrom the precedence order rather than from a defect in the implementation, and it is the tradeoff
against the alternative ordering, where a
NavHostnested inside a Fragment would lose to its hostfragment instead.
This is documented in the class KDoc, and it relates to the staleness question still open on #1909.
Happy to reorder if maintainers prefer the other tradeoff.
Nested-controller restore semantics
The destination slot overwrites rather than stacks. If a parent controller records
"a"and anested controller then records
"b","a"is already gone; disposing the child clears to thefallback chain rather than restoring
"a".I've left nested-controller restore semantics out of this PR since it needs owner/name pairs rather
than a single slot — a single displaced-value slot is not enough, because it would resurrect a stale
route from the same controller after an ordinary
a → b → cnavigation. That felt related to thestaleness question still open on the issue, but happy to fold it in if you'd rather.
Tests
:services(JUnit 5 + MockK) — all 7 pre-existingVisibleScreenTrackerTestcases pass unchanged;7 added:
"unknown"previouslyVisibleScreen:instrumentation:compose:navigation(JUnit 4 + Robolectric) — 7 added across two new files:user/{id}and neveruser/2, pinning the PII-safe defaultidempotent on double-clear
NavControllerExtensionsTestwas updated for the newattachOpenTelemetryparameter; that internalfunction exists as a test seam, so its signature is pinned by that file.
Verification
./gradlew spotlessApplyand./gradlew apiDumpboth clean, with the updated.apifile includedin the diff.
./gradlew :services:check :instrumentation:compose:navigation:check :core:check— BUILD SUCCESSFUL.services 89 tests, compose-navigation 23 tests, core 118 tests; 0 failures. The single skipped core
test is a pre-existing
@Ignore, andcoreis untouched by this PR.See #1909