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-1113] Notify performance entries by batch #1255

Merged
merged 8 commits into from
Jan 18, 2022

Conversation

amortemousque
Copy link
Contributor

@amortemousque amortemousque commented Jan 7, 2022

Motivation

Notifying performance entries by batch instead of one by one could be a nice quick win to avoid repeating the same actions too many times, notably when computing the loading time. Since most of the time, performance entries are already provided as a list by the browser APIs, we could simply notify once for each list.

Changes

  • Update performance collection to notify multiple entries at the same time
  • Update performance entry event subscription accordingly

Testing

  • Local
  • Staging
  • Unit
  • End to end

I have gone over the contributing documentation.

@amortemousque amortemousque requested a review from a team as a code owner January 7, 2022 15:13
@amortemousque amortemousque force-pushed the aymeric/notify-performance-entries-by-batch branch from 331fa54 to a95a3b9 Compare January 7, 2022 15:17
@codecov-commenter
Copy link

codecov-commenter commented Jan 7, 2022

Codecov Report

Merging #1255 (043fa95) into main (0615e30) will increase coverage by 0.05%.
The diff coverage is 83.09%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1255      +/-   ##
==========================================
+ Coverage   89.93%   89.98%   +0.05%     
==========================================
  Files         105      105              
  Lines        4370     4385      +15     
  Branches      989      991       +2     
==========================================
+ Hits         3930     3946      +16     
+ Misses        440      439       -1     
Impacted Files Coverage Δ
...ages/rum-core/src/browser/performanceCollection.ts 14.14% <0.00%> (+0.14%) ⬆️
packages/core/src/tools/utils.ts 86.11% <100.00%> (+0.39%) ⬆️
packages/rum-core/src/domain/lifeCycle.ts 100.00% <100.00%> (ø)
...rumEventsCollection/longTask/longTaskCollection.ts 100.00% <100.00%> (ø)
...rumEventsCollection/resource/resourceCollection.ts 97.22% <100.00%> (+0.07%) ⬆️
...umEventsCollection/view/trackInitialViewTimings.ts 100.00% <100.00%> (ø)
...omain/rumEventsCollection/view/trackViewMetrics.ts 100.00% <100.00%> (ø)
packages/rum-core/src/domain/waitIdlePage.ts 98.11% <100.00%> (-0.04%) ⬇️

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 0615e30...043fa95. Read the comment docs.

@amortemousque amortemousque force-pushed the aymeric/notify-performance-entries-by-batch branch from a95a3b9 to dce37e5 Compare January 7, 2022 15:24
@amortemousque amortemousque force-pushed the aymeric/notify-performance-entries-by-batch branch from dce37e5 to 361a562 Compare January 12, 2022 10:59

lifeCycle.notify(LifeCycleEventType.PERFORMANCE_ENTRY_COLLECTED, entry)
function filterRumPerformanceEntries(entries: PerformanceEntry[]) {
Copy link
Member

Choose a reason for hiding this comment

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

💬 suggestion: ‏I find it a bit confusing to have different levels of filtering. Maybe this function could be used similarly to isIncompleteNavigation and isForbiddenResource? Or maybe we could simply remove it and let subscribers do the check themselves...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

filterRumPerformanceEntries filters first because it garantes that the rest of the code deals only with RumPerformanceEntry. Maybe a different naming would be better?

Copy link
Contributor

@bcaudan bcaudan Jan 14, 2022

Choose a reason for hiding this comment

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

I find it a bit confusing to have different levels of filtering.

+1

we could also have a filterRumAllowedPerformanceEntries that do all filtering operations

Copy link
Member

Choose a reason for hiding this comment

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

💬 suggestion: ‏Also you could use a isRumPerformanceEntry(entry: PerformanceEntry): entry is RumPerformanceEntry to have a nice typeguard without needing to cast as unknown as RumPerformanceEntry

Copy link
Contributor Author

@amortemousque amortemousque Jan 14, 2022

Choose a reason for hiding this comment

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

The type guard does not work because there is no type relation between PerformanceEntry and RumPerformanceEntry. Also having a single filterRumAllowedPerformanceEntries where there is every check is weird since we can have PerformanceEntry[] or RumPerformanceEntry[] as input.
I identified two possible options:

  • The PerformanceEntry[] as to go through the first filter to be checked and cast to RumPerformanceEntry[].
  • The filter function as to be defined something like filterRumAllowedPerformanceEntries(entries: Array<PerformanceEntry | RumPerformanceEntry>)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I changed the implementation according to what we said during the daily. Let me know what you think.

@amortemousque
Copy link
Contributor Author

amortemousque commented Jan 14, 2022

💭 thought: It's not directly related to this PR but by looking at the code all performance subscribers need only one entryType at a time.
To avoid filtering each time, we could:

  • have different lifecycle events for each entryType
  • transform performanceCollection to an observable to avoid polluting lifecycle events

Of course, it could be done later in a different PR.


lifeCycle.notify(LifeCycleEventType.PERFORMANCE_ENTRY_COLLECTED, entry)
function filterRumPerformanceEntries(entries: PerformanceEntry[]) {
Copy link
Member

Choose a reason for hiding this comment

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

💬 suggestion: ‏Also you could use a isRumPerformanceEntry(entry: PerformanceEntry): entry is RumPerformanceEntry to have a nice typeguard without needing to cast as unknown as RumPerformanceEntry

@amortemousque amortemousque merged commit 4d8c293 into main Jan 18, 2022
@amortemousque amortemousque deleted the aymeric/notify-performance-entries-by-batch branch January 18, 2022 14:07
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