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

♻️ [RUM-958] Use a performance observable instead of the lifecycle #2818

Merged
merged 17 commits into from
Jul 8, 2024

Conversation

amortemousque
Copy link
Contributor

Motivation

The performanceCollection has too many concerns and limitations:

  • it forces us to set a central configuration, while sometimes we might want to have slightly different options (ex here for durationThreshold)
  • we always listen for every performance entries while in some cases it might not be needed (ex for longtask)
  • we cannot use takeRecords() because the observer is centralized and it would take records for all types

The goal of this PR is to be able to use separate performance observer for each entry type:

  • The first iteration uses a performanceObservable for performanceResourceTimings
  • The next iteration will use it for other types of performance

Changes

  • Introduce a lightweight performanceObservable which removes the performanceObservable fallback
  • Move retrieveInitialDocumentResourceTiming from performanceCollection.ts in /domain/resource/retrieveInitialDocumentResourceTiming.ts
  • Use the performanceObservable to collect performance resource
  • Use the performanceObservable in waitPageActivityEnd

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 June 18, 2024 13:51
@amortemousque amortemousque changed the title ♻️ [RUM-958] Use performance observer instead of the lifecycle ♻️ [RUM-958] Use a performance observable instead of the lifecycle Jun 18, 2024
Copy link

cit-pr-commenter bot commented Jun 18, 2024

Bundles Sizes Evolution

📦 Bundle Name Base Size Local Size 𝚫 𝚫% Status
Rum 160.25 KiB 160.86 KiB 626 B +0.38%
Logs 57.91 KiB 57.91 KiB 4 B +0.01%
Rum Slim 108.77 KiB 109.39 KiB 626 B +0.56%
Worker 25.21 KiB 25.21 KiB 0 B 0.00%
🚀 CPU Performance
Action Name Base Average Cpu Time (ms) Local Average Cpu Time (ms) 𝚫
addglobalcontext 0.001 0.002 0.001
addaction 0.030 0.043 0.013
adderror 0.032 0.044 0.013
addtiming 0.001 0.001 0.000
startview 0.893 1.020 0.127
startstopsessionreplayrecording 0.771 0.974 0.203
logmessage 0.020 0.026 0.006
🧠 Memory Performance
Action Name Base Consumption Memory (bytes) Local Consumption Memory (bytes) 𝚫 (bytes)
addglobalcontext 22.20 KiB 20.92 KiB -1308 B
addaction 71.94 KiB 71.11 KiB -849 B
adderror 88.93 KiB 86.31 KiB -2681 B
addtiming 21.90 KiB 19.44 KiB -2520 B
startview 314.64 KiB 334.02 KiB 19.38 KiB
startstopsessionreplayrecording 13.51 KiB 15.01 KiB 1.50 KiB
logmessage 72.84 KiB 70.91 KiB -1979 B

🔗 RealWorld

packages/rum-core/src/browser/performanceObservable.ts Outdated Show resolved Hide resolved
packages/rum-core/src/boot/startRum.ts Outdated Show resolved Hide resolved
packages/rum-core/src/browser/performanceObservable.ts Outdated Show resolved Hide resolved
packages/rum-core/src/browser/performanceObservable.ts Outdated Show resolved Hide resolved
packages/rum-core/src/browser/performanceCollection.ts Outdated Show resolved Hide resolved
import { getDocumentTraceId } from '../tracing/getDocumentTraceId'
import { FAKE_INITIAL_DOCUMENT, computeRelativePerformanceTiming } from './resourceUtils'

export function retrieveInitialDocumentResourceTiming(
Copy link
Member

Choose a reason for hiding this comment

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

💭 thought: ‏I think we can "clean" this up and don't fake a performance entry like this. Are you planning to do it when we'll migrate the NAVIGATION performance entry to the new createPerformanceObserver?

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 thought of cleaning it but the PR is already quite substantial. Like you mention, let's discuss it in the following NAVIGATION PR ;)

packages/rum-core/src/browser/performanceObservable.ts Outdated Show resolved Hide resolved
packages/rum-core/test/testSetupBuilder.ts Outdated Show resolved Hide resolved
@codecov-commenter
Copy link

codecov-commenter commented Jun 25, 2024

Codecov Report

Attention: Patch coverage is 94.35484% with 7 lines in your changes missing coverage. Please review.

Project coverage is 93.64%. Comparing base (c48a9bd) to head (96c47cc).

Files Patch % Lines
...s/rum-core/test/emulate/mockPerformanceObserver.ts 87.87% 4 Missing ⚠️
...ages/rum-core/src/browser/performanceObservable.ts 95.55% 2 Missing ⚠️
...ages/rum-core/src/browser/performanceCollection.ts 85.71% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2818      +/-   ##
==========================================
+ Coverage   93.31%   93.64%   +0.32%     
==========================================
  Files         263      266       +3     
  Lines        7482     7548      +66     
  Branches     1669     1679      +10     
==========================================
+ Hits         6982     7068      +86     
+ Misses        500      480      -20     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Comment on lines +50 to +55
const performanceResourceSubscription = createPerformanceObservable(configuration, {
type: RumPerformanceEntryType.RESOURCE,
buffered: true,
}).subscribe((entries) => {
for (const entry of entries) {
if (entry.entryType === RumPerformanceEntryType.RESOURCE && !isRequestKind(entry)) {
if (!isRequestKind(entry)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

👏 So nice to have this

Comment on lines +25 to +27
if (buffered) {
notify(instance, bufferedEntries)
}
Copy link
Member

Choose a reason for hiding this comment

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

💬 suggestion: ‏this is Safari behavior, but I'm not sure this is the standard behavior. We should at least state this as a comment. Maybe this could be done more explicitly by passing a { synchronousBufferedEntriesCallback: true } flag to mockPerformanceObserver.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hum, the mock always triggers entries synchronously even for non buffered ones. To me, it's more by convenience to avoid playing with the clock each time we use mockPerformanceObserver.
So I'm not sure it's a good idea to add a special behaviour for buffered entries and if we generalise it for any entries adding synchronousBufferedEntriesCallback: true everywhere will be a bit cumbersome. Wdyt?

Copy link
Member

Choose a reason for hiding this comment

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

Let's not talk about async/sync, but whether the PerformanceObserver callback is called during or after observe(). My point is not to make everything asynchronous, just to make the observe default behavior closer to the standard.

Again, I'd be fine with just documenting this with a comment so we know why it's implemented like this.

@amortemousque
Copy link
Contributor Author

/to-staging

@dd-devflow
Copy link

dd-devflow bot commented Jul 1, 2024

🚂 Branch Integration: starting soon, median merge time is 0s

Commit 96c47cc4f5 will soon be integrated into staging-27.

Use /to-staging -c to cancel this operation!

dd-mergequeue bot added a commit that referenced this pull request Jul 1, 2024
Integrated commit sha: 96c47cc

Co-authored-by: Aymeric Mortemousque <aymeric.mortemousque@datadoghq.com>
@dd-devflow
Copy link

dd-devflow bot commented Jul 1, 2024

🚂 Branch Integration: This commit was successfully integrated

Commit 96c47cc4f5 has been merged into staging-27 in merge commit ae24d645c0.

@dd-devflow dd-devflow bot added the staging-27 label Jul 1, 2024
@amortemousque
Copy link
Contributor Author

/to-staging

@dd-devflow
Copy link

dd-devflow bot commented Jul 8, 2024

🚂 Branch Integration: starting soon, median merge time is 11m

Commit 96c47cc4f5 will soon be integrated into staging-28.

Use /to-staging -c to cancel this operation!

dd-devflow bot added a commit that referenced this pull request Jul 8, 2024
@dd-devflow
Copy link

dd-devflow bot commented Jul 8, 2024

🚨 Branch Integration: This merge request has conflicts

We couldn't automatically merge the commit 96c47cc4f5 into staging-28!

You can use this resolution PR: #2849 to fix the conflicts.

If you need support, contact us on Slack #devflow with those details!

@amortemousque amortemousque merged commit 30ae145 into main Jul 8, 2024
21 of 22 checks passed
@amortemousque amortemousque deleted the aymeric/performance-observer branch July 8, 2024 15:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants