fix: read free_memory when the event is captured, not only at SDK startup #1962

kevinrenskers · 2022-07-12T11:18:15Z

📜 Description

When we capture an event, we now enhance the data with the actual free memory at that time. Until now we only used the value from when the SDK was initiated.

💡 Motivation and Context

Fixes #1824

💚 How did you test it?

Unit test

📝 Checklist

I reviewed the submitted code
I added tests to verify the changes
I updated the docs if needed
Review from the native team if needed
No breaking changes

🔮 Next steps

…rtup Fixes #1824

kevinrenskers · 2022-07-12T11:19:52Z

I think I understood the issue and the comment in the GH issue correctly, and made the changes as you see here. However, I'm getting SentryCrashIntegrationTests failures when running the entire Sentry testsuite, but they pass correctly when running only the SentryCrashIntegrationTests tests. Could someone take a look, see if I'm on the right track, and maybe have an idea what's going on with the tests?

Sources/Sentry/SentryClient.m

Co-authored-by: Dhiogo Brustolin <dhiogorb@gmail.com>

* master: fix: Crash in profiling logger (#1964) ref: Mark options.sdkInfo as deprecated (#1960) feat: Add extra app start span (#1952)

philipphofmann

Some important notes.

Sources/Sentry/SentryClient.m

philipphofmann · 2022-07-13T08:06:53Z

Sources/Sentry/SentryClient.m

+    NSMutableDictionary *device =
+        [[NSMutableDictionary alloc] initWithDictionary:context[@"device"]];
+
+    NSDictionary *systemInfo = [[SentryCrashWrapper sharedInstance] systemInfo];


h: The freeMemory in systemInfo is cached. If you log the freeMemory like this

[SentryLog logWithMessage:[NSString stringWithFormat:@"FreeMemory %@", systemInfo[@"freeMemory"]] andLevel:kSentryLevelInfo];

you see that it doesn't change when you capture events.

SentryCrash uses this methods to get the free and usable memory

sentry-cocoa/Sources/SentryCrash/Recording/Monitors/SentryCrashMonitor_System.m

Lines 191 to 213 in ce1b8d1

static uint64_t

freeMemory(void)

{

vm_statistics_data_t vmStats;

vm_size_t pageSize;

if (VMStats(&vmStats, &pageSize)) {

return ((uint64_t)pageSize) * vmStats.free_count;

}

return 0;

}

static uint64_t

usableMemory(void)

{

vm_statistics_data_t vmStats;

vm_size_t pageSize;

if (VMStats(&vmStats, &pageSize)) {

return ((uint64_t)pageSize)

* (vmStats.active_count + vmStats.inactive_count + vmStats.wire_count

+ vmStats.free_count);

}

return 0;

}

Ideally, I think we should call the same methods. We could make these functions available for internal usage by adding them to SentryCrashMonitor_System.h and then exposing them via the SentryCrashWrapper.

By switching to SentryCrash.sharedInstance.systemInfo like @brustolin suggested, we get the uncached value and adding the log indeed shows it does change. Do you think that is an acceptable fix, or do you want to call the freeMemory method like you suggested?

Really SentryCrash.sharedInstance.systemInfo is uncached? I would be surprised. It didn't work for me though.

My logs were definitely showing different values. With you they always shows the same?

SentryCrash.sharedInstance.systemInfo is uncached
SentryCrashWrapper.sharedInstance.systemInfo is cached

Yeah that's what I am seeing as well

SentryCrash.sharedInstance.systemInfo has a lit bit of overhead because all of the information retrieved. Wondering if just exposing the freeMemory function is not a better solution.

Alrighty, done. Confirmed that it works by sending multiple events and errors, and seeing the value actually change. Now, how would we unit test this?

* master: fix: sanitize span data (#1963)

philipphofmann

The overall approach LGTM. The only thing missing are tests, I think.

kevinrenskers · 2022-07-14T08:27:34Z

The overall approach LGTM. The only thing missing are tests, I think.

That's what I said in my last comment as well: "Now, how would we unit test this?" Because I don't really know how we'd test this?

brustolin · 2022-07-14T09:14:16Z

Now, how would we unit test this?

We already have some Event tests, all you need to add Is a check whether the memory information is also attached to the event. Make sure this information is not added to OOM events.

Maybe use the free memory to compare with the information in the event with some degree of difference, I don't believe calling free_memory twice would result in the same value.

kevinrenskers · 2022-07-14T10:05:22Z

We already have testCaptureOOMEvent_RemovesFreeMemoryFromContext.

It feels kinda uncertain to just capture 2 events and see if their free memory value is different, but yeah I'm not sure how else to do this, apart from mocking the actual function that gets the free memory, and I am not sure if that's worth it? So I did it like you suggested, and the test passes.

kevinrenskers · 2022-07-14T11:54:58Z

Ok, the unit test passes locally but fails on CI. I don't think this is the best way to test this.

Tests/SentryTests/SentryClientTests.swift

brustolin · 2022-07-14T14:10:47Z

Ok, the unit test passes locally but fails on CI. I don't think this is the best way to test this.

Yeah, this will be trick to test. I doubt you get two equal values out of this function, because free memory is changing all the time.
The alternative is to write a wrapper to use in the event, I dont like this idea.

@philipphofmann any suggestion?

kevinrenskers · 2022-07-14T14:12:04Z

I spoke about this with Philip, and I am now doing a test where we're injecting a known value of free memory via SentryCrashWrapper.

brustolin · 2022-07-14T14:14:16Z

I spoke about this with Philip, and I am now doing a test where we're injecting a known value of free memory via SentryCrashWrapper.

A little bit of overkill, but it works. Good.

* master: ci: Don't run benchmarks on release (#1971) Don't track OOMs for simulators (#1970) feat: Automatic nest new spans with the ui life cycle function (#1959) docs: update some docs/comments to read a little better (#1966) ci: benchmarking updates (#1926) feat: upload list of slow/frozen rendered frame timestamps during a profile (#1910) feat: Enhance the UIViewController breadcrumbs with more data (#1945) # Conflicts: # Sources/SentryCrash/Recording/Monitors/SentryCrashMonitor_System.h

brustolin

LGTM

kevinrenskers · 2022-07-18T07:48:19Z

Why did this get auto merged when there are build failures? I'll create a new PR to fix the lint issue.

kevinrenskers · 2022-07-18T08:06:27Z

And why didn't I get this same error locally when building? Seems strange that some errors are only enabled in CI?

* master: ref: Fix linter error (#1981) fix: read free_memory when the event is captured, not only at SDK startup (#1962) fix: Remove Sentry keys from cached HTTP request headers (#1975) release: 7.21.0 ci: Don't run benchmarks on release (#1971) Don't track OOMs for simulators (#1970) feat: Automatic nest new spans with the ui life cycle function (#1959) docs: update some docs/comments to read a little better (#1966) ci: benchmarking updates (#1926) feat: upload list of slow/frozen rendered frame timestamps during a profile (#1910)

fix: read free_memory when the event is captured, not only at SDK sta…

b352c5b

…rtup Fixes #1824

Changelog

67a225c

brustolin reviewed Jul 12, 2022

View reviewed changes

Sources/Sentry/SentryClient.m Show resolved Hide resolved

brustolin reviewed Jul 13, 2022

View reviewed changes

Sources/Sentry/SentryClient.m Outdated Show resolved Hide resolved

kevinrenskers and others added 2 commits July 13, 2022 10:03

Update Sources/Sentry/SentryClient.m

2f321ef

Co-authored-by: Dhiogo Brustolin <dhiogorb@gmail.com>

Merge branch 'master' into fix/1824-read-free-memory-at-time-of-event

56c19a7

* master: fix: Crash in profiling logger (#1964) ref: Mark options.sdkInfo as deprecated (#1960) feat: Add extra app start span (#1952)

philipphofmann reviewed Jul 13, 2022

View reviewed changes

kevinrenskers added 5 commits July 13, 2022 10:07

Change import

4336949

DRY

2b53bbd

Merge branch 'master' into fix/1824-read-free-memory-at-time-of-event

f05bd6f

* master: fix: sanitize span data (#1963)

Don't fetch all system info just to get the free memory

628dd39

Remove unused import

b237658

philipphofmann reviewed Jul 14, 2022

View reviewed changes

Add testCaptureCrash_FreeMemory

588bd6d

kevinrenskers marked this pull request as ready for review July 14, 2022 10:05

brustolin reviewed Jul 14, 2022

View reviewed changes

Tests/SentryTests/SentryClientTests.swift Outdated Show resolved Hide resolved

kevinrenskers added 2 commits July 14, 2022 16:14

Strengthen test via injection

72d4a8b

kevinrenskers requested review from philipphofmann and brustolin July 14, 2022 14:16

kevinrenskers enabled auto-merge (squash) July 14, 2022 16:22

Move changelog entry

9c507a8

Merge branch 'master' into fix/1824-read-free-memory-at-time-of-event

a114f78

brustolin approved these changes Jul 18, 2022

View reviewed changes

kevinrenskers merged commit 9640294 into master Jul 18, 2022

kevinrenskers deleted the fix/1824-read-free-memory-at-time-of-event branch July 18, 2022 07:16

kevinrenskers mentioned this pull request Aug 1, 2022

feat: Report App Memory Usage #2027

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: read free_memory when the event is captured, not only at SDK startup #1962

fix: read free_memory when the event is captured, not only at SDK startup #1962

kevinrenskers commented Jul 12, 2022 •

edited

kevinrenskers commented Jul 12, 2022

philipphofmann left a comment •

edited

philipphofmann Jul 13, 2022 •

edited

kevinrenskers Jul 13, 2022

philipphofmann Jul 13, 2022 •

edited

kevinrenskers Jul 13, 2022

brustolin Jul 13, 2022

kevinrenskers Jul 13, 2022

brustolin Jul 13, 2022

kevinrenskers Jul 13, 2022

philipphofmann left a comment

kevinrenskers commented Jul 14, 2022

brustolin commented Jul 14, 2022 •

edited

kevinrenskers commented Jul 14, 2022

kevinrenskers commented Jul 14, 2022

brustolin commented Jul 14, 2022 •

edited

kevinrenskers commented Jul 14, 2022

brustolin commented Jul 14, 2022

brustolin left a comment

kevinrenskers commented Jul 18, 2022

kevinrenskers commented Jul 18, 2022

	static uint64_t
	freeMemory(void)
	{
	vm_statistics_data_t vmStats;
	vm_size_t pageSize;
	if (VMStats(&vmStats, &pageSize)) {
	return ((uint64_t)pageSize) * vmStats.free_count;
	}
	return 0;
	}

	static uint64_t
	usableMemory(void)
	{
	vm_statistics_data_t vmStats;
	vm_size_t pageSize;
	if (VMStats(&vmStats, &pageSize)) {
	return ((uint64_t)pageSize)
	* (vmStats.active_count + vmStats.inactive_count + vmStats.wire_count
	+ vmStats.free_count);
	}
	return 0;
	}

fix: read free_memory when the event is captured, not only at SDK startup #1962

fix: read free_memory when the event is captured, not only at SDK startup #1962

Conversation

kevinrenskers commented Jul 12, 2022 • edited

📜 Description

💡 Motivation and Context

💚 How did you test it?

📝 Checklist

🔮 Next steps

kevinrenskers commented Jul 12, 2022

philipphofmann left a comment • edited

Choose a reason for hiding this comment

philipphofmann Jul 13, 2022 • edited

Choose a reason for hiding this comment

kevinrenskers Jul 13, 2022

Choose a reason for hiding this comment

philipphofmann Jul 13, 2022 • edited

Choose a reason for hiding this comment

kevinrenskers Jul 13, 2022

Choose a reason for hiding this comment

brustolin Jul 13, 2022

Choose a reason for hiding this comment

kevinrenskers Jul 13, 2022

Choose a reason for hiding this comment

brustolin Jul 13, 2022

Choose a reason for hiding this comment

kevinrenskers Jul 13, 2022

Choose a reason for hiding this comment

philipphofmann left a comment

Choose a reason for hiding this comment

kevinrenskers commented Jul 14, 2022

brustolin commented Jul 14, 2022 • edited

kevinrenskers commented Jul 14, 2022

kevinrenskers commented Jul 14, 2022

brustolin commented Jul 14, 2022 • edited

kevinrenskers commented Jul 14, 2022

brustolin commented Jul 14, 2022

brustolin left a comment

Choose a reason for hiding this comment

kevinrenskers commented Jul 18, 2022

kevinrenskers commented Jul 18, 2022

kevinrenskers commented Jul 12, 2022 •

edited

philipphofmann left a comment •

edited

philipphofmann Jul 13, 2022 •

edited

philipphofmann Jul 13, 2022 •

edited

brustolin commented Jul 14, 2022 •

edited

brustolin commented Jul 14, 2022 •

edited