-
Notifications
You must be signed in to change notification settings - Fork 140
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
♻️ [RUM-2445] split RUM and Logs public APIs modules #2575
Conversation
83bb638
to
b8a9a4e
Compare
/to-staging |
🚂 Branch Integration: starting soon, merge in < 0s Commit b8a9a4ec84 will soon be integrated into staging-03. This build is going to start soon! (estimated merge in less than 0s) Use |
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #2575 +/- ##
==========================================
- Coverage 92.82% 92.77% -0.06%
==========================================
Files 232 235 +3
Lines 6760 6781 +21
Branches 1486 1484 -2
==========================================
+ Hits 6275 6291 +16
- Misses 485 490 +5 ☔ View full report in Codecov by Sentry. |
… into staging-03 Co-authored-by: Benoît Zugmeyer <[email protected]>
🚂 Branch Integration: This commit was successfully integrated Commit b8a9a4ec84 has been merged into staging-03 in merge commit f9bed34ac9. Check out the triggered pipeline on Gitlab 🦊 |
function createPostStartStrategy(initConfiguration: RumInitConfiguration, startRumResult: StartRumResult): Strategy { | ||
return assign( | ||
{ | ||
init: (initConfiguration: RumInitConfiguration) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❓ question: Can we avoid to redefined the init function on the post strategy knowing that the one in the preStart already handles multiples init?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the record: I tried to make some changes, but after a review and discussions with Aymeric we were not quite satisfied with the tradeoffs. It seems to be better to keep "pre start" and "post start" strategies strictly separated.
/to-staging |
🚂 Branch Integration: starting soon, merge in < 9m Commit 0300c627a9 will soon be integrated into staging-05. This build is going to start soon! (estimated merge in less than 9m) Use |
… into staging-05 Co-authored-by: Benoît Zugmeyer <[email protected]>
🚂 Branch Integration: This commit was successfully integrated Commit 0300c627a9 has been merged into staging-05 in merge commit 46d8052c2a. Check out the triggered pipeline on Gitlab 🦊 |
…ify-public-apis-2
Motivation
The
rumPublicApi
andlogsPublicApi
modules are a bit large. As we want to add more logic before starting RUM and Logs for consent management, it is a good time to extract the "pre start" logic (code executed before actually starting RUM or Logs) to separate modules.Changes
In both RUM and Logs public APIs, introduce a
Strategy
interface for behaviors that differs before and after starting the SDK. TheStrategy
replaces the variousxxxStrategy
functions, and is created by a dedicatedcreatePreStartStrategy
function. The "post start" strategy is returned bystartRum
andstartLogs
functions, as before.In the RUM case, the
trackViewManually
logic has been revamped a little bit to make it a bit more explicit (the firststartView
call can be ignored and its related View options can be retrieved when starting RUM)Testing
I have gone over the contributing documentation.