feat(replay): Share performance instrumentation with tracing #9296

mydea · 2023-10-18T12:18:58Z

This streamlines web-vital & performance observer handling, by exposing a new addPerformanceInstrumentationHandler method from @sentry-internal/tracing.

This works similar to the instrumentation in utils, where the first time you add instrumentation for a given type, it will add a performance observer. And any further calls will just add more callbacks. This way, we avoid having multiple of the same performance observers.

Furthermore, this also aligns the handling of LCP capturing for replay. We used to do this separately, now we use the same data as for performance.

Finally, while doing this I noticed that a whole bunch of performance observer stuff we used to capture in Replay, was actually discarded 😬 so no need to capture these anymore at all. (We can always add it back later, if needed)

Some integration tests needed slight adjustments for this, probably due to minor timing semantics. But I think all the changes are good/"correct".

I also got rid of the event deduplication in replay 🤔 All tests are passing, so should we be good - or is there another thing we need to test there? This would supersede #8836.

Closes #9246

github-actions · 2023-10-18T12:29:45Z

size-limit report 📦

Path	Size
@sentry/browser (incl. Tracing, Replay) - Webpack (gzipped)	82.66 KB (+0.19% 🔺)
@sentry/browser (incl. Tracing, Replay) - Webpack with treeshaking flags (gzipped)	71.77 KB (+0.17% 🔺)
@sentry/browser (incl. Tracing) - Webpack (gzipped)	30.94 KB (+0.99% 🔺)
@sentry/browser - Webpack (gzipped)	21.26 KB (+0.07% 🔺)
@sentry/browser (incl. Tracing, Replay) - ES6 CDN Bundle (gzipped)	73.03 KB (+0.02% 🔺)
@sentry/browser (incl. Tracing) - ES6 CDN Bundle (gzipped)	28.93 KB (+0.82% 🔺)
@sentry/browser - ES6 CDN Bundle (gzipped)	21.09 KB (+0.08% 🔺)
@sentry/browser (incl. Tracing, Replay) - ES6 CDN Bundle (minified & uncompressed)	233.81 KB (+0.02% 🔺)
@sentry/browser (incl. Tracing) - ES6 CDN Bundle (minified & uncompressed)	87.77 KB (+0.78% 🔺)
@sentry/browser - ES6 CDN Bundle (minified & uncompressed)	62.76 KB (+0.05% 🔺)
@sentry/browser (incl. Tracing) - ES5 CDN Bundle (gzipped)	31.71 KB (+0.83% 🔺)
@sentry/react (incl. Tracing, Replay) - Webpack (gzipped)	83.05 KB (+0.1% 🔺)
@sentry/react - Webpack (gzipped)	21.29 KB (+0.08% 🔺)
@sentry/nextjs Client (incl. Tracing, Replay) - Webpack (gzipped)	99.43 KB (+0.06% 🔺)
@sentry/nextjs Client - Webpack (gzipped)	47.83 KB (+0.6% 🔺)

github-actions · 2023-10-18T13:18:32Z

Replay SDK metrics 🚀

		Plain	+Sentry			+Replay
	Revision	Value	Value	Diff	Ratio	Value	Diff	Ratio
LCP	This PR `23f4d37`	95.04 ms	84.44 ms	-10.60 ms	-11.15 %	99.77 ms	+4.73 ms	+4.97 %
LCP	Baseline `921b8df`	88.35 ms	75.54 ms	-12.81 ms	-14.49 %	69.47 ms	-18.88 ms	-21.37 %
CLS	This PR `23f4d37`	0.25 ms	0.26 ms	+0.01 ms	+5.54 %	0.25 ms	+0.01 ms	+2.07 %
CLS	Baseline `921b8df`	0.24 ms	0.25 ms	+0.01 ms	+4.86 %	0.26 ms	+0.02 ms	+9.58 %
CPU	This PR `23f4d37`	24.60 %	27.06 %	+2.46 pp	+10.01 %	55.90 %	+31.30 pp	+127.24 %
CPU	Baseline `921b8df`	22.22 %	23.95 %	+1.73 pp	+7.77 %	48.48 %	+26.26 pp	+118.15 %
JS heap avg	This PR `23f4d37`	3.78 MB	5.8 MB	+2.03 MB	+53.65 %	7.28 MB	+3.51 MB	+92.85 %
JS heap avg	Baseline `921b8df`	3.81 MB	6.8 MB	+3 MB	+78.70 %	7.28 MB	+3.47 MB	+91.08 %
JS heap max	This PR `23f4d37`	4.87 MB	7.29 MB	+2.41 MB	+49.53 %	12.54 MB	+7.67 MB	+157.29 %
JS heap max	Baseline `921b8df`	5.07 MB	8.12 MB	+3.06 MB	+60.35 %	12.95 MB	+7.89 MB	+155.68 %
netTx	This PR `23f4d37`	0 B	197.12 kB	+197.12 kB	n/a	104.24 kB	+104.24 kB	n/a
netTx	Baseline `921b8df`	0 B	290.49 kB	+290.49 kB	n/a	39.18 kB	+39.18 kB	n/a
netRx	This PR `23f4d37`	0 B	41 B	+41 B	n/a	82 B	+82 B	n/a
netRx	Baseline `921b8df`	0 B	41 B	+41 B	n/a	41 B	+41 B	n/a
netCount	This PR `23f4d37`	0	1	+1	n/a	2	+2	n/a
netCount	Baseline `921b8df`	0	1	+1	n/a	1	+1	n/a
netTime	This PR `23f4d37`	0.00 ms	153.92 ms	+153.92 ms	n/a	200.94 ms	+200.94 ms	n/a
netTime	Baseline `921b8df`	0.00 ms	118.23 ms	+118.23 ms	n/a	54.46 ms	+54.46 ms	n/a

Baseline results on branch: `develop`

Revision	LCP	CLS	CPU	JS heap avg	JS heap max	netTx	netRx	netCount	netTime
`a410b04`	-9.12 ms	+0.01 ms	+23.52 pp	+3.43 MB	+8.42 MB	+50.19 kB	+41 B	+1	+87.46 ms
`9218eaa`	-1.59 ms	-0.00 ms	+25.02 pp	+3.47 MB	+8.06 MB	+11.02 kB	+41 B	+1	+69.00 ms
`42a60b6`	-6.72 ms	+0.02 ms	+24.97 pp	+3.43 MB	+8.45 MB	+50.09 kB	+41 B	+1	+80.65 ms
`54babf0`	-9.97 ms	-0.02 ms	+25.74 pp	+3.42 MB	+8.29 MB	+38.49 kB	+41 B	+1	+98.01 ms
`046f868`	+1.85 ms	+0.00 ms	+23.91 pp	+3.44 MB	+8.43 MB	+50.08 kB	+41 B	+1	+75.77 ms
`16594de`	-7.14 ms	+0.00 ms	+23.37 pp	+3.46 MB	+7.72 MB	+11.05 kB	+41 B	+1	+63.46 ms
`d7de20e`	-10.79 ms	-0.00 ms	+25.71 pp	+3.45 MB	+7.9 MB	+11.06 kB	+41 B	+1	+67.65 ms
`d7de20e`	+5.92 ms	+0.01 ms	+24.47 pp	+3.41 MB	+8.4 MB	+50.26 kB	+41 B	+1	+66.02 ms
`7236510`	-2.12 ms	+0.01 ms	+21.49 pp	+3.41 MB	+8.57 MB	+50.25 kB	+41 B	+1	+67.97 ms

Previous results on branch: `fn/web-vitals`

Revision	LCP	CLS	CPU	JS heap avg	JS heap max	netTx	netRx	netCount	netTime
`921b8df`	-18.88 ms	+0.02 ms	+26.26 pp	+3.47 MB	+7.89 MB	+39.18 kB	+41 B	+1	+54.46 ms
`a410b04`	-9.12 ms	+0.01 ms	+23.52 pp	+3.43 MB	+8.42 MB	+50.19 kB	+41 B	+1	+87.46 ms
`9218eaa`	-1.59 ms	-0.00 ms	+25.02 pp	+3.47 MB	+8.06 MB	+11.02 kB	+41 B	+1	+69.00 ms
`42a60b6`	-6.72 ms	+0.02 ms	+24.97 pp	+3.43 MB	+8.45 MB	+50.09 kB	+41 B	+1	+80.65 ms
`54babf0`	-9.97 ms	-0.02 ms	+25.74 pp	+3.42 MB	+8.29 MB	+38.49 kB	+41 B	+1	+98.01 ms
`046f868`	+1.85 ms	+0.00 ms	+23.91 pp	+3.44 MB	+8.43 MB	+50.08 kB	+41 B	+1	+75.77 ms
`16594de`	-7.14 ms	+0.00 ms	+23.37 pp	+3.46 MB	+7.72 MB	+11.05 kB	+41 B	+1	+63.46 ms
`d7de20e`	-10.79 ms	-0.00 ms	+25.71 pp	+3.45 MB	+7.9 MB	+11.06 kB	+41 B	+1	+67.65 ms
`d7de20e`	+5.92 ms	+0.01 ms	+24.47 pp	+3.41 MB	+8.4 MB	+50.26 kB	+41 B	+1	+66.02 ms
`7236510`	-2.12 ms	+0.01 ms	+21.49 pp	+3.41 MB	+8.57 MB	+50.25 kB	+41 B	+1	+67.97 ms

*) pp - percentage points - an absolute difference between two percentages.
Last updated: Tue, 24 Oct 2023 08:28:35 GMT

packages/replay/src/replay.ts

billyvg · 2023-10-18T15:48:00Z

packages/replay/src/coreHandlers/performanceObserver.ts

-  return performanceObserver;
+  clearCallbacks.push(
+    addPerformanceInstrumentationHandler('lcp', ({ metric }) => {
+      replay.lcpMetric = metric;


Why not push this into performanceEvents?

What would this look like if we wanted to add other metrics (e.g. CLS)?

because it is not an performance event, but already the formed metric. Instead of this we could also have a performanceMetrics array eventually where we could also push other metrics like CLS in, that would work the same way. Currently we only transform the metric when we send it, but we could eventually refactor this to transform in here and already push the transformed event to a separate array.

I changed this a bit to make this more generic - now we keep performanceEntries and replayPerformanceEntries, which hopefully makes this a bit clearer.

billyvg · 2023-10-18T15:48:34Z

packages/replay/src/util/dedupePerformanceEntries.ts

lforst · 2023-10-19T07:57:06Z

packages/tracing-internal/src/browser/metrics/index.ts

@@ -103,25 +95,20 @@ export function startTrackingInteractions(): void {
        const duration = msToSec(entry.duration);

        transaction.startChild({
-          description: htmlTreeAsString(entry.target),
+          description: htmlTreeAsString((entry as PerformanceEventTiming).target),


l: Would it make sense to type the different performance entry types with a union to avoid these casts?

I am currently struggling with this quite a bit because non-browser apps complain that these types are in here, as they do not know that.

WIP trying to make this work 😬

I ended up typing this specific case more specifically so this should work now, hopefully, without a type cast.

lforst · 2023-10-19T07:57:51Z

packages/tracing-internal/src/browser/request.ts

    entries.forEach(entry => {
      if (isPerformanceResourceTiming(entry) && entry.name.endsWith(url)) {
        const spanData = resourceTimingEntryToSpanData(entry);
        spanData.forEach(data => span.setData(...data));
-        observer.disconnect();
+        // In the next tick, clean this handler up
+        setTimeout(cleanup);


l: Can we add a comment why we do this in the next tick?

AbhiPrasad · 2023-10-23T20:14:56Z

packages/tracing-internal/src/browser/instrument.ts

+const instrumented: { [key in InstrumentHandlerType]?: boolean } = {};
+
+/** Instruments given API */
+function instrument(type: InstrumentHandlerType): void {


I'd like to see instrument broken down into individual exported functions instead of using the switch statement (reduces bundle size), but I'm fine with keeping it for now.

Hmm, I wonder if it would really reduce bundle size, as we'd still need to do instrumented['cls'] etc. in each of the functions? But I can give it a try!

OK, I split this up, which is probably nicer anyhow. I was able to keep this in as reusable pieces as possible, making this as small as can be (and I think it shaved off a few bytes even).

mydea requested review from billyvg, k-fish, lforst, JonasBa and AbhiPrasad October 18, 2023 12:18

mydea self-assigned this Oct 18, 2023

mydea added the CI-Overhead-Measurements Add this label to run SDK overhead measurements on a PR label Oct 18, 2023

mydea force-pushed the fn/web-vitals branch from b461588 to 8ce1277 Compare October 18, 2023 13:11

mydea force-pushed the fn/web-vitals branch from 9de3757 to e81bfbb Compare October 18, 2023 13:57

JonasBa mentioned this pull request Oct 18, 2023

ref(replay): improve dedupe logic #8836

Closed

billyvg reviewed Oct 18, 2023

View reviewed changes

packages/replay/src/util/dedupePerformanceEntries.ts Outdated

Copy link

Member

billyvg Oct 18, 2023

Choose a reason for hiding this comment

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

👍

AbhiPrasad mentioned this pull request Oct 18, 2023

feat(types): Add statsd envelope types #9304

Merged

mydea force-pushed the fn/web-vitals branch from 032a007 to 94f3641 Compare October 19, 2023 07:31

lforst approved these changes Oct 19, 2023

View reviewed changes

mydea force-pushed the fn/web-vitals branch 5 times, most recently from 7c66a4e to 1c7b475 Compare October 23, 2023 12:32

AbhiPrasad approved these changes Oct 23, 2023

View reviewed changes

mydea added 4 commits October 24, 2023 10:03

feat(replay): Share performance instrumentation with tracing

ba4e746

unflake test ??

49c8e88

fix flaky tests...?

8a2fdc5

ref: Extract instrument into dedicated methods

e1592e9

mydea force-pushed the fn/web-vitals branch from 1c7b475 to e1592e9 Compare October 24, 2023 08:10

lforst approved these changes Oct 24, 2023

View reviewed changes

mydea merged commit a0ff516 into develop Oct 24, 2023
86 checks passed

mydea deleted the fn/web-vitals branch October 24, 2023 08:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(replay): Share performance instrumentation with tracing #9296

feat(replay): Share performance instrumentation with tracing #9296

mydea commented Oct 18, 2023

github-actions bot commented Oct 18, 2023 •

edited

Loading

github-actions bot commented Oct 18, 2023 •

edited

Loading

Baseline results on branch: `develop`

Previous results on branch: `fn/web-vitals`

billyvg Oct 18, 2023

mydea Oct 19, 2023

mydea Oct 23, 2023

billyvg Oct 18, 2023

lforst Oct 19, 2023

mydea Oct 19, 2023

mydea Oct 23, 2023

lforst Oct 19, 2023

AbhiPrasad Oct 23, 2023

mydea Oct 24, 2023

mydea Oct 24, 2023

feat(replay): Share performance instrumentation with tracing #9296

feat(replay): Share performance instrumentation with tracing #9296

Conversation

mydea commented Oct 18, 2023

github-actions bot commented Oct 18, 2023 • edited Loading

size-limit report 📦

github-actions bot commented Oct 18, 2023 • edited Loading

Replay SDK metrics 🚀

Baseline results on branch: develop

Previous results on branch: fn/web-vitals

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Oct 18, 2023 •

edited

Loading

github-actions bot commented Oct 18, 2023 •

edited

Loading

Baseline results on branch: `develop`

Previous results on branch: `fn/web-vitals`