♻️ [RUMF-1207] collect concurrent actions groundwork - move action history closer to action collection #1432
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Previously, we updated the view parent context because it included the current URL. Now that we have a dedicated `UrlContext` for this, we don't need to update the view context anymore: all values are immutable (view id and name).
This commit moves the "currentAction" ownership to the main `trackActions` function. This will allow future commits to iterate over the concept of "currentAction" and expose an API to access this information. To keep things more or less simple, the event listener logic has been moved away from the `trackActions` function. The `PendingAutoAction` class has been replaced with a `createAction` function, to follow the trend of preferring function over classes. It is also in charge of waiting the page to be idle, to make it self-contained: the whole action lifecycle is now in the same function. This will make it easier to track concurrent actions. This design might change drastically in "frustration-signals" future tasks.
Instead of using a plain variable, let's use the ContextHistory class directly! In future commits will use this history instead of the one in parentContexts.
BenoitZugmeyer
commented
Mar 21, 2022
| @@ -24,15 +24,6 @@ export function startParentContexts(lifeCycle: LifeCycle): ParentContexts { | |||
| viewContextHistory.setCurrent(buildViewContext(view), view.startClocks.relative) | |||
| }) | |||
|
|
|||
| lifeCycle.subscribe(LifeCycleEventType.VIEW_UPDATED, (view) => { | |||
There was a problem hiding this comment.
This change is unrelated to the PR, and I meant to include it in a follow-up PR, but I merged it on staging before realizing it was there. Sorry about that! I think it's small enough to include it here, you'll let me know.
This was referenced Mar 21, 2022
BenoitZugmeyer
changed the title
| @@ -41,117 +50,135 @@ export interface AutoActionCreatedEvent { | |||
|
|
|||
| // Maximum duration for automatic actions | |||
| export const AUTO_ACTION_MAX_DURATION = 10 * ONE_SECOND | |||
| export const ACTION_CONTEXT_TIME_OUT_DELAY = 5 * ONE_MINUTE // arbitrary | |||
|
|
|||
| interface ActionController { | |||
There was a problem hiding this comment.
What about ActionContext instead of ActionController? I think it would be more consistent with other context history
There was a problem hiding this comment.
I would be fine with it, but this type has a discard() method, which is not really context-y (like, it wouldn't be compatible with actual Context typings), don't you think?
packages/rum-core/src/domain/rumEventsCollection/action/trackActions.ts
Outdated
Show resolved
Hide resolved
Codecov Report
@@ Coverage Diff @@
## main #1432 +/- ##
==========================================
+ Coverage 89.79% 89.82% +0.02%
==========================================
Files 105 105
Lines 4292 4274 -18
Branches 955 952 -3
==========================================
- Hits 3854 3839 -15
+ Misses 438 435 -3
📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more |
packages/rum-core/src/domain/rumEventsCollection/action/actionCollection.ts
Outdated
Show resolved
Hide resolved
amortemousque
approved these changes
Mar 23, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation
Currently, actions are collected in
actionCollection, and an history is built inparentContext. To communicate between the twoLifeCycleevents are used, and the currently active action is stored in those two places.With concurrent actions, this implementation will be a bit more complex because
LifeCycleevents will need to be extended to track which actions are created/discarded, and a list of current actions have to be maintained in both places. This makes the whole process unnecessarily complex and error-prone.To make things a bit simpler, let's make the action collection in charge of managing the action history, and expose an
ActionContextsclass the same way we do for URLs and sessions.Changes
See commits.
Testing
I have gone over the contributing documentation.