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

⚗ Collect scroll metrics #2180

Merged
merged 50 commits into from
Jun 23, 2023
Merged

⚗ Collect scroll metrics #2180

merged 50 commits into from
Jun 23, 2023

Conversation

hamza-kadiri
Copy link
Contributor

@hamza-kadiri hamza-kadiri commented Apr 20, 2023

Motivation

We want to track scoll metrics

Changes

  • update trackViews to add scroll metrics
  • add these metrics to the view event

Testing

This PR is not production ready yet, it's intended to deploy it to staging first

  • Local
  • Staging
  • Unit
  • End to end

I have gone over the contributing documentation.

@bits-bot
Copy link

bits-bot commented Apr 20, 2023

CLA assistant check
All committers have signed the CLA.

@hamza-kadiri hamza-kadiri changed the title ⚗ Initial poc for scroll depth collection ⚗ Collect scroll metrics May 11, 2023
@codecov-commenter
Copy link

codecov-commenter commented May 12, 2023

Codecov Report

Merging #2180 (131d2f7) into main (629ebbf) will increase coverage by 0.05%.
The diff coverage is 84.61%.

@@            Coverage Diff             @@
##             main    #2180      +/-   ##
==========================================
+ Coverage   94.17%   94.22%   +0.05%     
==========================================
  Files         205      206       +1     
  Lines        6144     6180      +36     
  Branches     1361     1368       +7     
==========================================
+ Hits         5786     5823      +37     
+ Misses        358      357       -1     
Impacted Files Coverage Δ
.../rum/src/domain/record/observers/scrollObserver.ts 25.00% <ø> (ø)
packages/rum/src/domain/record/record.ts 79.06% <ø> (ø)
packages/rum/src/domain/record/viewports.ts 82.35% <ø> (+9.62%) ⬆️
packages/rum-core/src/browser/scroll.ts 62.50% <62.50%> (ø)
...omain/rumEventsCollection/view/trackViewMetrics.ts 97.84% <93.75%> (-2.16%) ⬇️
packages/core/src/tools/experimentalFeatures.ts 100.00% <100.00%> (ø)
.../src/domain/rumEventsCollection/view/trackViews.ts 100.00% <100.00%> (ø)
.../domain/rumEventsCollection/view/viewCollection.ts 100.00% <100.00%> (ø)

... and 3 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@hamza-kadiri hamza-kadiri marked this pull request as ready for review June 8, 2023 12:49
@hamza-kadiri hamza-kadiri requested review from a team as code owners June 8, 2023 12:49
Comment on lines +114 to +115
// We compute scroll metrics at loading time to ensure we have scroll data when loading the view initially
// This is to ensure that we have the depth data even if the user didn't scroll or if the view is not scrollable.
Copy link
Contributor

Choose a reason for hiding this comment

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

❓ question: a fair amount of views don't have loading time, could it be bothering to not have scroll information for those views?
why not doing it at init?‏

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's not really a problem to not have scroll information for all the views, we can consider that this information is saved only when the user has spent enough time on the view (i.e. it has at least finished loading). Loading time is intersting because the page has usually reached a state where metrics like the scroll height are stable. It's true that it's not 100% reliable though, we sometimes send values before the page is fully loaded (See how small the scroll height can be here)

scrollMetrics = {
maxScrollHeight: scrollHeight,
maxScrollDepth: scrollDepth,
maxScrollDepthTime: newLoadingTime,
Copy link
Contributor

Choose a reason for hiding this comment

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

❓ question: ‏why maxScrollDepthTime should be related to loadingTime in this case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I kind of explain this in my previous comment, I think it's not ideal because the page is often not fully loaded at "loading time" I'm open to suggestions on how to improve this

Copy link
Contributor

@ThibautGeriz ThibautGeriz left a comment

Choose a reason for hiding this comment

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

LGTM for replay

export function trackScrollMetrics(
viewStart: ClocksState,
callback: (scrollMetrics: ScrollMetrics) => void,
getScrollMetrics = computeScrollValues
Copy link
Contributor

Choose a reason for hiding this comment

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

💬 suggestion: ‏reinforce naming and avoid confusion with the other getScrollMetrics

Suggested change
getScrollMetrics = computeScrollValues
getScrollValues = computeScrollValues

@hamza-kadiri hamza-kadiri merged commit 2301f97 into main Jun 23, 2023
17 checks passed
@hamza-kadiri hamza-kadiri deleted the hamza/scrolldepth-poc branch June 23, 2023 13:05
@BenoitZugmeyer BenoitZugmeyer mentioned this pull request Sep 25, 2023
4 tasks
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

7 participants