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

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

Merged
merged 14 commits into from
Jul 18, 2022
Merged

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

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
b352c5b
fix: read free_memory when the event is captured, not only at SDK sta…
kevinrenskers Jul 12, 2022
67a225c
Changelog
kevinrenskers Jul 12, 2022
2f321ef
Update Sources/Sentry/SentryClient.m
kevinrenskers Jul 13, 2022
56c19a7
Merge branch 'master' into fix/1824-read-free-memory-at-time-of-event
kevinrenskers Jul 13, 2022
4336949
Change import
kevinrenskers Jul 13, 2022
2b53bbd
DRY
kevinrenskers Jul 13, 2022
f05bd6f
Merge branch 'master' into fix/1824-read-free-memory-at-time-of-event
kevinrenskers Jul 13, 2022
628dd39
Don't fetch all system info just to get the free memory
kevinrenskers Jul 13, 2022
b237658
Remove unused import
kevinrenskers Jul 13, 2022
588bd6d
Add testCaptureCrash_FreeMemory
kevinrenskers Jul 14, 2022
72d4a8b
Strengthen test via injection
kevinrenskers Jul 14, 2022
a48bdba
Merge branch 'master' into fix/1824-read-free-memory-at-time-of-event
kevinrenskers Jul 14, 2022
9c507a8
Move changelog entry
kevinrenskers Jul 15, 2022
a114f78
Merge branch 'master' into fix/1824-read-free-memory-at-time-of-event
brustolin Jul 18, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@

- Properly sanitize the event context and SDK information (#1943)
- Don't send error 429 as `network_error` (#1957)
- Read free_memory when the event is captured, not only at SDK startup (#1962)

## 7.20.0

Expand Down
25 changes: 23 additions & 2 deletions Sources/Sentry/SentryClient.m
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
#import "SentryCrashDefaultMachineContextWrapper.h"
#import "SentryCrashIntegration.h"
#import "SentryCrashStackEntryMapper.h"
#import "SentryCrashWrapper.h"
#import "SentryDebugImageProvider.h"
#import "SentryDefaultCurrentDateProvider.h"
#import "SentryDependencyContainer.h"
Expand Down Expand Up @@ -487,10 +488,13 @@ - (SentryEvent *_Nullable)prepareEvent:(SentryEvent *)event

event = [scope applyToEvent:event maxBreadcrumb:self.options.maxBreadcrumbs];

// Remove free_memory if OOM as free_memory stems from the current run and not of the time of
// the OOM.
if ([self isOOM:event isCrashEvent:isCrashEvent]) {
// Remove free_memory if OOM as free_memory stems from the current run and not of
// the time of the OOM.
[self removeFreeMemoryFromDeviceContext:event];
} else {
// Store the actual free memory at the time of this event
[self storeFreeMemoryToDeviceContext:event];
kevinrenskers marked this conversation as resolved.
Show resolved Hide resolved
}

// With scope applied, before running callbacks run:
Expand Down Expand Up @@ -638,6 +642,23 @@ - (BOOL)isOOM:(SentryEvent *)event isCrashEvent:(BOOL)isCrashEvent
[exception.mechanism.type isEqualToString:SentryOutOfMemoryMechanismType];
}

- (void)storeFreeMemoryToDeviceContext:(SentryEvent *)event
kevinrenskers marked this conversation as resolved.
Show resolved Hide resolved
{
if (nil == event.context || event.context.count == 0 || nil == event.context[@"device"]) {
return;
}

NSMutableDictionary *context = [[NSMutableDictionary alloc] initWithDictionary:event.context];
NSMutableDictionary *device =
[[NSMutableDictionary alloc] initWithDictionary:context[@"device"]];

NSDictionary *systemInfo = [[SentryCrashWrapper sharedInstance] systemInfo];
kevinrenskers marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Member

@philipphofmann philipphofmann Jul 13, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Member

@philipphofmann philipphofmann Jul 13, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah that's what I am seeing as well

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

device[SentryDeviceContextFreeMemoryKey] = systemInfo[@"freeMemory"];

context[@"device"] = device;
event.context = context;
}

- (void)removeFreeMemoryFromDeviceContext:(SentryEvent *)event
{
if (nil == event.context || event.context.count == 0 || nil == event.context[@"device"]) {
Expand Down