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-430] new session strategy #343

Merged
merged 15 commits into from
Apr 8, 2020
Merged

✨[RUMF-430] new session strategy #343

merged 15 commits into from
Apr 8, 2020

Conversation

bcaudan
Copy link
Contributor

@bcaudan bcaudan commented Apr 7, 2020

Motivations

  • Allow page with iframe displayed to continue to expand the session even if there is no interaction with the main page
  • Reduce long lasting sessions

Changes

Behind the 'new-session' flag:

  • Check page visibility every 1 minute to expand the session
  • Add a session timeout at 4h
  • Enforce the session expiration if last interaction was more than 15 minutes ago

@bcaudan bcaudan requested a review from a team as a code owner April 7, 2020 09:04
add missing monitor
@codecov-io
Copy link

codecov-io commented Apr 7, 2020

Codecov Report

Merging #343 into master will increase coverage by 0.46%.
The diff coverage is 92.20%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #343      +/-   ##
==========================================
+ Coverage   86.79%   87.26%   +0.46%     
==========================================
  Files          29       29              
  Lines        1621     1672      +51     
  Branches      333      344      +11     
==========================================
+ Hits         1407     1459      +52     
+ Misses        214      213       -1     
Impacted Files Coverage Δ
packages/core/src/transport.ts 91.83% <0.00%> (ø)
packages/rum/src/performanceCollection.ts 63.79% <0.00%> (ø)
packages/rum/src/viewTracker.ts 93.75% <33.33%> (-1.49%) ⬇️
packages/core/src/sessionManagement.ts 100.00% <100.00%> (ø)
packages/core/src/utils.ts 96.09% <100.00%> (+0.33%) ⬆️
packages/logs/src/loggerSession.ts 100.00% <100.00%> (+4.54%) ⬆️
packages/rum/src/rumSession.ts 100.00% <100.00%> (ø)
packages/rum/src/userActionCollection.ts 98.73% <100.00%> (ø)
packages/logs/src/logger.ts 95.77% <0.00%> (+1.40%) ⬆️

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 0247a70...b3e79c8. Read the comment docs.

packages/core/src/sessionManagement.ts Show resolved Hide resolved
packages/core/src/sessionManagement.ts Outdated Show resolved Hide resolved
packages/core/src/sessionManagement.ts Outdated Show resolved Hide resolved
)
}

function trackVisibility(expandSession: () => void, visibilityStateProvider: () => VisibilityState) {
Copy link
Member

Choose a reason for hiding this comment

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

IMO

  • trackVisibility should expose an Observable instead of a callback function
  • be self contained: no visibilityStateProvider, use document.visibilityState directly. We can mock document.visibilityState in tests

Copy link
Contributor Author

@bcaudan bcaudan Apr 8, 2020

Choose a reason for hiding this comment

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

trackVisibility should expose an Observable instead of a callback function

what advantage do you see with it?

We can mock document.visibilityState in tests

How would you do that?

Copy link
Member

Choose a reason for hiding this comment

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

what advantage do you see with it?

I think having a common strategy to pass data streams around would make things more future-proof. For now, our Observable implementation is quite limited, but in the future it could be extendend with a "destructor" when unused (example), and we would have a common way to remove listeners and stuff. This would make functions like this more self-contained and easier to test.

But if you feel that this is thinking too far ahead, I'm okay to keep a callback for now :)

How would you do that?

Object.defineProperty(document, 'visibilityState', { get() { return "zog" }, configurable: true })
// then
delete document.visibilityState

Copy link
Contributor Author

Choose a reason for hiding this comment

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

About observable instead of callbacks, I am not sure that observable pattern should be enforce in every situation.
I think it:

  • allow us to easily reuse a data source in another part of the code
  • can help us to decouple some of the browser API to the domain logic
    But it also adds an extra layer of abstraction.
    About future-proof arguments, let's see when we will have the need to do more complicated stuff with our observable and address it at this time.

In this case, I am not sure to see much benefit to switch to observable.

packages/core/src/sessionManagement.ts Outdated Show resolved Hide resolved
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

@bcaudan bcaudan merged commit fa49ad9 into master Apr 8, 2020
@bcaudan bcaudan deleted the bcaudan/new-session branch April 8, 2020 14:05
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.

5 participants