Skip to content
This repository was archived by the owner on Aug 14, 2024. It is now read-only.

Add app_memory to app context #658

Merged
merged 2 commits into from
Aug 9, 2022
Merged

Add app_memory to app context #658

merged 2 commits into from
Aug 9, 2022

Conversation

timfish
Copy link
Contributor

@timfish timfish commented Aug 2, 2022

Currently the device context has properties for the device memory usage but it can be useful to know how much memory an app was using when an event or transaction was captured.

Open to better naming than memory_used 🤔

Related issues:
getsentry/sentry-cocoa#999

@vercel
Copy link

vercel bot commented Aug 2, 2022

@timfish is attempting to deploy a commit to the Sentry Team on Vercel.

A member of the Team first needs to authorize it.

@bruno-garcia
Copy link
Member

bruno-garcia commented Aug 2, 2022

@timfish
Copy link
Contributor Author

timfish commented Aug 2, 2022

Might also be worth including storage_used to help with this:
getsentry/team-mobile#8

@bruno-garcia
Copy link
Member

Dotnet sdk has "Memory Info", i believe the
units are not well defined. So the ui doesn't present KB/MB etc, just shows as-is

@AbhiPrasad
Copy link
Member

I'm +1 on adding this, and I think memory_used effectively articulates what it's doing.

If we are worried about units not being well defined, we could also just include the unit as part of the value.
For example, "memory_used": "30 kB". Thoughts on that? We are probably not going to index this - so I don't think we need to worry about high cardinality concerns.

@timfish
Copy link
Contributor Author

timfish commented Aug 2, 2022

Everything in contexts is in bytes apart from gpu.memory_size which is in Megabytes.

@marandaneto
Copy link
Contributor

marandaneto commented Aug 2, 2022

Android SDK already reports contexts.device.memory_size, contexts.device.free_memory and contexts.device. usable_memory, all in bytes.
IIRC, if we add fields to the spec, we need validation on relay first.
Since the contexts accept any unknown value, you could technically just send it unless you want to standardize across SDKs.
Ideally, we'd also customize the UI when showing the field from the raw key such as free_memory to a more human-readable version such as Free Memory or something similar, that would be on https://github.com/getsentry/sentry/blob/53fe8ba3447b010e75932175a0ebf9e9edad3b6a/static/app/components/events/contexts/device/getDeviceKnownDataDetails.tsx#L87

@timfish
Copy link
Contributor Author

timfish commented Aug 2, 2022

There are a large number of properties documented in contexts.device that are not specifically handled in Relay or Sentry right now such as boot_time, processor_count, processor_frequency and all the supports_*.

@AbhiPrasad
Copy link
Member

@AbhiPrasad
Copy link
Member

We can open up a GH issue on the Sentry repo to add memory_used to the UI after we merge the spec in!

@brustolin
Copy link
Contributor

brustolin commented Aug 3, 2022

I want to suggest to keep pattern and call it "app_memory"

Co-authored-by: Dhiogo Brustolin <[email protected]>
@timfish timfish changed the title Add memory_used to app context Add app_memory to app context Aug 4, 2022
@brustolin
Copy link
Contributor

@AbhiPrasad, @bruno-garcia, What else do we need to merge this?

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.

@AbhiPrasad
Copy link
Member

I'm going to go ahead and merge this! I'll take care of updating relay.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants