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

✨[RUMF-373] Add View load duration and load type #388

Merged
merged 48 commits into from
May 20, 2020

Conversation

mquentin
Copy link
Contributor

@mquentin mquentin commented May 4, 2020

What it does

Implement a load duration for each collected View.
This duration starts at the View start time and end when there is no loading activity for a given time.
As the view load occurs before any possible user actions, the collection of user action starts right after.

In addition, a view loading type is collected in view to identify initial page load and route change as any new view is loaded.

Context

View load and User actions need to have separated lifecycle. The view loading mode needs to be identify.

Change

  • Prevent UA collection during the view load
  • Collect the loading mode
  • Test UA collection and View loading lifecycles
  • Test view loading mode scenarios

@mquentin mquentin marked this pull request as ready for review May 4, 2020 14:19
@mquentin mquentin requested a review from a team as a code owner May 4, 2020 14:19
Copy link
Member

@BenoitZugmeyer BenoitZugmeyer left a comment

Choose a reason for hiding this comment

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

This looks nice!

I think that we could make things simpler and easier to understand by moving some parts around:

  • userActionCollection should expose a cancelCurrentUserAction function that viewCollection could call to cancel a potential user action

  • a new utility module could be created to track page activities, and move some of the useraction code into it. It could expose a function like waitPageIdle(callback: (endTime: number | undefined) => void): { stop(): void }, that would be used by both userActionCollection and viewCollection.

By doing this, we shouldn't need a new lifecycle event (VIEW_LOAD_COMPLETED). The viewCollection logic would not use functions named waitUserActionCompletion (this is unintuitive: I understand that loadDuration follows the same logic than useraction, but it should not be based on it).

We can talk about this if it's unclear!

packages/rum/src/userActionCollection.ts Outdated Show resolved Hide resolved
packages/rum/src/userActionCollection.ts Outdated Show resolved Hide resolved
packages/rum/src/userActionCollection.ts Outdated Show resolved Hide resolved
packages/rum/test/userActionCollection.spec.ts Outdated Show resolved Hide resolved
packages/rum/src/viewCollection.ts Outdated Show resolved Hide resolved
packages/rum/src/viewCollection.ts Outdated Show resolved Hide resolved
packages/rum/src/userActionCollection.ts Outdated Show resolved Hide resolved
packages/rum/src/userActionCollection.ts Outdated Show resolved Hide resolved
packages/rum/src/userActionCollection.ts Outdated Show resolved Hide resolved
packages/rum/src/userActionCollection.ts Outdated Show resolved Hide resolved
packages/rum/src/userActionCollection.ts Outdated Show resolved Hide resolved
packages/rum/src/viewCollection.ts Outdated Show resolved Hide resolved
packages/rum/test/userActionCollection.spec.ts Outdated Show resolved Hide resolved
packages/rum/test/viewCollection.spec.ts Outdated Show resolved Hide resolved
packages/rum/test/viewCollection.spec.ts Outdated Show resolved Hide resolved
packages/rum/test/viewCollection.spec.ts Outdated Show resolved Hide resolved
@codecov-io
Copy link

codecov-io commented May 5, 2020

Codecov Report

Merging #388 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #388   +/-   ##
=======================================
  Coverage   86.95%   86.95%           
=======================================
  Files          31       31           
  Lines        1733     1733           
  Branches      348      348           
=======================================
  Hits         1507     1507           
  Misses        226      226           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4614cfd...4614cfd. Read the comment docs.

packages/rum/src/userActionCollection.ts Outdated Show resolved Hide resolved
packages/rum/src/trackPageActivities.ts Outdated Show resolved Hide resolved
packages/rum/src/trackPageActivities.ts Outdated Show resolved Hide resolved
packages/rum/src/viewCollection.ts Outdated Show resolved Hide resolved
packages/rum/test/userActionCollection.spec.ts Outdated Show resolved Hide resolved
packages/rum/test/userActionCollection.spec.ts Outdated Show resolved Hide resolved
packages/rum/test/userActionCollection.spec.ts Outdated Show resolved Hide resolved
@mquentin mquentin changed the title Add View load duration and load type ✨[RUMF-373] Add View load duration and load type May 7, 2020
Copy link
Member

@BenoitZugmeyer BenoitZugmeyer left a comment

Choose a reason for hiding this comment

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

OK for me, I have two last small comments, but it looks fine otherwise!

packages/rum/src/userActionCollection.ts Outdated Show resolved Hide resolved
packages/rum/src/viewCollection.ts Outdated Show resolved Hide resolved
packages/rum/src/trackPageActivities.ts Outdated Show resolved Hide resolved
packages/rum/src/userActionCollection.ts Outdated Show resolved Hide resolved
packages/rum/src/userActionCollection.ts Outdated Show resolved Hide resolved
packages/rum/src/userActionCollection.ts Outdated Show resolved Hide resolved
packages/rum/src/userActionCollection.ts Outdated Show resolved Hide resolved
packages/rum/src/viewCollection.ts Outdated Show resolved Hide resolved
packages/rum/test/viewCollection.spec.ts Outdated Show resolved Hide resolved
packages/rum/src/userActionCollection.ts Outdated Show resolved Hide resolved
packages/rum/src/userActionCollection.ts Outdated Show resolved Hide resolved
packages/rum/src/viewCollection.ts Outdated Show resolved Hide resolved
packages/rum/src/rum.ts Outdated Show resolved Hide resolved
packages/rum/src/userActionCollection.ts Outdated Show resolved Hide resolved
packages/rum/src/viewCollection.ts Outdated Show resolved Hide resolved
packages/rum/test/viewCollection.spec.ts Outdated Show resolved Hide resolved
packages/rum/test/viewCollection.spec.ts Outdated Show resolved Hide resolved
packages/rum/test/viewCollection.spec.ts Outdated Show resolved Hide resolved
packages/rum/test/viewCollection.spec.ts Show resolved Hide resolved
packages/rum/test/userActionCollection.spec.ts Outdated Show resolved Hide resolved
packages/rum/src/userActionCollection.ts Outdated Show resolved Hide resolved
packages/rum/test/userActionCollection.spec.ts Outdated Show resolved Hide resolved
packages/rum/test/userActionCollection.spec.ts Outdated Show resolved Hide resolved
packages/rum/test/userActionCollection.spec.ts Outdated Show resolved Hide resolved
@mquentin mquentin merged commit 80f0d35 into master May 20, 2020
@mquentin mquentin deleted the maxime.quentin/SDKviewAddLoadDurationAndLoadType branch May 20, 2020 16:10
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.

4 participants