feat: Report LCP metric on pageload transactions #2624

rhcarvalho · 2020-05-28T13:08:00Z

Largest Contentful Paint (LCP) is an user-centric metric for measuring
perceived load speed. It marks the point in the page load timeline when
the page's main content has likely loaded.

Reference: https://web.dev/lcp/#measure-lcp-in-javascript

getsentry-bot · 2020-05-28T13:14:11Z

	Messages
📖	@sentry/browser bundle gzip'ed minified size: (ES5: 17.0732 kB) (ES6: 16.1367 kB)
📖	✅ TSLint passed

Generated by 🚫 dangerJS against c98d924

packages/apm/src/integrations/tracing.ts

rhcarvalho · 2020-05-30T00:43:33Z

packages/apm/src/integrations/tracing.ts

+    // FIXME: depending on the 'op' directly is brittle.
+    if (transactionSpan.op === 'pageload') {


I didn't see an obvious quick fix for this brittleness.

The problem:

Any transaction could be called pageload, and that should not mean it should report the LCP;

The "pageload" transaction could be called something else, and we'd miss the LCP;

We could add additional things into the data property and check for that but tbh I think for now this is enough.

rhcarvalho · 2020-05-30T00:45:59Z

packages/apm/src/integrations/tracing.ts

+      forceLCP();
+      if (lcp) {
+        // Set the last observed LCP score.
+        transactionSpan.setData('_sentry_extra_metrics', JSON.stringify({ lcp }));


Sending the LCP as a data entry for now, can be changed.

rhcarvalho · 2020-05-30T00:56:48Z

packages/apm/src/integrations/tracing.ts

+        // Set the last observed LCP score.
+        transactionSpan.setData('_sentry_extra_metrics', JSON.stringify({ lcp }));
+      }
+    }


Note that the reference implementation sends the LCP when the page visibility changes to hidden. That happens when a user switches tabs, navigates away, etc.

Instead here, as per Daniel's suggestion, we send the current known LCP at the time of sending the "idle transaction".
Local exploratory tests suggest that it is easy to miss the proper LCP entry (either get nothing or get an earlier element).

We may discuss this behavior and decide whether we're happy with it for starters.

Two alternative ways forward are delaying the submission of the pageload transaction (wait for page visibility hidden event) or sending a separate transaction for LCP (in which case we would benefit from a being able to send a trimmed down payload, e.g. without breadcrumbs and other non-essential or redundant data).

Snippet from the reference implementation

HazAT

+ Changelog

I left a comment that this should move into the class, we otherwise also miss the option of adding an option to toggle this in the future.

packages/apm/src/integrations/tracing.ts

rhcarvalho · 2020-06-04T18:07:29Z

Changelog

Added.

Would be nice to automate this when generating the release. Doing this in every PR requires extra manual work and is an obvious spot for merge conflicts when multiple PRs are in-flight.

For the Go SDK I've documented a partial automation for the CHANGELOG generation:

https://github.com/getsentry/sentry-go/blob/master/CONTRIBUTING.md#release

$ git log --no-merges --format=%s $(git describe --abbrev=0).. | sed 's/^/- /'

rhcarvalho · 2020-06-04T18:17:10Z

packages/apm/src/integrations/tracing.ts

+          Tracing._lcp = {
+            // @ts-ignore
+            elementId: entry.id,
+            // @ts-ignore
+            elementSize: entry.size,
+            largestContentfulPaint: entry.startTime,
+          };


entry.id and entry.size are not always available.
We may or may not want more details from the LCP entry.
I've put those here as examples of what is possible.

https://developer.mozilla.org/en-US/docs/Web/API/LargestContentfulPaint

@HazAT @kamilogorek @dashed feel free to comment on this.

Also note that we're only sending a number like 63ms instead of a UNIX timestamp (like in spans).

I think entry.startTime would be relative to performance.timeOrigin.

Is it sufficient to infer the start timestamp relative to the transaction's start timestamp?

This is assuming that the transaction's start timestamp uses performance.timeOrigin in some fashion.

@dashed yes, yes, yes :)

Since the LCP metric is defined in terms of a page load, when looking at aggregation reporting we should probably look at it in isolation from transaction start timestamps.

For overlaying into a single pageload transaction view (a.k.a. vertical line), it should be okay-ish to consider it an offset from the transaction start timestamp.

rhcarvalho · 2020-06-04T18:18:05Z

packages/apm/src/integrations/tracing.ts

+      Tracing._forceLCP();
+      if (Tracing._lcp) {
+        // Set the last observed LCP score.
+        transactionSpan.setData('_sentry_extra_metrics', { lcp: Tracing._lcp });


@HazAT @kamilogorek @dashed PTAL

The idea here is to have a "namespace" where we can add other browser metrics later.

Using Transaction.data for now, but we can brainstorm how we want to do this going forward.

@rhcarvalho I'm open to have a meeting to talk more about this "namespace" to store these kinds of metrics.

packages/core/src/baseclient.ts

packages/apm/src/integrations/tracing.ts

HazAT

I changed my mind, let's not normalize contexts for now.

dashed

Looks good to me 👍

rhcarvalho · 2020-06-08T10:11:07Z

Note that this without a fix for #2646 requires manual change of normalizeDepth in Sentry.init, otherwise the page load transaction will look like

{"contexts":{"trace":{"data":{"_sentry_web_vitals":"[Object]"}...

rhcarvalho · 2020-06-08T11:44:30Z

Rebased on top of latest master and fixed conflict in CHANGELOG.md.

CHANGELOG.md

Largest Contentful Paint (LCP) is an user-centric metric for measuring perceived load speed. It marks the point in the page load timeline when the page's main content has likely loaded. Reference: https://web.dev/lcp/#measure-lcp-in-javascript

This allows us to optionally disable LCP based on config later.

This allows for proper serialization of objects as values in context.trace.data.

This reverts commit 19cdf22. This is too aggressive, would increase the side of breadcrumbs

All of the context types documented in [1] are flat objects. The new contexts.trace is the only key under contexts that may have nested objects under contexts.trace.data. That value holds the data for the Transaction Span and matches spans[].data for invidual Spans. At the moment one of the data fields is heavily limited, while the other is ignored by normalization. [1]: https://develop.sentry.dev/sdk/event-payloads/contexts/ This is the simplest to implement fix to #2646, thus what I'm doing here perhaps temporarily. Fixes #2646

This reverts commit 88c210c.

Tracing._lcp can be undefined if no LCP is ever reported.

The idea is to align with the name used in web-vitals (LCP in uppercase, not lcp) and to constrain the custom container in data to metrics related to Web Vitals. Do not double encode as JSON, assume that normalization is solved elsewhere.

Co-authored-by: Daniel Griesser <daniel.griesser.86@gmail.com>

rhcarvalho · 2020-06-08T12:12:27Z

Rebased on top of latest master and fixed conflict in CHANGELOG.md, again.

rhcarvalho · 2020-06-08T12:17:10Z

Going to ignore the test runs, @kamilogorek will do a release next so we can save some time re-running CI.

rhcarvalho added the Type: Feature label May 28, 2020

rhcarvalho marked this pull request as draft May 28, 2020 13:08

dashed self-requested a review May 28, 2020 13:17

rhcarvalho force-pushed the rhcarvalho/lcp branch from cfacd9e to 945d14e Compare May 30, 2020 00:28

rhcarvalho commented May 30, 2020

View reviewed changes

rhcarvalho marked this pull request as ready for review May 30, 2020 00:57

rhcarvalho requested a review from kamilogorek as a code owner May 30, 2020 00:57

rhcarvalho requested a review from HazAT May 30, 2020 00:57

HazAT requested changes Jun 2, 2020

View reviewed changes

packages/apm/src/integrations/tracing.ts Outdated Show resolved Hide resolved

packages/apm/src/integrations/tracing.ts Outdated Show resolved Hide resolved

rhcarvalho force-pushed the rhcarvalho/lcp branch from 945d14e to 269567f Compare June 4, 2020 17:56

rhcarvalho commented Jun 4, 2020

View reviewed changes

rhcarvalho requested a review from HazAT June 4, 2020 18:21

HazAT requested changes Jun 5, 2020

View reviewed changes

packages/core/src/baseclient.ts Outdated Show resolved Hide resolved

dashed reviewed Jun 5, 2020

View reviewed changes

packages/apm/src/integrations/tracing.ts Show resolved Hide resolved

dashed reviewed Jun 5, 2020

View reviewed changes

packages/apm/src/integrations/tracing.ts Outdated Show resolved Hide resolved

HazAT approved these changes Jun 5, 2020

View reviewed changes

rhcarvalho marked this pull request as draft June 5, 2020 15:54

rhcarvalho force-pushed the rhcarvalho/lcp branch from bc7d5d5 to 3c88853 Compare June 5, 2020 16:18

rhcarvalho mentioned this pull request Jun 5, 2020

feat: Report Web Vitals metrics on pageload transactions #2651

Closed

dashed approved these changes Jun 5, 2020

View reviewed changes

rhcarvalho force-pushed the rhcarvalho/lcp branch from 1f0599f to cb14f1c Compare June 8, 2020 11:43

rhcarvalho marked this pull request as ready for review June 8, 2020 11:44

HazAT approved these changes Jun 8, 2020

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

kamilogorek approved these changes Jun 8, 2020

View reviewed changes

rhcarvalho added 2 commits June 8, 2020 14:10

ref: Move LCP tracking to static side of Tracing

91948bb

This allows us to optionally disable LCP based on config later.

rhcarvalho and others added 9 commits June 8, 2020 14:10

feat: Increase default normalizeDepth

64ab997

This allows for proper serialization of objects as values in context.trace.data.

Revert "feat: Increase default normalizeDepth"

ec371db

This reverts commit 19cdf22. This is too aggressive, would increase the side of breadcrumbs

misc: CHANGELOG

eace0c2

Revert "feat: Remove normalization of contexts"

9d114cc

This reverts commit 88c210c.

misc: Update type definition

66d2329

Tracing._lcp can be undefined if no LCP is ever reported.

misc: Rename keys leading up to LCP metric

3f9e921

The idea is to align with the name used in web-vitals (LCP in uppercase, not lcp) and to constrain the custom container in data to metrics related to Web Vitals. Do not double encode as JSON, assume that normalization is solved elsewhere.

fix: Include elementId and elementSize only when available

0fd278d

Update CHANGELOG.md

c98d924

Co-authored-by: Daniel Griesser <daniel.griesser.86@gmail.com>

rhcarvalho force-pushed the rhcarvalho/lcp branch from ce9747f to c98d924 Compare June 8, 2020 12:11

rhcarvalho merged commit 15415c1 into master Jun 8, 2020

rhcarvalho deleted the rhcarvalho/lcp branch June 8, 2020 12:18

dashed mentioned this pull request Sep 16, 2020

feat(rum): Add measurements support and web vitals #2909

Merged

4 tasks

robinglen mentioned this pull request Oct 8, 2020

Any Analysis Service?(like sentry) GoogleChrome/web-vitals#26

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Report LCP metric on pageload transactions #2624

feat: Report LCP metric on pageload transactions #2624

rhcarvalho commented May 28, 2020 •

edited

Loading

getsentry-bot commented May 28, 2020 •

edited

Loading

rhcarvalho May 30, 2020

HazAT Jun 2, 2020

rhcarvalho May 30, 2020

rhcarvalho May 30, 2020

HazAT left a comment

rhcarvalho commented Jun 4, 2020

rhcarvalho Jun 4, 2020

dashed Jun 5, 2020

rhcarvalho Jun 5, 2020

rhcarvalho Jun 4, 2020

dashed Jun 4, 2020

HazAT left a comment

dashed left a comment

rhcarvalho commented Jun 8, 2020

rhcarvalho commented Jun 8, 2020

rhcarvalho commented Jun 8, 2020

rhcarvalho commented Jun 8, 2020

		// FIXME: depending on the 'op' directly is brittle.
		if (transactionSpan.op === 'pageload') {

feat: Report LCP metric on pageload transactions #2624

feat: Report LCP metric on pageload transactions #2624

Conversation

rhcarvalho commented May 28, 2020 • edited Loading

getsentry-bot commented May 28, 2020 • edited Loading

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

HazAT left a comment

Choose a reason for hiding this comment

rhcarvalho commented Jun 4, 2020

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

HazAT left a comment

Choose a reason for hiding this comment

dashed left a comment

Choose a reason for hiding this comment

rhcarvalho commented Jun 8, 2020

rhcarvalho commented Jun 8, 2020

rhcarvalho commented Jun 8, 2020

rhcarvalho commented Jun 8, 2020

rhcarvalho commented May 28, 2020 •

edited

Loading

getsentry-bot commented May 28, 2020 •

edited

Loading