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-530][RUM] allow using RUM API before init #551

Merged
merged 15 commits into from
Oct 12, 2020

Conversation

BenoitZugmeyer
Copy link
Member

Motivation

We want to allow using RUM APIs before initialising the SDK. (see #545 for the Logs side)

Changes

  • Remove stubs and regroup the public API implementation
  • Use a bounded buffer to collect pre-init user actions and send them at init
  • Make getInternalContext return undefined before init

Testing

Unit tests have been updated. Manual testing can be done by running some DD_RUM.addUserAction('foo') or DD_RUM.getInternalContext() before DD_RUM.init().


I have gone over the contributing documentation.

@BenoitZugmeyer BenoitZugmeyer requested a review from a team as a code owner October 2, 2020 16:20
@BenoitZugmeyer BenoitZugmeyer changed the title ✨ [RUMF-530][Logs] allow using RUM API before init ✨ [RUMF-530][RUM] allow using RUM API before init Oct 5, 2020
packages/rum/src/rum.ts Outdated Show resolved Hide resolved
packages/rum/src/rum.ts Outdated Show resolved Hide resolved
packages/rum/src/rum.ts Show resolved Hide resolved
packages/rum/test/rum.spec.ts Show resolved Hide resolved
@codecov-commenter
Copy link

Codecov Report

Merging #551 into master will decrease coverage by 1.55%.
The diff coverage is 64.28%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #551      +/-   ##
==========================================
- Coverage   89.66%   88.11%   -1.56%     
==========================================
  Files          38       38              
  Lines        2265     2255      -10     
  Branches      473      474       +1     
==========================================
- Hits         2031     1987      -44     
- Misses        234      268      +34     
Impacted Files Coverage Δ
packages/core/src/init.ts 70.96% <ø> (-10.86%) ⬇️
packages/rum/src/userActionCollection.ts 98.00% <ø> (ø)
packages/rum/src/rum.ts 77.77% <45.16%> (-14.93%) ⬇️
packages/rum/src/requestCollection.ts 78.37% <66.66%> (-11.87%) ⬇️
packages/rum/src/rum.entry.ts 87.50% <83.33%> (+12.87%) ⬆️
packages/core/src/errorCollection.ts 89.28% <0.00%> (-9.53%) ⬇️
packages/rum/src/rumSession.ts 90.62% <0.00%> (-9.38%) ⬇️
packages/core/src/configuration.ts 86.66% <0.00%> (-5.00%) ⬇️
... 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 27f4afa...7df9f30. Read the comment docs.

Comment on lines 97 to 98
context: combine({}, context),
globalContext: combine({}, globalContextManager.get()),
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe having a cloneDeep (lodash style) method would be more explicit

Copy link
Member Author

Choose a reason for hiding this comment

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

FTR: I'll do this in a separate PR as it has other implications (combine does not perform a deep clone at the moment)

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

codecov-io commented Oct 9, 2020

Codecov Report

Merging #551 into master will decrease coverage by 0.78%.
The diff coverage is 73.86%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #551      +/-   ##
==========================================
- Coverage   89.31%   88.52%   -0.79%     
==========================================
  Files          38       39       +1     
  Lines        2265     2258       -7     
  Branches      473      474       +1     
==========================================
- Hits         2023     1999      -24     
- Misses        242      259      +17     
Impacted Files Coverage Δ
packages/core/src/init.ts 70.96% <ø> (-10.86%) ⬇️
packages/rum/src/lifeCycle.ts 100.00% <ø> (ø)
packages/rum/src/userActionCollection.ts 98.00% <ø> (ø)
packages/rum/src/rum.ts 82.19% <52.77%> (-6.13%) ⬇️
packages/logs/src/logger.ts 95.91% <66.66%> (-4.09%) ⬇️
packages/rum/src/requestCollection.ts 78.37% <66.66%> (-11.87%) ⬇️
packages/rum/src/rum.entry.ts 94.00% <95.83%> (+19.37%) ⬆️
packages/core/src/contextManager.ts 100.00% <100.00%> (ø)
packages/logs/src/logs.entry.ts 100.00% <100.00%> (ø)
... and 7 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 61d2252...9e4e487. Read the comment docs.

@BenoitZugmeyer BenoitZugmeyer merged commit 63fba33 into master Oct 12, 2020
@BenoitZugmeyer BenoitZugmeyer deleted the benoit/allow-api-call-before-init-rum branch October 12, 2020 10:08
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