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-1083] introduce session context history #1187

Merged
merged 11 commits into from
Dec 9, 2021

Conversation

bcaudan
Copy link
Contributor

@bcaudan bcaudan commented Nov 30, 2021

Motivation

Third part of session management rework, cf #1166, #1180

Changes

  • move ContextHistory to core
  • expose sessionStore.expireObservable
  • store session context history in session management
  • use session history in RUM and Logs

Testing

  • Local
  • Staging
  • Unit
  • End to end

I have gone over the contributing documentation.

@bcaudan bcaudan changed the title Bcaudan/rework session part 3 ♻️ [RUMF-1083] introduce session context history Nov 30, 2021
@bcaudan bcaudan force-pushed the bcaudan/rework-session_part-3 branch 2 times, most recently from 1884bc3 to 810a862 Compare November 30, 2021 13:55
@bcaudan bcaudan marked this pull request as ready for review November 30, 2021 14:59
@bcaudan bcaudan requested review from a team as code owners November 30, 2021 14:59
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 great!

Maybe we could change the session API a bit to have something like:

interface SessionHistory {
  findSession(date: RelativeTime): { isTracked(): boolean, getId(): boolean, ...}
}

So when accessing multiple attributes from the same session we don't have to look for it through the history every time:

const session = sessionHistory.findSession(startTime) 
if (session.isTracked()) {
  ...
  session: { id: session.getId()! }
}

We could also imagine having a session type like:

type Session<T> = {
  tracked: true,
  id: string,
  replayPlan: boolean
} | {
  tracked: false,
  replayPlan: false,
}

packages/core/src/domain/session/sessionStore.ts Outdated Show resolved Hide resolved
packages/core/src/domain/session/sessionStore.ts Outdated Show resolved Hide resolved
packages/core/src/domain/session/sessionManagement.spec.ts Outdated Show resolved Hide resolved
packages/rum-core/src/rawRumEvent.types.ts Show resolved Hide resolved
@codecov-commenter
Copy link

codecov-commenter commented Dec 1, 2021

Codecov Report

Merging #1187 (1bb1d7d) into main (6858d86) will increase coverage by 0.12%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1187      +/-   ##
==========================================
+ Coverage   88.85%   88.98%   +0.12%     
==========================================
  Files         100      100              
  Lines        4270     4293      +23     
  Branches      971      975       +4     
==========================================
+ Hits         3794     3820      +26     
+ Misses        476      473       -3     
Impacted Files Coverage Δ
packages/core/src/tools/contextHistory.ts 100.00% <ø> (ø)
packages/logs/src/domain/logger.ts 94.73% <ø> (ø)
packages/rum-core/src/domain/urlContexts.ts 100.00% <ø> (ø)
packages/rum-core/src/rawRumEvent.types.ts 100.00% <ø> (ø)
...kages/core/src/domain/session/sessionManagement.ts 100.00% <100.00%> (ø)
packages/core/src/domain/session/sessionStore.ts 100.00% <100.00%> (ø)
packages/logs/src/boot/startLogs.ts 93.18% <100.00%> (+0.15%) ⬆️
packages/logs/src/domain/loggerSession.ts 100.00% <100.00%> (+4.34%) ⬆️
packages/rum-core/src/boot/startRum.ts 43.33% <100.00%> (ø)
packages/rum-core/src/domain/assembly.ts 100.00% <100.00%> (ø)
... and 6 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 6858d86...1bb1d7d. Read the comment docs.

@bcaudan
Copy link
Contributor Author

bcaudan commented Dec 1, 2021

Maybe we could change the session API a bit to have something like:

interface SessionHistory {
  findSession(date: RelativeTime): { isTracked(): boolean, getId(): boolean, ...}
}

@BenoitZugmeyer that looks interesting, I'd be in favor of exploring it in a following PR though

@bcaudan bcaudan force-pushed the bcaudan/rework-session_part-3 branch from 7b4faca to 73a04fc Compare December 1, 2021 15:26
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.

LGTM!

Base automatically changed from bcaudan/rework-session_part-2 to main December 9, 2021 08:46
@bcaudan bcaudan force-pushed the bcaudan/rework-session_part-3 branch from 604c333 to 1641792 Compare December 9, 2021 08:52
@bcaudan bcaudan force-pushed the bcaudan/rework-session_part-3 branch from 1641792 to 1bb1d7d Compare December 9, 2021 08:57
@bcaudan bcaudan merged commit baef148 into main Dec 9, 2021
@bcaudan bcaudan deleted the bcaudan/rework-session_part-3 branch December 9, 2021 09:49
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