✨ [RUMF-1207] collect concurrent actions #1434
Conversation
| }) | ||
| expect((serverRumEvents[0] as RumActionEvent).action.id).not.toEqual('7890') | ||
| expect((serverRumEvents[0] as RumActionEvent).action.id).toEqual(generatedRawRumActionEvent.action.id) |
There was a problem hiding this comment.
I made this change because I feared that not.toEqual was not specific enough now that it can be either an array or a string.
* Simplify the test context from `{ value: string }` to `string`. As
we'll write a few more tests, it will help to have more concise
values.
* `startClocks` is in fact called `startTime` in the code
* Add a test to make sure that calling `find()` without argument does
not return the last closed context.
Because we'll have multiple concurrent action entries, we need to change the `ContextHistory` to support this. With this commit: * instead of keeping a reference to the current active context, the context history now returns the reference with associated methods to "close" and "remove" it. The code using context history is able to keep multiple references to the active contexts. * a new `findAll` method is introduced returning a list of contexts instead of a single context that we'll use to get multiple action ids.
This commit changes the `action.id` typings to support either a single string or an array. It impacts: * any event that can be an action child * the internal monitoring context * the internal context, so logs will also inherit from this change
Finally, remove the "one action at a time" limitation and collect a new action for every click, even if another one is ongoing. To do this, we still need to keep a reference to pending actions because we need to discard them: * when a view is created (this will be removed in a future PR) * when stopping action collections (for tests) This is somewhat cumbersome, but not a huge deal either.
BenoitZugmeyer
force-pushed
the
benoit/concurrent-actions-2
branch
from
d4bb96d to
6beff83
Compare
Codecov Report
@@ Coverage Diff @@
## main #1434 +/- ##
==========================================
+ Coverage 89.86% 89.88% +0.02%
==========================================
Files 107 107
Lines 4291 4291
Branches 952 952
==========================================
+ Hits 3856 3857 +1
+ Misses 435 434 -1
📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more |
| * Return the latest context that was active during `startTime`, or the currently active context | ||
| * if no `startTime` is provided. This method assumes that entries are not overlapping. | ||
| */ | ||
| find(startTime: RelativeTime = END_OF_TIMES): Context | undefined { |
There was a problem hiding this comment.
I think it could be more expressive to have a find() with a required param and a getActive() a bit like before.
Wdyt?
| for (const entry of this.entries) { | ||
| if (entry.startTime <= startTime) { | ||
| if (startTime <= entry.endTime) { | ||
| return entry.context | ||
| } | ||
| break | ||
| } | ||
| if (startTime >= previousContext.startTime) { | ||
| return previousContext.context | ||
| } | ||
| } |
There was a problem hiding this comment.
Isn't this code equal to findAll(startTime)[0]?
There was a problem hiding this comment.
Yes, we could replace this by that. But we don't really need to iterate over all entries for find, so the current find implementation is in fact an optimization that was present in the previous ContextHistory implementation.
There was a problem hiding this comment.
I changed it. The optimization for findAll we talked about was not possible after investigation, but I still simplified the find method as you suggested.
There was a problem hiding this comment.
Actually I changed my mind, I think the find method should keep this optimization. Who knows, maybe an app could have many URL changes, and produce a lot of views. In this case, without optimization, all events produced will need to iterate over all views, which will have a performance impact.
| }) | ||
| expect((serverRumEvents[0] as RumActionEvent).action.id).not.toEqual('7890') | ||
| expect((serverRumEvents[0] as RumActionEvent).action.id).toEqual(generatedRawRumActionEvent.action.id) |
BenoitZugmeyer
force-pushed
the
benoit/concurrent-actions-2
branch
from
6b35d63 to
6beff83
Compare
Motivation
Follow-up of #1432 : this actually implements concurrent actions behind a FF
Changes
Testing
I have gone over the contributing documentation.