Skip to content

Add memory usage to contexts#2133

Merged
denrase merged 19 commits into
mainfrom
feat/memory-usage
Jul 9, 2024
Merged

Add memory usage to contexts#2133
denrase merged 19 commits into
mainfrom
feat/memory-usage

Conversation

@denrase

@denrase denrase commented Jun 25, 2024

Copy link
Copy Markdown
Collaborator

📜 Description

Add memory usage to contexts

💡 Motivation and Context

Closes #1826

💚 How did you test it?

Sample app

📝 Checklist

  • I reviewed submitted code
  • I added tests to verify changes
  • No new PII added or SDK only sends newly added PII if sendDefaultPii is enabled
  • I updated the docs if needed
  • All tests passing
  • No breaking changes

@github-actions

github-actions Bot commented Jun 25, 2024

Copy link
Copy Markdown
Contributor

iOS Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1240.94 ms 1263.18 ms 22.24 ms
Size 8.33 MiB 9.63 MiB 1.29 MiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
e3ef570 1220.08 ms 1239.66 ms 19.58 ms
1b66358 1260.81 ms 1282.08 ms 21.27 ms
64af39c 1243.98 ms 1257.69 ms 13.71 ms
f275487 1291.65 ms 1339.92 ms 48.26 ms
2d3b03d 1258.19 ms 1272.69 ms 14.50 ms
bdd1a23 1225.78 ms 1235.82 ms 10.04 ms
408bdab 1239.63 ms 1257.61 ms 17.98 ms
0db91cc 1267.63 ms 1279.69 ms 12.06 ms
4ad2751 1247.67 ms 1249.20 ms 1.53 ms
11fb408 1256.14 ms 1283.51 ms 27.37 ms

App size

Revision Plain With Sentry Diff
e3ef570 8.32 MiB 9.38 MiB 1.06 MiB
1b66358 8.10 MiB 9.08 MiB 1004.35 KiB
64af39c 8.28 MiB 9.34 MiB 1.06 MiB
f275487 8.32 MiB 9.38 MiB 1.05 MiB
2d3b03d 8.10 MiB 9.07 MiB 1000.83 KiB
bdd1a23 8.29 MiB 9.39 MiB 1.10 MiB
408bdab 8.28 MiB 9.34 MiB 1.06 MiB
0db91cc 8.15 MiB 9.15 MiB 1018.56 KiB
4ad2751 8.29 MiB 9.39 MiB 1.10 MiB
11fb408 8.10 MiB 9.08 MiB 1004.36 KiB

Previous results on branch: feat/memory-usage

Startup times

Revision Plain With Sentry Diff
e8d8880 1244.51 ms 1264.81 ms 20.30 ms
ce54ed1 1222.17 ms 1248.96 ms 26.79 ms
1ab1f7e 1244.94 ms 1263.58 ms 18.65 ms
cf5df45 1245.84 ms 1262.33 ms 16.50 ms
fcbcade 1234.31 ms 1253.57 ms 19.26 ms

App size

Revision Plain With Sentry Diff
e8d8880 8.33 MiB 9.63 MiB 1.30 MiB
ce54ed1 8.33 MiB 9.62 MiB 1.29 MiB
1ab1f7e 8.33 MiB 9.62 MiB 1.29 MiB
cf5df45 8.33 MiB 9.63 MiB 1.29 MiB
fcbcade 8.33 MiB 9.63 MiB 1.30 MiB

@github-actions

github-actions Bot commented Jun 25, 2024

Copy link
Copy Markdown
Contributor
Messages
📖 Do not forget to update Sentry-docs with your feature once the pull request gets approved.

Generated by 🚫 dangerJS against c1d87f6

@codecov

codecov Bot commented Jun 25, 2024

Copy link
Copy Markdown

Codecov Report

Attention: Patch coverage is 67.85714% with 18 lines in your changes missing coverage. Please review.

Project coverage is 88.30%. Comparing base (f172c4d) to head (c1d87f6).

Files Patch % Lines
...c/event_processor/enricher/io_platform_memory.dart 61.70% 18 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2133      +/-   ##
==========================================
+ Coverage   86.46%   88.30%   +1.84%     
==========================================
  Files         166      225      +59     
  Lines        5850     7773    +1923     
==========================================
+ Hits         5058     6864    +1806     
- Misses        792      909     +117     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@denrase
denrase marked this pull request as ready for review June 25, 2024 14:33
@buenaflor

Copy link
Copy Markdown
Contributor

We could use these fields if they make sense.

memory_size
Optional. Total system memory available in bytes.

free_memory
Optional. Free system memory in bytes.

usable_memory
Optional. Memory usable for the app in bytes.

see https://develop.sentry.dev/sdk/event-payloads/contexts/

@buenaflor

Copy link
Copy Markdown
Contributor

app_memory might also be useful

app_memory
Optional. Amount of memory used by the application in bytes.

depending on the meaning of app_memory both current and max rss could fit here. we'd need more clarification for that

@buenaflor

buenaflor commented Jun 25, 2024

Copy link
Copy Markdown
Contributor

Okay it's supposed to be the memory at the time of the event so currentRss should fit here

@buenaflor buenaflor left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

left some comments,

I'm not sure though how we should categorize maxRss

Comment thread CHANGELOG.md Outdated
Comment on lines +45 to +46
_bytesToHumanReadableFileSize(ProcessInfo.currentRss),
'maxResidentSetSize': _bytesToHumanReadableFileSize(ProcessInfo.maxRss),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

are there any alternatives for web?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

this is based on this https://developer.mozilla.org/en-US/docs/Web/API/Performance/memory

it's deprecated and not all browsers support it, dunno if it's worth the effort to add this if we have to remove it again at some point

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, i removed it after reading the docs and added some info.


contexts['dart_context'] = _getDartContext();
contexts['process_info'] = <String, dynamic>{
'currentResidentSetSize':

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

let's add currentRss to app_memory

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

How does currentRss compare to what the native side sets? Does it override what the native side sets?

@denrase denrase Jul 1, 2024

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Updated the code and we only set these values if there are no native SDKs

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I don't think android sets it, I know iOS for sure does @denrase

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Updated so we set appMemory to ProcessInfo.currentRss, it it's not already set, regardless of native integration. The other values are merged, so this is additional information.

On Linux/Windows we now also add free and total system memory, if possible.

@denrase

denrase commented Jul 1, 2024

Copy link
Copy Markdown
Collaborator Author

@buenaflor Used the mapping below, as these seemed the most appropriate accodring to the documentation. WDYT?

Used

  • app_memory -> currentRss
    Optional. Amount of memory used by the application in bytes.

  • usable_memory -> maxRss
    Optional. Memory usable for the app in bytes.

Values below are not used, as we can't say anything about the system memory as far as I can see.

  • memory_size
    Optional. Total system memory available in bytes.

  • free_memory
    Optional. Free system memory in bytes.

@denrase

denrase commented Jul 1, 2024

Copy link
Copy Markdown
Collaborator Author

There's a package to get more system info we could look into: https://pub.dev/packages/system_info2

@denrase

denrase commented Jul 1, 2024

Copy link
Copy Markdown
Collaborator Author

@buenaflor

Copy link
Copy Markdown
Contributor

usable_memory -> maxRss
Optional. Memory usable for the app in bytes.

hm not sure if this completely fits since maxRss captures the highest amount used thus far

@buenaflor

Copy link
Copy Markdown
Contributor

There's a package to get more system info we could look into: https://pub.dev/packages/system_info2

can those commands be run in production?

Comment thread dart/lib/src/event_processor/enricher/io_platform_memory.dart Outdated
@github-actions

github-actions Bot commented Jul 1, 2024

Copy link
Copy Markdown
Contributor

Android Performance metrics 🚀

  Plain With Sentry Diff
Startup time 390.06 ms 458.00 ms 67.94 ms
Size 6.35 MiB 7.35 MiB 1019.02 KiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
d189e01 328.67 ms 397.12 ms 68.45 ms
7f75f32 347.36 ms 419.58 ms 72.22 ms
457a85b 312.37 ms 376.67 ms 64.31 ms
2331d89 352.45 ms 417.34 ms 64.89 ms
0118295 365.71 ms 438.56 ms 72.85 ms
eb1a7c1 332.98 ms 381.55 ms 48.57 ms
ef2f368 350.06 ms 429.44 ms 79.38 ms
e5b744f 302.70 ms 342.17 ms 39.47 ms
c73ab67 353.82 ms 408.71 ms 54.90 ms
4477d2e 392.75 ms 472.69 ms 79.94 ms

App size

Revision Plain With Sentry Diff
d189e01 6.16 MiB 7.14 MiB 1009.90 KiB
7f75f32 6.26 MiB 7.20 MiB 959.18 KiB
457a85b 6.06 MiB 7.09 MiB 1.03 MiB
2331d89 5.94 MiB 6.96 MiB 1.02 MiB
0118295 6.33 MiB 7.26 MiB 947.07 KiB
eb1a7c1 5.94 MiB 6.92 MiB 1005.76 KiB
ef2f368 5.94 MiB 6.89 MiB 975.81 KiB
e5b744f 6.06 MiB 7.09 MiB 1.03 MiB
c73ab67 6.15 MiB 7.13 MiB 999.97 KiB
4477d2e 6.33 MiB 7.26 MiB 950.38 KiB

Previous results on branch: feat/memory-usage

Startup times

Revision Plain With Sentry Diff
e8d8880 427.13 ms 496.06 ms 68.93 ms
cf5df45 405.76 ms 472.82 ms 67.06 ms
ce54ed1 416.06 ms 507.80 ms 91.74 ms
1ab1f7e 387.86 ms 451.34 ms 63.48 ms
fcbcade 407.13 ms 482.32 ms 75.19 ms

App size

Revision Plain With Sentry Diff
e8d8880 6.35 MiB 7.35 MiB 1.00 MiB
cf5df45 6.35 MiB 7.35 MiB 1018.79 KiB
ce54ed1 6.35 MiB 7.35 MiB 1019.03 KiB
1ab1f7e 6.35 MiB 7.35 MiB 1019.02 KiB
fcbcade 6.35 MiB 7.35 MiB 1.00 MiB

@buenaflor buenaflor left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

left some comments, regarding maxRss we can also name it ourselves, not sure if usable_memory is the best fit. wdyt?

Comment thread dart/lib/src/event_processor/enricher/io_platform_memory.dart Outdated
Comment thread CHANGELOG.md Outdated
@denrase

denrase commented Jul 8, 2024

Copy link
Copy Markdown
Collaborator Author

can those commands be run in production?

Testes in debug in am Windows VM and tests pass so far.

@denrase
denrase requested review from buenaflor and ueman July 8, 2024 13:54

@buenaflor buenaflor left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

one last fix pls

Comment thread CHANGELOG.md Outdated
@denrase
denrase requested a review from buenaflor July 8, 2024 14:19
@denrase
denrase merged commit 9f9dd52 into main Jul 9, 2024
@denrase
denrase deleted the feat/memory-usage branch July 9, 2024 08:34
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.

Report used memory/max used memory

3 participants