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.

Sign up for GitHub

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

Jump to bottom

feat(tracing): Add GB unit to device memory tag value #4935

Merged
merged 1 commit into from
Apr 14, 2022
Merged

feat(tracing): Add GB unit to device memory tag value #4935

merged 1 commit into from
Apr 14, 2022

Conversation

dashed
Copy link
Member

@dashed dashed commented Apr 13, 2022 •
edited
Loading

Since tag values are always strings, it'll be useful to attach units to the deviceMemory tag. Since navigator.deviceMemory are approximate in gigabytes, we can suffix the tag value with " GB".

When seeing these values in Sentry product, it's non-obvious that they're in gigabytes:

Screen Shot 2022-04-13 at 3 35 41 PM

Reference: https://developer.mozilla.org/en-US/docs/Web/API/Navigator/deviceMemory

@dashed dashed requested review from doralchan and a team April 13, 2022 19:32
@dashed dashed self-assigned this Apr 13, 2022
@dashed dashed requested review from lobsterkatie and AbhiPrasad and removed request for a team April 13, 2022 19:32
@dashed
Copy link
Member Author

dashed commented Apr 13, 2022

@lobsterkatie @AbhiPrasad Unsure if this is something we could add to v7 release 👀

@github-actions
Copy link
Contributor

github-actions bot commented Apr 13, 2022 •
edited
Loading

size-limit report 📦

Path Size
@sentry/browser - ES5 CDN Bundle (gzipped + minified) 20.2 KB (+0.28% 🔺)
@sentry/browser - ES5 CDN Bundle (minified) 64.33 KB (-0.44% 🔽)
@sentry/browser - ES6 CDN Bundle (gzipped + minified) 18.84 KB (-0.09% 🔽)
@sentry/browser - ES6 CDN Bundle (minified) 57.94 KB (-0.06% 🔽)
@sentry/browser - Webpack (gzipped + minified) 23.41 KB (+0.76% 🔺)
@sentry/browser - Webpack (minified) 81.21 KB (-0.63% 🔽)
@sentry/react - Webpack (gzipped + minified) 23.46 KB (+0.79% 🔺)
@sentry/nextjs Client - Webpack (gzipped + minified) 47.96 KB (-0.19% 🔽)
@sentry/browser + @sentry/tracing - ES5 CDN Bundle (gzipped + minified) 26.04 KB (-0.12% 🔽)
@sentry/browser + @sentry/tracing - ES6 CDN Bundle (gzipped + minified) 24.42 KB (-0.27% 🔽)

AbhiPrasad
AbhiPrasad reviewed Apr 14, 2022
View reviewed changes
Copy link
Member

@AbhiPrasad AbhiPrasad left a comment

Choose a reason for hiding this comment

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

Is this something that would fit better as a custom measurement? I guess we don't index measurements in the same way as tags though (at least not yet).

As per #4882, we are targeting it so that all changes get merged into 7.x branch. We can merge this in for v7!

@dashed
Copy link
Member Author

dashed commented Apr 14, 2022

Is this something that would fit better as a custom measurement? I guess we don't index measurements in the same way as tags though (at least not yet).

I intentionally set this as a tag as it's easier to do with minimal complexity.

But it might be transitioned once metrics product is more fleshed out.

As per #4882, we are targeting it so that all changes get merged into 7.x branch. We can merge this in for v7!

I'll see if I can rebase this PR to 7.x branch in #4876. Would that be preferable?

@AbhiPrasad
Copy link
Member

I'll see if I can rebase this PR to 7.x branch in #4876. Would that be preferable?

Yes please, that would be great!

@dashed dashed changed the base branch from master to 7.x April 14, 2022 15:51
@dashed dashed force-pushed the add-gb-units-to-device-memory branch from f90ba85 to aa91d09 Compare April 14, 2022 15:51
@dashed
Copy link
Member Author

dashed commented Apr 14, 2022

@AbhiPrasad I've rebased it against 7.x branch. 👍

AbhiPrasad
AbhiPrasad approved these changes Apr 14, 2022
View reviewed changes
Copy link
Member

@AbhiPrasad AbhiPrasad left a comment

Choose a reason for hiding this comment

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

Thank you for your contribution!

@AbhiPrasad AbhiPrasad changed the title feat: Add GB unit to device memory tag value feat(tracing): Add GB unit to device memory tag value Apr 14, 2022
@AbhiPrasad AbhiPrasad merged commit f67b1c2 into 7.x Apr 14, 2022
@AbhiPrasad AbhiPrasad deleted the add-gb-units-to-device-memory branch April 14, 2022 16:41
@dashed
Copy link
Member Author

dashed commented Apr 18, 2022

@AbhiPrasad thanks! Is there a tentative timeline for the v7 release? Just something I can communicate back to the team for this fix.

@AbhiPrasad
Copy link
Member

First betas are our end of Q1 - then 2-3 weeks of testing until full release prob.

@AbhiPrasad AbhiPrasad added this to the 7.0.0 milestone Apr 25, 2022
Lms24 pushed a commit that referenced this pull request Apr 26, 2022
@dashed @Lms24
feat(tracing): Add GB unit to device memory tag value (#4935)
704a093 
Since tag values are always strings, it'll be useful to attach units to the `deviceMemory` tag. Since `navigator.deviceMemory` are approximate in gigabytes, we can suffix the tag value with " GB".

Reference: https://developer.mozilla.org/en-US/docs/Web/API/Navigator/deviceMemory
lobsterkatie pushed a commit that referenced this pull request Apr 26, 2022
@dashed @lobsterkatie
feat(tracing): Add GB unit to device memory tag value (#4935)
57c7c32 
Since tag values are always strings, it'll be useful to attach units to the `deviceMemory` tag. Since `navigator.deviceMemory` are approximate in gigabytes, we can suffix the tag value with " GB".

Reference: https://developer.mozilla.org/en-US/docs/Web/API/Navigator/deviceMemory
lobsterkatie pushed a commit that referenced this pull request Apr 26, 2022
@dashed @lobsterkatie
feat(tracing): Add GB unit to device memory tag value (#4935)
1660047 
Since tag values are always strings, it'll be useful to attach units to the `deviceMemory` tag. Since `navigator.deviceMemory` are approximate in gigabytes, we can suffix the tag value with " GB".

Reference: https://developer.mozilla.org/en-US/docs/Web/API/Navigator/deviceMemory
AbhiPrasad pushed a commit that referenced this pull request May 30, 2022
@dashed @AbhiPrasad
feat(tracing): Add GB unit to device memory tag value (#4935)
0693ab1 
Since tag values are always strings, it'll be useful to attach units to the `deviceMemory` tag. Since `navigator.deviceMemory` are approximate in gigabytes, we can suffix the tag value with " GB".

Reference: https://developer.mozilla.org/en-US/docs/Web/API/Navigator/deviceMemory
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AbhiPrasad AbhiPrasad AbhiPrasad approved these changes

@doralchan doralchan Awaiting requested review from doralchan

@lobsterkatie lobsterkatie Awaiting requested review from lobsterkatie lobsterkatie was automatically assigned from getsentry/team-web-sdk-frontend

Assignees

@dashed dashed

Labels
None yet
Projects
None yet
Milestone
7.0.0
Development

Successfully merging this pull request may close these issues.
2 participants
@dashed @AbhiPrasad