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

New ViewDataSource #717

Merged
merged 11 commits into from
Apr 16, 2024
Merged

New ViewDataSource #717

merged 11 commits into from
Apr 16, 2024

Conversation

nelsitoPuglisi
Copy link
Contributor

@nelsitoPuglisi nelsitoPuglisi commented Apr 13, 2024

Goal

The purpose is to capture view navigation as spans instead of breadcrumbs. We have joined views logged manually through the public API, and automatically captured views, like Activities.

Testing

Unit, Integration and Manual test

Copy link
Contributor Author

nelsitoPuglisi commented Apr 13, 2024

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

Join @nelsitoPuglisi and the rest of your teammates on Graphite Graphite

@nelsitoPuglisi nelsitoPuglisi force-pushed the nelson/push-notification-data-source branch from 4117a92 to 504be70 Compare April 15, 2024 12:38
@nelsitoPuglisi nelsitoPuglisi force-pushed the nelson/push-notification-data-source branch from 504be70 to db6a928 Compare April 15, 2024 13:04
@nelsitoPuglisi nelsitoPuglisi force-pushed the nelson/push-notification-data-source branch from db6a928 to 251548a Compare April 15, 2024 17:23
@nelsitoPuglisi nelsitoPuglisi force-pushed the nelson/view-data-source branch 3 times, most recently from 39a54f8 to 80ac036 Compare April 15, 2024 18:39
@nelsitoPuglisi nelsitoPuglisi changed the title viewDataSource New ViewDataSource Apr 15, 2024
@nelsitoPuglisi nelsitoPuglisi force-pushed the nelson/push-notification-data-source branch from aee3ba3 to 83a4109 Compare April 15, 2024 21:24
@nelsitoPuglisi nelsitoPuglisi force-pushed the nelson/view-data-source branch 2 times, most recently from a42e011 to 40d0253 Compare April 15, 2024 21:47
@nelsitoPuglisi nelsitoPuglisi marked this pull request as ready for review April 15, 2024 21:47
@nelsitoPuglisi nelsitoPuglisi requested a review from a team as a code owner April 15, 2024 21:47
@@ -97,8 +97,8 @@ internal fun IntegrationTestRule.Harness.recordSession(
// end session 30s later by entering background
overriddenClock.tick(30000)
activityController?.pause()
activityService.onBackground()
activityController?.stop()
Copy link
Collaborator

Choose a reason for hiding this comment

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

@fractalwrench is it guaranteed that that activityLifecycleListners will fire before the ProcessLifecycleListeners if both have onStop() listeners registered? We require the view span to end before the session can be stopped, so we need to ensure there's no race condition between to listener calls so the order can be guaranteed,

Copy link
Contributor

Choose a reason for hiding this comment

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

Interesting question! registerActivityLifecycleCallbacks uses an ArrayList internally, and the docs for ProcessLifecycleOwner state the callback gets added via codegen or reflection. My assumption is that whatever class gets instantiated first will get called first. I'd assume that EmbraceProcessStateService gets instantiated first.

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

@@ -9,7 +9,7 @@ import io.embrace.android.embracesdk.payload.ViewBreadcrumb
/**
* Captures tap breadcrumbs.
*/
internal class ViewBreadcrumbDataSource(
internal class LegacyViewBreadcrumbDataSource(
Copy link
Contributor

Choose a reason for hiding this comment

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

This can be deleted as we've removed the ability to set the payload from this

Base automatically changed from nelson/push-notification-data-source to master April 16, 2024 12:49
Copy link

codecov bot commented Apr 16, 2024

Codecov Report

Attention: Patch coverage is 68.42105% with 6 lines in your changes are missing coverage. Please review.

Project coverage is 80.66%. Comparing base (b09b5ea) to head (03b2955).
Report is 2 commits behind head on master.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #717      +/-   ##
==========================================
+ Coverage   80.19%   80.66%   +0.47%     
==========================================
  Files         429      423       -6     
  Lines       11374    11260     -114     
  Branches     1725     1703      -22     
==========================================
- Hits         9121     9083      -38     
+ Misses       1532     1458      -74     
+ Partials      721      719       -2     
Files Coverage Δ
...ndroid/embracesdk/gating/SessionSanitizerFacade.kt 100.00% <ø> (ø)
...d/embracesdk/injection/DataCaptureServiceModule.kt 98.41% <ø> (-0.03%) ⬇️
...e/android/embracesdk/injection/DataSourceModule.kt 100.00% <100.00%> (ø)
...race/android/embracesdk/injection/SessionModule.kt 100.00% <100.00%> (ø)
.../embrace/android/embracesdk/payload/Breadcrumbs.kt 100.00% <100.00%> (ø)
...ssion/orchestrator/OrchestratorBoundaryDelegate.kt 75.00% <ø> (-2.78%) ⬇️
...ndroid/embracesdk/capture/crumbs/ViewDataSource.kt 81.25% <81.81%> (ø)
...racesdk/capture/crumbs/EmbraceBreadcrumbService.kt 66.66% <0.00%> (-13.34%) ⬇️

... and 4 files with indirect coverage changes

@nelsitoPuglisi nelsitoPuglisi merged commit dcffbad into master Apr 16, 2024
6 checks passed
@nelsitoPuglisi nelsitoPuglisi deleted the nelson/view-data-source branch April 16, 2024 15:39
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.

None yet

3 participants