Skip to content
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

Component to map lifecycle events to the activity open events #1320

Merged
merged 3 commits into from
Nov 29, 2024

Conversation

bidetofevil
Copy link
Collaborator

@bidetofevil bidetofevil commented Sep 3, 2024

Goal

Listen to activity lifecycle callback events and map them to UiLoadEvents. With this, activities can go through its lifecycle and the relevant data will be tracked by UiLoadTraceEmitter so it can log traces when it receives a terminal event.

Note: the activity render event is NOT part of the Activity lifecycle, so the mapping isn't complete

Testing

Unit tests added to map the lifecycle events to the UI load events

Copy link

codecov bot commented Sep 9, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 85.33%. Comparing base (6bc8e94) to head (b27c99c).
Report is 4 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1320      +/-   ##
==========================================
+ Coverage   85.26%   85.33%   +0.07%     
==========================================
  Files         461      462       +1     
  Lines       10606    10658      +52     
  Branches     1586     1590       +4     
==========================================
+ Hits         9043     9095      +52     
  Misses        848      848              
  Partials      715      715              
Files with missing lines Coverage Δ
...dk/internal/capture/activity/UiLoadEventEmitter.kt 100.00% <100.00%> (ø)

... and 2 files with indirect coverage changes

@bidetofevil bidetofevil force-pushed the hho/open-trace-emitter branch from c426aab to d3bf543 Compare September 11, 2024 05:02
@bidetofevil bidetofevil force-pushed the hho/open-event-emitter branch from 0e1d93e to 044d183 Compare September 11, 2024 05:02
@bidetofevil bidetofevil force-pushed the hho/open-trace-emitter branch from d3bf543 to 32a8aa5 Compare September 11, 2024 16:56
@bidetofevil bidetofevil force-pushed the hho/open-event-emitter branch from 044d183 to c6a07c0 Compare September 11, 2024 16:57
@bidetofevil bidetofevil force-pushed the hho/open-trace-emitter branch from 32a8aa5 to 2e485ed Compare September 12, 2024 06:59
@bidetofevil bidetofevil force-pushed the hho/open-event-emitter branch from c6a07c0 to 8294561 Compare September 12, 2024 06:59
@bidetofevil bidetofevil force-pushed the hho/open-trace-emitter branch from 2e485ed to 7bde79a Compare September 12, 2024 15:39
@bidetofevil bidetofevil force-pushed the hho/open-event-emitter branch from 8294561 to 2afc05f Compare September 12, 2024 15:39
@bidetofevil bidetofevil force-pushed the hho/open-trace-emitter branch from 7bde79a to 89c30d0 Compare September 13, 2024 06:08
@bidetofevil bidetofevil force-pushed the hho/open-event-emitter branch from 2afc05f to ab9cfd9 Compare September 13, 2024 06:08
@bidetofevil bidetofevil force-pushed the hho/open-trace-emitter branch 2 times, most recently from fad7ab9 to af6f85a Compare September 13, 2024 07:13
@bidetofevil bidetofevil force-pushed the hho/open-event-emitter branch from ab9cfd9 to 3180666 Compare September 13, 2024 07:13
@bidetofevil bidetofevil force-pushed the hho/open-trace-emitter branch from af6f85a to e41a207 Compare September 13, 2024 07:13
Comment on lines +31 to +39
override fun onActivityCreated(activity: Activity, bundle: Bundle?) {
if (!versionChecker.firePrePostEvents()) {
create(activity)
}
}

override fun onActivityPostCreated(activity: Activity, savedInstanceState: Bundle?) {
createEnd(activity)
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is there a version check for activityCreated but not activityPostCreated? (This function and others are only available in API 29)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So this is the reason why I did the changes further down the stack to separate the implementation between OS versions - this won't be called in pre-29 lifecycles, and having it called implies it's 29+.

Yeah. Not great. Which is why this was refactored out in #1683 lol

Comment on lines +47 to +48
createEnd(activity)
start(activity)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I find the naming of these functions tricky to follow. I think readability would be improved by naming them onActivityStarted or something similar. Alternatively there might be scope for representing the potential states with an enum/sealed class hierarchy and routing events through one function?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So these get moved around in this PR

While the names haven't change, not mixing up two implementations in one make them a little more clear.

And this was a half-hearted effort to decouple the UILoad events from the activity lifecycle events - these stages SHOULD be generic so we can support different view loading lifecycles (e.g. compose), so I've made it possible to genericize it without actually doing all the work right now (because it's not needed).

For stacking an merging reasons, I'd like to merge this as is, and we can look at #1683 to see if there's a better way of naming these things, or maybe even doing the genericization there?

}

override fun onActivityPrePaused(activity: Activity) {
abandonTrace(activity)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is the trace abandoned?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is in case the activity didn't go through its entirely lifecycle before being backgrounded. If the trace was recorded, it's a no-op - if it hasn't, it will be marked as failed.

timestampMs = startTimeMs + PRE_DURATION
),
createEvent(
stage = "createEnd",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are these values sent to the backend? I have similar feedback to the naming in UiLoadEventEmitter that this feels tricky to understand & I wouldn't like to communicate those names when debugging something on a call with a customer

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope. These are effective points where we create or end child spans. We can look at improving the naming in #1683 for these generic stages, even if the child spans they create map (right now) only to activity lifecycle stages.

@bidetofevil bidetofevil force-pushed the hho/open-trace-emitter branch from 1c45d99 to 356af39 Compare November 28, 2024 17:22
@bidetofevil bidetofevil force-pushed the hho/open-event-emitter branch from 52b31dc to 54431ea Compare November 28, 2024 17:22
Copy link
Contributor

@fractalwrench fractalwrench left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@bidetofevil bidetofevil force-pushed the hho/open-trace-emitter branch from 356af39 to eacfa2d Compare November 29, 2024 16:41
@bidetofevil bidetofevil force-pushed the hho/open-event-emitter branch 2 times, most recently from d320798 to d13a3e9 Compare November 29, 2024 18:10
@bidetofevil bidetofevil changed the base branch from hho/open-trace-emitter to graphite-base/1320 November 29, 2024 18:21
@bidetofevil bidetofevil force-pushed the hho/open-event-emitter branch from d13a3e9 to f6ca940 Compare November 29, 2024 18:22
@bidetofevil bidetofevil changed the base branch from graphite-base/1320 to main November 29, 2024 18:22
@bidetofevil bidetofevil force-pushed the hho/open-event-emitter branch from f6ca940 to b27c99c Compare November 29, 2024 18:22
Copy link
Contributor

Dependency Review

✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.

OpenSSF Scorecard

PackageVersionScoreDetails

Scanned Files

@bidetofevil bidetofevil merged commit 68fe304 into main Nov 29, 2024
8 checks passed
Copy link
Collaborator Author

Merge activity

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

@bidetofevil bidetofevil deleted the hho/open-event-emitter branch November 29, 2024 18:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants