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.

Sign up for GitHub

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

Jump to bottom

⚗✨ [RUMF-1209] introduce "dead" and "error" frustration types #1487

Merged
merged 19 commits into from
Apr 28, 2022
Merged

⚗✨ [RUMF-1209] introduce "dead" and "error" frustration types #1487

merged 19 commits into from
Apr 28, 2022

Conversation

BenoitZugmeyer
Copy link
Member

@BenoitZugmeyer BenoitZugmeyer commented Apr 5, 2022 •
edited
Loading

Motivation

This is the first PR to implement action.frustration_type field. It has a bit of groundworks, so it might look weird or overkill at some places because subsequent PRs will iterate over this new design, but feel free to challenge the design and we'll talk.

This PR will be followed by #1488

Changes

Groundwork on trackActions and implement "dead" and "error" frustration types.

Testing

  • Local
  • Staging
  • Unit
  • End to end

I have gone over the contributing documentation.

@BenoitZugmeyer BenoitZugmeyer requested a review from a team as a code owner April 5, 2022 15:12
@BenoitZugmeyer BenoitZugmeyer force-pushed the benoit/frustration-signals-1 branch from 49f663c to 284ba68 Compare April 5, 2022 15:14
@BenoitZugmeyer
♻️✅ [RUMF-1209] merge trackActions tests
3b4a283 
It makes no sense anymore to split trackActions into two `describe`,
since the `newAction` function does not exist anymore, and tests are
mostly similar. Let's merge the two `describe` and reorder a few test.
@BenoitZugmeyer
♻️ [RUMF-1209] frustration detection groundwork
c4e51a8 
This commit changes the design of `trackActions` to prepare for
frustration detection. In particular, it re-introduce the concept of
"potential action" that will be used when generating rage clicks.

It also introduces a concept of `TrackActionState` to be able to split
the implementation into multiple functions, as the `trackActions`
implementation will grow a bit.
@BenoitZugmeyer BenoitZugmeyer force-pushed the benoit/frustration-signals-1 branch from 284ba68 to 176d6d3 Compare April 5, 2022 15:18
@codecov-commenter
Copy link

codecov-commenter commented Apr 5, 2022 •
edited
Loading

Codecov Report

Merging #1487 (6b75376) into main (97aecd9) will increase coverage by 0.03%.
The diff coverage is 96.47%.

@@            Coverage Diff             @@
##             main    #1487      +/-   ##
==========================================
+ Coverage   89.91%   89.94%   +0.03%     
==========================================
  Files         111      111              
  Lines        4322     4346      +24     
  Branches      961      968       +7     
==========================================
+ Hits         3886     3909      +23     
- Misses        436      437       +1
Impacted Files Coverage Δ
packages/rum-core/src/domain/lifeCycle.ts 100.00% <ø> (ø)
...ain/rumEventsCollection/action/actionCollection.ts 95.45% <0.00%> (ø)
...in/rumEventsCollection/action/trackClickActions.ts 97.46% <97.46%> (ø)
packages/core/src/tools/utils.ts 86.81% <100.00%> (+0.12%) ⬆️

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

@BenoitZugmeyer BenoitZugmeyer force-pushed the benoit/frustration-signals-1 branch from 176d6d3 to 4ec2927 Compare April 5, 2022 15:32
@amortemousque
Copy link
Collaborator

Like you said in the PR description, the design seems a bit convoluted for what it does currently.
It's a bit hard to challenge it if we don't really get where you want to go.
I'm personally not against having a bigger PR to better understand it. We can talk about it if you want :)

@BenoitZugmeyer
Copy link
Member Author

Like you said in the PR description, the design seems a bit convoluted for what it does currently. It's a bit hard to challenge it if we don't really get where you want to go. I'm personally not against having a bigger PR to better understand it. We can talk about it if you want :)

You can always have a look to the next PR: #1488

bcaudan
bcaudan reviewed Apr 14, 2022
View reviewed changes
packages/rum-core/src/domain/rumEventsCollection/action/trackActions.ts Outdated Show resolved Hide resolved
packages/rum-core/src/domain/rumEventsCollection/action/trackActions.ts Outdated Show resolved Hide resolved
packages/rum-core/src/domain/rumEventsCollection/action/trackActions.ts Outdated
Comment on lines 159 to 160
// Else complete the action at the end of the page activity
singleClickPotentialAction.complete(idleEvent.end)
Copy link
Contributor

Choose a reason for hiding this comment

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

💭 thought: ‏not related to the PR, I find my self not really integrating the idle page concept and like here, prefer to think of it as the page activity end.
It could be interesting to rename "idle page" occurrences then, wdyt?

Copy link
Member Author

Choose a reason for hiding this comment

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

Let's do it in a separate PR

packages/rum-core/src/domain/rumEventsCollection/action/trackActions.ts Outdated Show resolved Hide resolved
packages/rum-core/src/domain/rumEventsCollection/action/trackActions.ts Outdated Show resolved Hide resolved
test/e2e/scenario/rum/actions.scenario.ts Show resolved Hide resolved
amortemousque
amortemousque reviewed Apr 15, 2022
View reviewed changes
packages/rum-core/src/domain/rumEventsCollection/action/trackActions.ts Outdated
historyEntry.close(getRelativeTime(endTime))
eventCountsSubscription.stop()
if (eventCountsSubscription.eventCounts.errorCount > 0) {
frustrations.add(FrustrationType.ERROR)
Copy link
Collaborator

Choose a reason for hiding this comment

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

💬 suggestion: ‏Here you flag with FrustrationType.ERROR inside PotentialAction while for FrustrationType.DEAD you do it outside. Could be nice to do it in a single place.

@BenoitZugmeyer BenoitZugmeyer force-pushed the benoit/frustration-signals-1 branch 2 times, most recently from 01b7c28 to c50f02b Compare April 20, 2022 15:49
@BenoitZugmeyer
👌 remove TrackActionsState, replace with closures
4095ac3
@BenoitZugmeyer
👌🐛 do not set a duration for dead clicks
62fbb90
@BenoitZugmeyer
👌 rename endClick to stopClickProcessing
48e4b0a
@BenoitZugmeyer
👌 simplify PotentialAction
4c59a4e 
* Merge `notifyIfComplete` and `complete` into a new `validate` method
* Remove unneeded `finalState`
* Factorize cleanup code to a `stop` function
* Remove unused `base` property
* Implement a `addFrustration` method
@BenoitZugmeyer BenoitZugmeyer force-pushed the benoit/frustration-signals-1 branch from c50f02b to 4c59a4e Compare April 20, 2022 18:02
bcaudan
bcaudan reviewed Apr 27, 2022
View reviewed changes
test/e2e/scenario/rum/actions.scenario.ts Show resolved Hide resolved
packages/rum-core/src/domain/rumEventsCollection/action/trackClickActions.ts Outdated Show resolved Hide resolved
packages/rum-core/src/domain/rumEventsCollection/action/trackClickActions.ts Outdated Show resolved Hide resolved
packages/rum-core/src/domain/rumEventsCollection/action/trackClickActions.ts
Comment on lines +191 to +195
validate: (endTime?: TimeStamp) => {
if (isStopped) {
return
}
stop(endTime)
Copy link
Contributor

Choose a reason for hiding this comment

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

💬 suggestion: ‏What about moving the isStopped check inside the stop method?
it should allow us to:

  • not perform this check for both validate and discard
  • allow next PR to call stop before validate

Copy link
Member Author

Choose a reason for hiding this comment

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

Mmh if we move the isStopped check inside the stop function, how would we ensure that validate code is not run twice?

Copy link
Member Author

Choose a reason for hiding this comment

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

Anyway, isStopped is replaced with a more advanced state in the next PR, because we need to store additional data, so I don't think it matters too much.

Copy link
Contributor

Choose a reason for hiding this comment

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

how would we ensure that validate code is not run twice?

FMU, it was not checked before, can it happen?

packages/rum-core/src/domain/rumEventsCollection/action/trackClickActions.ts Outdated Show resolved Hide resolved
@BenoitZugmeyer
👌 more explicit type for id history
94a2f0a
@BenoitZugmeyer
👌 replace unnecessary call to isExperimentalFeatureEnabled
00689e0
@BenoitZugmeyer
👌 use a helper to transform a Set to an Array
7dc2112
@BenoitZugmeyer
🏷️ [RUMF-1209] add "ES2015" to the test app tsconfig
125a6df 
We now use Set to track actions, and it appears in module type
definitions (.d.ts). By default, TS does not include the type definition
for Set, so we need to use the `ES2015` lib.

This should not be a breaking change, since we already used Set in other
places of the SDK.
@BenoitZugmeyer BenoitZugmeyer merged commit c689b99 into main Apr 28, 2022
@BenoitZugmeyer BenoitZugmeyer deleted the benoit/frustration-signals-1 branch April 28, 2022 09:34
@BenoitZugmeyer BenoitZugmeyer mentioned this pull request May 6, 2022
Merged
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@amortemousque amortemousque amortemousque approved these changes

@bcaudan bcaudan bcaudan approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@BenoitZugmeyer @codecov-commenter @amortemousque @bcaudan