feat(replay): Replay Web Vital Breadcrumbs #12296

c298lee · 2024-05-30T23:14:46Z

Adds CLS, FID, and INP breadcrumbs. Updates all web vital breadcrumbs to include rating

Co-authored-by: Andrei <168741329+andreiborza@users.noreply.github.com>

I noticed here https://github.com/getsentry/sentry-javascript/pulls that some tests were becoming flaky. Investigating this some more, I figured out what was happening: 1. For idle spans, when the span is ended (in `on('spanEnded')`, which is triggered in `span.end()`), we call `onIdleSpanEnded` 2. In there, we call `beforeSpanEnd` 3. This is used by `browserTracingIntegration` to call `addPerformanceEntries(span)`, which adds performance spans to the idle span 4. After that, in `onIdleSpanEnded`, any spans having start/end timestamps outside of the idle span start/end timestamp will be discarded This lead to cases where performance spans were discarded because they were out of bounds of the idle span - which is not what we want! Now, this PR changes the timing of this a bit: Now, we actually patch `span.end` of the SentrySpan, to ensure that we can always run `beforeSpanEnd` with the correct timing, taking all spans that are added in `beforeSpanEnd` into account to adjust the start/end time of the idle span.

Oops... Fixes #12295

)

Re-use this for playwright config in E2E tests. Now, instead of repeating the whole playwright config, we can import the util from `@sentry-internal/test-utils` and pass it some config. I left the places where do not yet use the proxy, because for simplicity I built the util in a way that assumes the proxy (you can override it, but I figured not worth it for stuff that we will refactor soon anyhow). While at it, I also streamlined the playwright version used everywhere to an up-to-date version.

Part of #11910

When using `_experiments.traceInternals`, this could lead to an infinite loop, as we run this check when a breadcrumb is added, and we add this breadcrumb in the check, ... Fixes #12283

Part of #11910 - we already have error & transaction tests there, so we can just delete the sending tests.

…12313) Part of #11910 I renamed `standard-frontend-react` to `react-router-6` as this is more descriptive (what does standard frontend even mean?). This test now also uses the event proxy.

Add an e2e (or rather integration) test for our AWS lambda layer bundle. The motivation for this test is that we broke the layer during the initial v8 releases (multiple times for different reasons) without us noticing this in tests. Simply because we never tested the bundled SDK code that we create and publish for the lambda layer.

Fixes #12229

…ientTraceMetadata` option (#12323) Co-authored-by: Luca Forstner <luca.forstner@sentry.io>

Bumps [pnpm/action-setup](https://github.com/pnpm/action-setup) from 2 to 4. <details> <summary>Release notes</summary> <p><em>Sourced from <a href="https://github.com/pnpm/action-setup/releases">pnpm/action-setup's releases</a>.</em></p> <blockquote> <h2>v4.0.0</h2> <p>An error is thrown if one version of pnpm is specified in the <code>packageManager</code> field of <code>package.json</code> and a different version is specified in the action's settings <a href="https://redirect.github.com/pnpm/action-setup/pull/122">#122</a></p> <h2>v3.0.0</h2> <p>The action is updated to run on Node.js v20</p> <h2>v2.4.0</h2> <p>Add ability to install standalone binary (<a href="https://redirect.github.com/pnpm/action-setup/pull/92">pnpm/action-setup#92</a>).</p> <h2>v2.3.0</h2> <ul> <li>feat: specifying path to non-root <code>package.json</code> file (<a href="https://redirect.github.com/pnpm/action-setup/pull/88">pnpm/action-setup#88</a>).</li> <li>docs: improve.</li> </ul> <h2>v2.2.4</h2> <p>No deprecation warnings are printed about set-state and set-output commands (<a href="https://redirect.github.com/pnpm/action-setup/issues/57">pnpm/action-setup#57</a>)</p> <h2>v2.2.3</h2> <p>Bump Node.js version to 16 <a href="https://redirect.github.com/pnpm/action-setup/pull/56">pnpm/action-setup#56</a></p> <h2>v2.2.2</h2> <p>Fixing network issues.</p> <p>Related issues:</p> <ul> <li><a href="https://redirect.github.com/pnpm/action-setup/issues/44">pnpm/action-setup#44</a></li> <li><a href="https://redirect.github.com/pnpm/action-setup/issues/22">pnpm/action-setup#22</a></li> </ul> <p>Related PR:</p> <ul> <li><a href="https://redirect.github.com/pnpm/action-setup/pull/46">pnpm/action-setup#46</a></li> </ul> <h2>v2.2.1</h2> <p>Fix <code>"packageManager"</code> reader <a href="https://redirect.github.com/pnpm/action-setup/pull/35">pnpm/action-setup#35</a></p> <h2>v2.2.0</h2> <ul> <li>Support the <code>packageManager</code> field in the <code>package.json</code> file <a href="https://redirect.github.com/pnpm/action-setup/pull/33">pnpm/action-setup#33</a>.</li> <li>Use <code>@pnpm/fetch</code> as an attempt to fix <a href="https://redirect.github.com/pnpm/action-setup/issues/22">pnpm/action-setup#22</a> <a href="https://redirect.github.com/pnpm/action-setup/pull/32">pnpm/action-setup#32</a>.</li> </ul> <h2>v2.1.0</h2> <p>Support pnpm v7 <a href="https://redirect.github.com/pnpm/action-setup/pull/29">pnpm/action-setup#29</a></p> <h2>v2.0.1</h2> <p>Update versions in code examples in README.md to the latest ones to avoid users using the wrong versions.</p> </blockquote> </details> <details> <summary>Commits</summary> <ul> <li><a href="https://github.com/pnpm/action-setup/commit/fe02b34f77f8bc703788d5817da081398fad5dd2"><code>fe02b34</code></a> docs: bump action-setup version in README</li> <li><a href="https://github.com/pnpm/action-setup/commit/bee1f099e575ebe79c239f210b7b841a7597f87b"><code>bee1f09</code></a> feat: throw error when multiple versions specified (<a href="https://redirect.github.com/pnpm/action-setup/issues/122">#122</a>)</li> <li><a href="https://github.com/pnpm/action-setup/commit/ce859e384f8a1a0be70b423054e9fc15175c02a9"><code>ce859e3</code></a> refactor: replace <code>fs-extra</code> with Node.js built-in fs methods (<a href="https://redirect.github.com/pnpm/action-setup/issues/120">#120</a>)</li> <li><a href="https://github.com/pnpm/action-setup/commit/2ab6dce4f53589dada480b94c53cb05a06b04673"><code>2ab6dce</code></a> docs(README): fix link to LICENSE</li> <li><a href="https://github.com/pnpm/action-setup/commit/e280758d01dee817bb34e0eb1114fde6d3a9f1db"><code>e280758</code></a> docs(README): update dependency versions (<a href="https://redirect.github.com/pnpm/action-setup/issues/117">#117</a>)</li> <li><a href="https://github.com/pnpm/action-setup/commit/129abb77bf5884e578fcaf1f37628e41622cc371"><code>129abb7</code></a> Bump undici from 5.28.2 to 5.28.3 (<a href="https://redirect.github.com/pnpm/action-setup/issues/115">#115</a>)</li> <li><a href="https://github.com/pnpm/action-setup/commit/a3252b78c470c02df07e9d59298aecedc3ccdd6d"><code>a3252b7</code></a> docs(README): update version</li> <li><a href="https://github.com/pnpm/action-setup/commit/1ee9c9d01d7ec8c679f2f85de0ff31e8af2c9944"><code>1ee9c9d</code></a> feat!: node20 upgrade (<a href="https://redirect.github.com/pnpm/action-setup/issues/110">#110</a>)</li> <li><a href="https://github.com/pnpm/action-setup/commit/ebcfd6995dade4b0104ac774445cef8b3b4635b0"><code>ebcfd69</code></a> Bump actions/setup-node from 3 to 4 (<a href="https://redirect.github.com/pnpm/action-setup/issues/103">#103</a>)</li> <li><a href="https://github.com/pnpm/action-setup/commit/d2613e087f2e0aa841925861c5a5f7395d552177"><code>d2613e0</code></a> docs: update pnpm version in caching example (<a href="https://redirect.github.com/pnpm/action-setup/issues/94">#94</a>)</li> <li>Additional commits viewable in <a href="https://github.com/pnpm/action-setup/compare/v2...v4">compare view</a></li> </ul> </details> <br /> [![Dependabot compatibility score](https://dependabot-badges.githubapp.com/badges/compatibility_score?dependency-name=pnpm/action-setup&package-manager=github_actions&previous-version=2&new-version=4)](https://docs.github.com/en/github/managing-security-vulnerabilities/about-dependabot-security-updates#about-compatibility-scores) Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting `@dependabot rebase`. [//]: # (dependabot-automerge-start) [//]: # (dependabot-automerge-end) --- <details> <summary>Dependabot commands and options</summary> <br /> You can trigger Dependabot actions by commenting on this PR: - `@dependabot rebase` will rebase this PR - `@dependabot recreate` will recreate this PR, overwriting any edits that have been made to it - `@dependabot merge` will merge this PR after your CI passes on it - `@dependabot squash and merge` will squash and merge this PR after your CI passes on it - `@dependabot cancel merge` will cancel a previously requested merge and block automerging - `@dependabot reopen` will reopen this PR if it is closed - `@dependabot close` will close this PR and stop Dependabot recreating it. You can achieve the same result by closing it manually - `@dependabot show <dependency name> ignore conditions` will show all of the ignore conditions of the specified dependency - `@dependabot ignore this major version` will close this PR and stop Dependabot creating any more for this major version (unless you reopen the PR or upgrade to it yourself) - `@dependabot ignore this minor version` will close this PR and stop Dependabot creating any more for this minor version (unless you reopen the PR or upgrade to it yourself) - `@dependabot ignore this dependency` will close this PR and stop Dependabot creating any more for this dependency (unless you reopen the PR or upgrade to it yourself) </details> Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

Observed a flake here: https://github.com/getsentry/sentry-javascript/actions/runs/9348183996/job/25727088799?pr=12267#step:12:92

…4.1 (#12333)

github-actions · 2024-06-04T14:47:14Z

size-limit report 📦

Path	Size
@sentry/browser	22 KB (0%)
@sentry/browser (incl. Tracing)	33.19 KB (0%)
@sentry/browser (incl. Tracing, Replay)	68.92 KB (+0.17% 🔺)
@sentry/browser (incl. Tracing, Replay) - with treeshaking flags	62.23 KB (+0.23% 🔺)
@sentry/browser (incl. Tracing, Replay with Canvas)	72.98 KB (+0.18% 🔺)
@sentry/browser (incl. Tracing, Replay, Feedback)	85.1 KB (+0.13% 🔺)
@sentry/browser (incl. Tracing, Replay, Feedback, metrics)	86.94 KB (+0.13% 🔺)
@sentry/browser (incl. metrics)	26.19 KB (0%)
@sentry/browser (incl. Feedback)	38.17 KB (0%)
@sentry/browser (incl. sendFeedback)	26.59 KB (0%)
@sentry/browser (incl. FeedbackAsync)	31.15 KB (0%)
@sentry/react	24.77 KB (0%)
@sentry/react (incl. Tracing)	36.24 KB (0%)
@sentry/vue	26.01 KB (0%)
@sentry/vue (incl. Tracing)	35.04 KB (0%)
@sentry/svelte	22.14 KB (0%)
CDN Bundle	23.36 KB (0%)
CDN Bundle (incl. Tracing)	34.87 KB (0%)
CDN Bundle (incl. Tracing, Replay)	68.98 KB (+0.2% 🔺)
CDN Bundle (incl. Tracing, Replay, Feedback)	74.11 KB (+0.16% 🔺)
CDN Bundle - uncompressed	68.63 KB (0%)
CDN Bundle (incl. Tracing) - uncompressed	103.22 KB (0%)
CDN Bundle (incl. Tracing, Replay) - uncompressed	213.64 KB (+0.24% 🔺)
CDN Bundle (incl. Tracing, Replay, Feedback) - uncompressed	226.11 KB (+0.23% 🔺)
@sentry/nextjs (client)	35.59 KB (0%)
@sentry/sveltekit (client)	33.83 KB (0%)
@sentry/node	129.8 KB (0%)
@sentry/node - without tracing	92.55 KB (0%)
@sentry/aws-serverless	117.61 KB (0%)

mydea

looks pretty nice, with minimal bundle size increase 🚀 great work!

billyvg · 2024-06-06T15:40:33Z

packages/replay-internal/src/coreHandlers/performanceObserver.ts

+
+  function webVitalHandler(getter: (metric: Metric) => ReplayPerformanceEntry<AllPerformanceEntryData>): (data: { metric: Metric; }) => void {
+    return ({metric}) => void replay.replayPerformanceEntries.push(getter(metric));
+  }


nit: let's move this up where other functions in this method are defined. Functionally it works because it gets hoisted, but it's a bit awkward to have it after the return.

mydea · 2024-06-07T08:31:07Z

(Also a side note, please make sure that the flaky test detection check passes before merging this, we do not block merging on this but we shouldn't merge a PR that has a flaky test if we can avoid it 😅 )

billyvg

I think it's fine to skip the INP tests since they're flakey in CI -- let's open a draft PR that adds it in and make sure the draft PR is added to our backlog

billyvg · 2024-06-07T17:23:49Z

dev-packages/browser-integration-tests/suites/replay/customEvents/test.ts

@@ -47,7 +49,7 @@ sentryTest(

    expect(replayEvent0).toEqual(getExpectedReplayEvent({ segment_id: 0 }));

-    await page.locator('#img-button').click();


Should we keep this?

yes that click generates LCP, CLS and FID metrics and the next replay event

I mean the await

oh oops that should be kept, you're right

billyvg · 2024-06-07T19:49:40Z

dev-packages/browser-integration-tests/suites/replay/customEvents/test.ts

@@ -47,7 +49,7 @@ sentryTest(

    expect(replayEvent0).toEqual(getExpectedReplayEvent({ segment_id: 0 }));

-    await page.locator('#img-button').click();


I mean the await

billyvg · 2024-06-07T19:56:51Z

packages/replay-internal/src/util/createPerformanceEntries.ts

 }

 /**
 * Add a CLS event to the replay based on a CLS metric.
 */
 export function getCumulativeLayoutShift(metric: Metric): ReplayPerformanceEntry<WebVitalData> {
-  return getWebVital(metric, 'cumulative-layout-shift');
+  return getWebVital(metric, 'cumulative-layout-shift', undefined);


Is there not an attribution element for CLS? I see something like this in the web-vitals lib

oh there is, let me add that too

billyvg · 2024-06-07T19:57:39Z

packages/replay-internal/src/util/createPerformanceEntries.ts

 }

 /**
 * Add an INP event to the replay based on an INP metric.
 */
 export function getInteractionToNextPaint(metric: Metric): ReplayPerformanceEntry<WebVitalData> {
-  return getWebVital(metric, 'interaction-to-next-paint');
+  const lastEntry = metric.entries[metric.entries.length - 1] as (PerformanceEntry & { target?: Element }) | undefined;


Should we move this into a function?

Only FID and INP have the same types, so I'm not sure we should. LCP has element instead of target and CLS has an array which contains node

billyvg

Looks great! Let's talk about CLS more next week -- it may be beneficial to record all attribution nodes instead of the first.

Adds CLS, FID, and INP breadcrumbs. Updates all web vital breadcrumbs to include rating Closes #11639 --------- Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: Luca Forstner <luca.forstner@sentry.io> Co-authored-by: Andrei <168741329+andreiborza@users.noreply.github.com> Co-authored-by: Francesco Novy <francesco.novy@sentry.io> Co-authored-by: Lukas Stracke <lukas.stracke@sentry.io> Co-authored-by: Yamagishi Kazutoshi <ykzts@desire.sh> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

c298lee and others added 25 commits May 30, 2024 19:13

replay web vital breadcrumbs

59a985f

placeholder tests

6e29bcc

update tests and refactor

32a774b

chore: Document e2e test env vars (#12303)

35c1db0

Co-authored-by: Andrei <168741329+andreiborza@users.noreply.github.com>

fix(browser): Fix types export path for CJS (#12305)

da25922

Oops... Fixes #12295

feat(vercel-edge): Export core integrations from Vercel edge SDK (#12308

ba9acbe

)

test: Refactor webpack E2E tests to avoid sending to Sentry (#12312)

4772147

Part of #11910

fix(replay): Avoid infinite loop of logs (#12309)

2cb5215

When using `_experiments.traceInternals`, this could lead to an infinite loop, as we run this check when a breadcrumb is added, and we add this breadcrumb in the check, ... Fixes #12283

test(e2e): Streamline react-router-6-use-routes test (#12315)

66a8cf9

Part of #11910 - we already have error & transaction tests there, so we can just delete the sending tests.

test(e2e): Update react-router-6 test to avoid sending to Sentry (#…

1880316

…12313) Part of #11910 I renamed `standard-frontend-react` to `react-router-6` as this is more descriptive (what does standard frontend even mean?). This test now also uses the event proxy.

fix(feedback): Override TriggerLabel Option (#12316)

9ce85f7

Fixes #12229

fix(nextjs): Fix version detection and option insertion logic for `cl…

84886c7

…ientTraceMetadata` option (#12323) Co-authored-by: Luca Forstner <luca.forstner@sentry.io>

ref(browser): Ensure idle span ending is consistent (#12310)

5494cb5

feat(deps): Bump @sentry/cli from 2.31.2 to 2.32.1 (#12332)

10833b0

ci: Fix sveltekit flakes (#12327)

761b9d2

Observed a flake here: https://github.com/getsentry/sentry-javascript/actions/runs/9348183996/job/25727088799?pr=12267#step:12:92

feat(deps): Bump @opentelemetry/propagator-aws-xray from 1.3.1 to 1.2…

48b228c

…4.1 (#12333)

placeholder tests

03f68d0

Merge branch 'develop' into replay-web-vitals

0e110b1

linting

087b0b3

Merge branch 'develop' into replay-web-vitals

2436558

fix inp import

753c99b

c298lee added 3 commits June 4, 2024 12:28

format + update test

7c34d01

lint

0cc8090

fix test

f695f68

c298lee marked this pull request as ready for review June 4, 2024 18:18

mydea approved these changes Jun 6, 2024

View reviewed changes

billyvg reviewed Jun 6, 2024

View reviewed changes

c298lee added 7 commits June 6, 2024 14:05

PR comments

aa04cf8

add inp to test

f81ca6f

typos & tests

bdc7bca

lint & fix performance entry test

27c53ee

hopefully actually fixed some tests??

f523075

remove inp from e2e test

f21a812

try copying web-vitals-inp test?

3eaf3f9

c298lee added 4 commits June 7, 2024 11:27

Merge branch 'develop' into replay-web-vitals

a27fa22

add promise all to test

39e20b2

rm await inside all

e7d7db0

remove inp from tests

44590d1

c298lee requested a review from billyvg June 7, 2024 17:16

billyvg approved these changes Jun 7, 2024

View reviewed changes

c298lee added 2 commits June 7, 2024 14:30

get element from right place for other web vitals

4849a8f

Merge branch 'develop' into replay-web-vitals

98f29b5

c298lee requested a review from billyvg June 7, 2024 18:31

forgot to update e2e test

f2a287b

billyvg reviewed Jun 7, 2024

View reviewed changes

c298lee added 2 commits June 7, 2024 16:57

add cls node

738508a

lint

853ed14

c298lee requested a review from billyvg June 7, 2024 21:10

billyvg approved these changes Jun 7, 2024

View reviewed changes

c298lee merged commit 691e5de into develop Jun 7, 2024
106 checks passed

c298lee deleted the replay-web-vitals branch June 7, 2024 21:55

This was referenced Jul 3, 2024

[Snyk] Upgrade @sentry/browser from 8.8.0 to 8.9.1 gokulk16/type-to-calculate-web#28

Closed

[Snyk] Upgrade @sentry/browser from 8.8.0 to 8.9.2 gokulk16/type-to-calculate-web#29

Closed

		@@ -47,7 +49,7 @@ sentryTest(

		expect(replayEvent0).toEqual(getExpectedReplayEvent({ segment_id: 0 }));

		await page.locator('#img-button').click();

Uh oh!

feat(replay): Replay Web Vital Breadcrumbs #12296

feat(replay): Replay Web Vital Breadcrumbs #12296

Uh oh!

Conversation

c298lee commented May 30, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Jun 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

size-limit report 📦

Uh oh!

mydea left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mydea commented Jun 7, 2024

Uh oh!

billyvg left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

billyvg left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

c298lee commented May 30, 2024 •

edited

Loading

github-actions bot commented Jun 4, 2024 •

edited

Loading