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(tracing): Change resource span op name and add data #2816

Merged
merged 7 commits into from
Aug 19, 2020

Conversation

AbhiPrasad
Copy link
Member

@AbhiPrasad AbhiPrasad commented Aug 13, 2020

Add transferSize, encodedBodySize, and decodedBodySize metrics to resource spans and adjust resource op name to account for initiatorType (ex. resource.css, resource.img)

This ones for 🇨🇦

image

image

@github-actions
Copy link
Contributor

github-actions bot commented Aug 13, 2020

size-limit report

Path Size
@sentry/browser - CDN Bundle (gzipped) 17.67 KB (0%)
@sentry/browser - Webpack 18.46 KB (0%)
@sentry/react - Webpack 18.46 KB (0%)
@sentry/browser + @sentry/tracing - CDN Bundle (gzipped) 22.69 KB (0%)

@AbhiPrasad AbhiPrasad force-pushed the abhi/performance-entry branch 3 times, most recently from b8a4d95 to 9881251 Compare August 13, 2020 20:37
if (entry.decodedBodySize) {
data.decodedBodySize = entry.decodedBodySize;
}

Copy link
Member

@k-fish k-fish Aug 13, 2020

Choose a reason for hiding this comment

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

We also need a timing (such as responseStart) to exclude cross-origin resources that aren't setup to allow timing.

Copy link
Member

@k-fish k-fish Aug 13, 2020

Choose a reason for hiding this comment

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

We also could include some sort of comparison (eg. entry.name.startsWith(window.location.origin)) to determine whether the resource is cross-origin explicitly.

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 will leave this for the future, merging this in for now.

Opening a ticket.

Copy link
Member

@dashed dashed left a comment

Choose a reason for hiding this comment

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

Looks good to me 👍

@AbhiPrasad AbhiPrasad force-pushed the abhi/performance-entry branch from 9881251 to a68731c Compare August 14, 2020 15:28
@getsentry getsentry deleted a comment from getsentry-bot Aug 14, 2020
@AbhiPrasad AbhiPrasad force-pushed the abhi/performance-entry branch from a68731c to 8bb9c42 Compare August 18, 2020 13:23

Verified

This commit was signed with the committer’s verified signature.
thevilledev Ville Vesilehto

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
@AbhiPrasad AbhiPrasad force-pushed the abhi/performance-entry branch from 8bb9c42 to 49a5d6e Compare August 18, 2020 13:24
@@ -226,14 +234,26 @@ function addResourceSpans(
return undefined;
}

const data: Record<string, any> = {};
if ('transferSize' in entry) {
data.transferSize = entry.transferSize;
Copy link
Contributor

Choose a reason for hiding this comment

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

Given that arbitrary data fields are not special-cased in the UI to make them read nice, would it be desirable here to use a "pretty" format like data["Transfer Size"]? cc @dashed

Specifically:

image

Copy link
Member

Choose a reason for hiding this comment

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

Yep that should work fine.

@AbhiPrasad AbhiPrasad merged commit ef6396a into master Aug 19, 2020
@AbhiPrasad AbhiPrasad deleted the abhi/performance-entry branch August 19, 2020 13:28
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.

None yet

6 participants