Add comopnent to create an activity open trace based on a sequence of events

[bidetofevil]

Goal

This is a component that takes a set of input events and creates the appropriate activity open trace. It's not hooked up to anything so no traces are going to be logged when this is merged, but it's sizeable enough that I want it reviewed separately, as its logic can be validated via tests.

Testing

Unit tests verifying this components works properly if given the right input in the right order.

[bidetofevil]

• Create OS-specific implementations of the ActivityLoadEventEmitter #1683
• Rename the interfaces, methods, and objects to be somewhat Activity agnostic #1682
• Hook up Activity Open tracing component #1348
• Remove startup trace recording callback once it's logged startup #1347
• Create callback mechanism to run code post app startup #1346
• Create annotation to opt into activity open tracing #1345
• Component to map lifecycle events to the activity open events #1320
• Add comopnent to create an activity open trace based on a sequence of events #1319 👈 (View in Graphite)
• main

This stack of pull requests is managed by Graphite. Learn more about stacking.

[codecov]

Codecov Report

Attention: Patch coverage is 90.29851% with 13 lines in your changes missing coverage. Please review.

Project coverage is 85.25%. Comparing base (adb5a46) to head (475b2a2).
Report is 5 commits behind head on main.

Files with missing lines	Patch %	Lines
...dk/internal/capture/activity/UiLoadTraceEmitter.kt	89.92%	5 Missing and 8 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1319      +/-   ##
==========================================
+ Coverage   85.21%   85.25%   +0.04%     
==========================================
  Files         459      461       +2     
  Lines       10474    10606     +132     
  Branches     1572     1586      +14     
==========================================
+ Hits         8925     9042     +117     
- Misses        843      848       +5     
- Partials      706      716      +10     

Files with missing lines	Coverage Δ
...android/embracesdk/internal/arch/schema/EmbType.kt	88.88% <100.00%> (ø)
...id/embracesdk/internal/spans/EmbraceSpanBuilder.kt	94.44% <100.00%> (ø)
...embracesdk/internal/capture/activity/UiLoadType.kt	100.00% <100.00%> (ø)
...dk/internal/capture/activity/UiLoadTraceEmitter.kt	89.92% <89.92%> (ø)

... and 2 files with indirect coverage changes

[fractalwrench]

Is this meant to be exposed to end-users? I was under the impression we'd just hook into lifecycle callbacks & none of this would be publicly visible

[bidetofevil]

I should add doc to more clearly articulate the design - I was going to do that before submitting this for review, but (I thought) the PR messiness with the trunk change had forced my hand.

So, the idea is that the set of events to create a trace is a super set of the lifecycle events and spans multiple invocations of activity opening. And how they are fired for the lifecycle events also depends on the Android version. This set of events effectively normalizes all those difference and combines both the lifecycle and non-lifecycle events (i.e. the render stuff).

The listener for these events will be responsible for starting and stoping spans, so they only have to be aware of these, instead of all the messiness of what this intends to encapsulate and normalize. This design will make each component more easily testable, and tease out the logic and complexity.

[bidetofevil]

But to answer your question, I didn't intend for them to be public yet, just usable in our own modules. Is there a more appropriate place to put these?

[fractalwrench]

embrace-android-features or embrace-android-core will hide it from prying eyes

[fractalwrench]

I initially assumed this meant only 1 activity could be traced at a time with this interface

[bidetofevil]

Yeah, only one activity with a particular type. This could be expanded later to support the creation of multiple instances, but there's a assumption that if a new instance for the same activity pops up, the previous has been abandoned.

Is there a case where this assumption would be incorrect?

[fractalwrench]

An activity can be passed an intent and alter its behavior depending on the bundle inside - I don't think it's safe to assume 1 activity type == only 1 instance at a time. I've certainly written apps in the past that host different fragments etc based on intent args, so there may be apps in the wild that made similar decisions

[bidetofevil]

Got it. So the case you're concerned about is FooActivity opening another instance of FooActivity?

I /believe/ this case is handled correctly if the first instance completes its lifecycle before starting another instance of the same activity. I will write a test to verify this.

The case I was talking about is regarding a new Activity instance beginning to initialize before the previous one is paused, in which case the unfinished instance, regardless of type, will be assumed to be abandoned.

For the startup trace, I had initially made the incorrect assumption that if the type is the same that it must be the same instance, which is a subset of the case you're talking about and cannot be assumed. This is why I'm using the hash code rather than the type, to prevent the case where a new implementation of FooActivity is started before the previous instance has completed

[fractalwrench]

I'm unclear on the circumstances under which these 2 would be called. The other functions make sense to me as they're analogous to lifecycle callbacks

[bidetofevil]

This is when one activity's onStop() is called without another's onStart() is called. It is meant to denote the app going into the background so another activity has not been started.

We keep track of this in order to properly gauge the start time of the next activity, as I wanted to account for the time between onPause and onCreate between two activities and have it as part of the open time for the latter if we are doing a foreground transition. Obviously we don't want to track this if the app was previously backgrounded.

[fractalwrench]

Got it. So this is equivalent to ProcessStateService#onBackground

[bidetofevil]

Yeah, except I think this will fire for an activity even if it opened another activity, whereas I think the LifecycleEventObserver only fires onStop if it doesn't detect that another activity is being opened?

Hmm. Makes me think whether I should hook into this instead of the activity itself to record these traces....

[fractalwrench]

Are there any assumptions made here that might be invalidated by multi-window mode or picture-in-picture mode?

[bidetofevil]

Yeah. The assumption that one activity's pause is when the next activity starts to open will be broken. Without this assumption, we wouldn't really know the when activity's opening is triggered, unless we we can somehow get that info from the intent?

I would like to not have to make this assumption, but if we do, we should call it out. I don't know how multi-screen windowing works with respect to the activity lifecycle callbacks - I feel like a lot of things we have that uses that API will be broken if this assumption is incorrect.

So we should really look into that 😅

[fractalwrench]

We could probably just document that it's not explicitly supported I guess? It's not a common use-case so we might be building something no/few folks want.

[bidetofevil]

Yeah, if it's even possible. I'll confirm or call out.

[bidetofevil]

This is possible and we should take care of it. It will involve redoing how the listener attachment works and how we account for the "real" start time of the trace, so I'll do it after this initial implementation is merged

[bidetofevil]

Added an explanation of the overall design and posted it on Slack.

[fractalwrench]

LGTM once a couple of ignored test cases are resolved

[fractalwrench]

Test cases with the ignore annotation should be updated to run before merging, or can be removed if they're no longer relevant

[bidetofevil]

Ah, I think they work - so I'll either un-ignore or fix them

[bidetofevil]

Merge activity

• Nov 29, 1:21 PM EST: A user merged this pull request with Graphite.