⚗[RUMF-878] add trackViewsManually option (disabled) #867

bcaudan · 2021-05-27T10:19:19Z

Motivation

Following #850, another step for the manual view naming strategy, allowing to disable the automatic tracking of views on location change.

Changes

add trackViewsManually configuration (behind view-renaming flag)
when trackViewsManually is enabled:
- rum start is postponed until the first startView call
- new view is not created on session renew
- current view location is still updated on location change
rename boot/[package].ts to boot/start[Package].ts

Testing

unit, e2e, manual

I have gone over the contributing documentation.

codecov-commenter · 2021-05-27T10:19:51Z

Codecov Report

Merging #867 (f6e4cb9) into main (2d0efc9) will increase coverage by 0.33%.
The diff coverage is 95.04%.

@@            Coverage Diff             @@
##             main     #867      +/-   ##
==========================================
+ Coverage   88.78%   89.12%   +0.33%     
==========================================
  Files          81       81              
  Lines        3762     3787      +25     
  Branches      836      845       +9     
==========================================
+ Hits         3340     3375      +35     
+ Misses        422      412      -10

Impacted Files	Coverage Δ
packages/logs/src/boot/logs.entry.ts	`100.00% <ø> (ø)`
packages/logs/src/boot/startLogs.ts	`77.55% <ø> (ø)`
packages/rum-core/src/boot/startRum.ts	`38.46% <0.00%> (ø)`
packages/rum-recorder/src/boot/startRecording.ts	`100.00% <ø> (ø)`
.../src/domain/rumEventsCollection/view/trackViews.ts	`98.11% <95.45%> (+0.33%)`	⬆️
packages/rum-core/src/boot/rumPublicApi.ts	`94.89% <96.66%> (+0.27%)`	⬆️
packages/core/src/domain/configuration.ts	`96.42% <100.00%> (+19.50%)`	⬆️
.../domain/rumEventsCollection/view/viewCollection.ts	`100.00% <100.00%> (ø)`
...ages/rum-recorder/src/boot/rumRecorderPublicApi.ts	`100.00% <100.00%> (ø)`
packages/core/src/boot/init.ts	`81.81% <0.00%> (+12.12%)`	⬆️
... and 1 more

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 2d0efc9...f6e4cb9. Read the comment docs.

packages/rum-core/src/domain/rumEventsCollection/view/trackViews.ts

BenoitZugmeyer · 2021-05-27T13:20:18Z

packages/rum-core/src/domain/rumEventsCollection/view/trackViews.ts

+    lifeCycle.subscribe(LifeCycleEventType.SESSION_RENEWED, () => {
+      // do not trigger view update to avoid wrong data
+      currentView.end()
+      if (areViewsTrackedAutomatically) {
+        // Renew view on session renewal
+        currentView = trackViewChange()
+      }
+    })


Has it be discussed somewhere? Since the user can't know when the session is renewed, they won't be able to create a new view in this case.

Has it be discussed somewhere?

No!
IMO, it seems more aligned with the manual tracking strategy to not take the responsibility to start a new view in this case. It can also be deceptive to have view without name in this case and I am not sure that we can make the assumption that we can reuse the previous view name 😕
We could expose something like DD_RUM.onSessionRestart(callback) API to allow customer to deal with these cases.
wdyt?

Ok it makes sense to avoid making assumption on the new view name. I would prefer to avoid adding new APIs if possible. Maybe ensuring that the session is renewed when using DD_RUM.startView() could be enough?

looks like a good plan to me, I'd be in favor of adding this behavior in an a following PR though.

degregar · 2021-06-02T08:06:18Z

Hi!
I guess this new feature will help me using addTiming. Right now it counts time relative to the initial view timestamp. I've noticed, that when some resources load longer, addTiming is longer as well. I have a SPA app and I need to measure how long it's bootstrapping, and addTiming is unreliable to do this.

So I guess I could trigger startView() and from then on it would restart the clock? Or is there any other way to achieve this?

Do you have any idea when you will be ready with that feature?

bcaudan · 2021-06-02T08:12:06Z

Hi @degregar,

This is a work in progress but we hope to have something usable publicly in the coming weeks.
Would you mind opening a separate issue to describe your expectations about addTiming and the behavior you observed?
It would be easier to follow the conversation outside of this PR 🙂

- introduce `trackViewsManually` configuration - on manual mode, wait for the first startView to start RUM - keep processing beforeInitCalls for potential startView but execute calls at start - ensure that recording start after RUM start

packages/rum-core/src/boot/rumPublicApi.ts

packages/rum-recorder/src/boot/rumRecorderPublicApi.spec.ts

packages/rum-core/src/domain/rumEventsCollection/view/trackViews.ts

on manual mode: - do not create new view on location - do not track new view on session renewed - keep updating view location on location changes

- introduce viewTest helper to easily check view behaviors consistently - test behaviors for auto/manual mode - move location changes test to trackView.spec to be more symetric between auto and manual tests

packages/rum-core/src/domain/rumEventsCollection/view/trackViewMetrics.spec.ts

eslint-local-rules/disallowSpecImportSpec.js

bcaudan force-pushed the bcaudan/manual-view-tracking branch 3 times, most recently from c9aec32 to 750eb2b Compare May 27, 2021 12:32

BenoitZugmeyer reviewed May 27, 2021

View reviewed changes

packages/rum-core/src/domain/rumEventsCollection/view/trackViews.ts Outdated Show resolved Hide resolved

packages/rum-core/src/domain/rumEventsCollection/view/trackViews.ts Show resolved Hide resolved

BenoitZugmeyer reviewed May 27, 2021

View reviewed changes

bcaudan force-pushed the bcaudan/manual-view-tracking branch 2 times, most recently from c0c9cf4 to b66ed1b Compare June 1, 2021 16:11

bcaudan force-pushed the bcaudan/manual-view-tracking branch 6 times, most recently from 8b4c441 to d6d2ba7 Compare June 2, 2021 15:41

✅ add expected behavior

8103e5c

bcaudan force-pushed the bcaudan/manual-view-tracking branch from d6d2ba7 to e1f1ff0 Compare June 2, 2021 15:45

bcaudan force-pushed the bcaudan/manual-view-tracking branch 2 times, most recently from 775fa44 to a633a0b Compare June 2, 2021 16:22

BenoitZugmeyer reviewed Jun 3, 2021

View reviewed changes

packages/rum-core/src/boot/rumPublicApi.ts Show resolved Hide resolved

packages/rum-recorder/src/boot/rumRecorderPublicApi.spec.ts Show resolved Hide resolved

packages/rum-core/src/domain/rumEventsCollection/view/trackViews.ts Outdated Show resolved Hide resolved

bcaudan force-pushed the bcaudan/manual-view-tracking branch from 4f8a608 to 0cefb10 Compare June 3, 2021 13:55

✨ allow to not track views automatically

afcb405

on manual mode: - do not create new view on location - do not track new view on session renewed - keep updating view location on location changes

bcaudan force-pushed the bcaudan/manual-view-tracking branch from 0cefb10 to 9daeb53 Compare June 3, 2021 14:00

bcaudan added 2 commits June 3, 2021 16:06

✅ rework view unit tests

538ac38

- introduce viewTest helper to easily check view behaviors consistently - test behaviors for auto/manual mode - move location changes test to trackView.spec to be more symetric between auto and manual tests

♻️ rename boot/[package].ts to boot/start[package].ts

a4bb9ae

bcaudan force-pushed the bcaudan/manual-view-tracking branch from 9daeb53 to a4bb9ae Compare June 3, 2021 14:09

bcaudan marked this pull request as ready for review June 3, 2021 14:28

bcaudan requested a review from a team as a code owner June 3, 2021 14:28

BenoitZugmeyer reviewed Jun 4, 2021

View reviewed changes

packages/rum-core/src/domain/rumEventsCollection/view/trackViewMetrics.spec.ts Outdated Show resolved Hide resolved

bcaudan added 3 commits June 4, 2021 16:29

add eslint rule to check spec import inside spec

d9547e1

🐛 avoid to execute view tests twice

4fcff0d

Merge branch 'main' into bcaudan/manual-view-tracking

2b79460

bcaudan force-pushed the bcaudan/manual-view-tracking branch 2 times, most recently from 2fcf57f to 2b79460 Compare June 4, 2021 16:15

BenoitZugmeyer approved these changes Jun 7, 2021

View reviewed changes

eslint-local-rules/disallowSpecImportSpec.js Outdated Show resolved Hide resolved

eslint-local-rules/disallowSpecImportSpec.js Outdated Show resolved Hide resolved

bcaudan and others added 2 commits June 8, 2021 11:46

👌 check spec import everywhere + simplification

5be4f1b

Merge branch 'main' into bcaudan/manual-view-tracking

f6e4cb9

BenoitZugmeyer approved these changes Jun 8, 2021

View reviewed changes

bcaudan merged commit 8d93832 into main Jun 8, 2021

bcaudan deleted the bcaudan/manual-view-tracking branch June 8, 2021 11:49

bcaudan mentioned this pull request Jul 6, 2021

✨ [RUMF-928] enable manual view tracking #924

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

⚗[RUMF-878] add trackViewsManually option (disabled) #867

⚗[RUMF-878] add trackViewsManually option (disabled) #867

bcaudan commented May 27, 2021 •

edited

Loading

codecov-commenter commented May 27, 2021 •

edited

Loading

BenoitZugmeyer May 27, 2021

bcaudan Jun 1, 2021 •

edited

Loading

BenoitZugmeyer Jun 4, 2021

bcaudan Jun 8, 2021 •

edited

Loading

degregar commented Jun 2, 2021

bcaudan commented Jun 2, 2021

⚗[RUMF-878] add trackViewsManually option (disabled) #867

⚗[RUMF-878] add trackViewsManually option (disabled) #867

Conversation

bcaudan commented May 27, 2021 • edited Loading

Motivation

Changes

Testing

codecov-commenter commented May 27, 2021 • edited Loading

Codecov Report

BenoitZugmeyer May 27, 2021

Choose a reason for hiding this comment

bcaudan Jun 1, 2021 • edited Loading

Choose a reason for hiding this comment

BenoitZugmeyer Jun 4, 2021

Choose a reason for hiding this comment

bcaudan Jun 8, 2021 • edited Loading

Choose a reason for hiding this comment

degregar commented Jun 2, 2021

bcaudan commented Jun 2, 2021

bcaudan commented May 27, 2021 •

edited

Loading

codecov-commenter commented May 27, 2021 •

edited

Loading

bcaudan Jun 1, 2021 •

edited

Loading

bcaudan Jun 8, 2021 •

edited

Loading