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

feat(web-vitals): Capture information about the LCP element culprit #3427

Merged
merged 5 commits into from
Apr 22, 2021

Conversation

dashed
Copy link
Member

@dashed dashed commented Apr 20, 2021

Split from #3012

For LCP (largest contentful paint), we'd like to know the element (or image) that contributed the largest contentful paint:

The properties we capture for the LCP culprit are:

  • path to the element/image
  • id attribute
  • if it is an image, we capture its URL
  • intrinsic size

We capture these properties as tags.

Screen Shot 2021-04-20 at 5 10 43 PM

We aim to use these properties for the tag/segment explorer on the Performance product.

@dashed dashed requested a review from a team April 20, 2021 21:19
@dashed dashed self-assigned this Apr 20, 2021
@dashed dashed requested a review from kamilogorek as a code owner April 20, 2021 21:19
@dashed dashed requested a review from a team April 20, 2021 21:20
Copy link
Member

@k-fish k-fish left a comment

Choose a reason for hiding this comment

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

Looks fine code-wise although obviously someone from sdk should probably give it a review too. I'd say the minimum we need is lcp.element for now, but if the others are cheap to add that's fine too.

packages/tracing/src/browser/metrics.ts Show resolved Hide resolved
@github-actions
Copy link
Contributor

github-actions bot commented Apr 20, 2021

size-limit report

Path Size
@sentry/browser - CDN Bundle (gzipped) 20.61 KB (0%)
@sentry/browser - Webpack 21.49 KB (0%)
@sentry/react - Webpack 21.53 KB (0%)
@sentry/browser + @sentry/tracing - CDN Bundle (gzipped) 27.91 KB (+0.3% 🔺)

Copy link
Contributor

@rhcarvalho rhcarvalho left a comment

Choose a reason for hiding this comment

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

Good to have this back!

}

if (this._lcpEntry.url) {
transaction.setTag('lcp.url', this._lcpEntry.url);
Copy link
Contributor

Choose a reason for hiding this comment

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

I thought one curious case to consider is what if the image is not coming from a remote URL but instead inlined with a data URL, like

<img src="data:image/gif;base64,R0lGODlhEAAOALMAAOazToeHh0tLS/7LZv/0jvb29t/f3//Ub/
/ge8WSLf/rhf/3kdbW1mxsbP//mf///yH5BAAAAAAALAAAAAAQAA4AAARe8L1Ekyky67QZ1hLnjM5UUde0ECwLJoExKcpp
V0aCcGCmTIHEIUEqjgaORCMxIC6e0CcguWw6aFjsVMkkIr7g77ZKPJjPZqIyd7sJAgVGoEGv2xsBxqNgYPj/gAwXEQA7" 
width="16" height="14" alt="embedded folder icon">

embedded folder icon

Would we still want to include that as a tag? Definitely a corner case, but the image could be large enough that would cause the event/transaction to be dropped in Relay.

(I don't know if the browser performance API would ever report such URLs)

Copy link
Member Author

Choose a reason for hiding this comment

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

I think we should include it. Ultimately the browser would determine what element contributes the LCP. IMO, if a data URL was marked as an LCP, then that might be a good signal that the page is performant. I think @k-fish (Kevan) can comment further.

Copy link
Member

Choose a reason for hiding this comment

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

I don't know that it would provide a ton of value, except in cases where the url is necessary to discern between img tags, but the element tree should be sufficient in most cases to determine the exact lcp element. That being said, all we have to do to avoid having the transaction rejected is truncate to the relay tag value limit of 200 chars anyway.

Copy link
Contributor

Choose a reason for hiding this comment

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

I tried this out. Inline images using a data: URI report the full URL in the Performance Entry. We don't want to send that, in particular not if it is too big.

image

We could special-case data: and trim-off the contents (just indicate this was a data URI), since truncating won't be useful (cannot render an image from a truncated URI and the base64 blob makes no sense by itself other than string matching to something in the user's code base, but for that we'd have better tools like element path and/or id).

Copy link
Member Author

Choose a reason for hiding this comment

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

@rhcarvalho Oh I see. I had forgotten that data URIs can be quite big. I can trim it to first 200 characters as suggested by @k-fish .

Copy link
Member Author

Choose a reason for hiding this comment

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

@rhcarvalho should be addressed in c513a29

id: string;
url: string;
element?: Element;
toJSON(): Record<string, unknown>;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we use unknown here?

Copy link
Contributor

Choose a reason for hiding this comment

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

I missed that. toJSON cannot shouldnt do anything else than Record<string, string>

Copy link
Member Author

Choose a reason for hiding this comment

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

I updated this in 5f42677

@dashed dashed force-pushed the tag-lcp-elements branch from 72aa247 to 5f42677 Compare April 21, 2021 16:45
@dashed dashed requested a review from rhcarvalho April 21, 2021 16:51

if (this._lcpEntry.url) {
// Trim URL to the first 200 characters.
transaction.setTag('lcp.url', this._lcpEntry.url.trim().slice(0, 200));
Copy link
Contributor

Choose a reason for hiding this comment

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

@dashed do you think it will be important to disambiguate in Sentry when the attribute was truncated or not?

Copy link
Member Author

Choose a reason for hiding this comment

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

@k-fish any thoughts on this?

Copy link
Member

Choose a reason for hiding this comment

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

The only way to clearly disambiguate it would be to set another tag when it was truncated, I feel like this is overkill considering we're not sure this is even necessary. I'd wait on adding any additional data until we see a clear case for it in the product.

Copy link
Member Author

@dashed dashed Apr 22, 2021

Choose a reason for hiding this comment

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

@rhcarvalho @k-fish I'll merge this in as is. Once we surface this on the product on the segment explorer, it'll be obvious if there's a need to disambiguate it. And if there is, we can follow up with another PR.

@dashed dashed merged commit 259ec68 into master Apr 22, 2021
@dashed dashed deleted the tag-lcp-elements branch April 22, 2021 19:15
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