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.

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

feat: Add onCrashedLastRun #808

Merged
merged 21 commits into from
Nov 25, 2020
Merged
Show file tree
Hide file tree
Changes from 17 commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
0961c89
feat: Add onCrashedLastRun
philipphofmann Oct 21, 2020
416321d
Committing formatted code
Oct 21, 2020
bd556f4
Merge branch 'master' into feat/on-crashed-last-run
philipphofmann Nov 12, 2020
fd0d836
Call callback only for first crash event
philipphofmann Nov 12, 2020
eb52611
Add comment about downside of callback
philipphofmann Nov 12, 2020
29ce980
Merge branch 'master' into feat/on-crashed-last-run
philipphofmann Nov 12, 2020
cebee03
Merge branch 'master' into feat/on-crashed-last-run
philipphofmann Nov 16, 2020
16ff083
Check if event contains unhandled exception
philipphofmann Nov 16, 2020
c99eeab
Committing formatted code
Nov 16, 2020
cc4c291
Merge branch 'master' into feat/on-crashed-last-run
philipphofmann Nov 16, 2020
1633157
Merge branch 'master' into feat/on-crashed-last-run
philipphofmann Nov 18, 2020
dacc561
Add tests
philipphofmann Nov 19, 2020
b3e49d8
Undo changes in iOS-Swift AppDelegate
philipphofmann Nov 19, 2020
80439ea
Remove SentryId from SentryDefines
philipphofmann Nov 19, 2020
fa66553
Update changelog
philipphofmann Nov 19, 2020
44b4f5e
Merge branch 'master' into feat/on-crashed-last-run
philipphofmann Nov 20, 2020
01ee8fc
Merge branch 'master' into feat/on-crashed-last-run
philipphofmann Nov 23, 2020
8a9faea
Toggle BOOL after action for callOnCrashedLastRun
philipphofmann Nov 24, 2020
b04e388
Fix typos
philipphofmann Nov 24, 2020
4950e6b
Add captureCrash to SentryClient
philipphofmann Nov 24, 2020
008008d
Merge branch 'master' into feat/on-crashed-last-run
philipphofmann Nov 25, 2020
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
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

## unreleased

- feat: Add onCrashedLastRun #808
- feat: Add SentrySdkInfo to SentryOptions #859

## 6.0.9
Expand Down
5 changes: 5 additions & 0 deletions Sources/Sentry/Public/SentryDefines.h
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,11 @@ typedef SentryBreadcrumb *_Nullable (^SentryBeforeBreadcrumbCallback)(
*/
typedef SentryEvent *_Nullable (^SentryBeforeSendEventCallback)(SentryEvent *_Nonnull event);

/**
* A callback to be notified when the last program execution terminated with a crash.
*/
typedef void (^SentryOnCrashedLastRunCallback)(SentryEvent *_Nonnull event);

/**
* Block can be used to determine if an event should be queued and stored
* locally. It will be tried to send again after next successful send. Note that
Expand Down
11 changes: 11 additions & 0 deletions Sources/Sentry/Public/SentryOptions.h
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,17 @@ NS_SWIFT_NAME(Options)
*/
@property (nonatomic, copy) SentryBeforeBreadcrumbCallback _Nullable beforeBreadcrumb;

/**
* This gets called shortly after the initialization of the SDK when the last program execution
* terminated with a crash. It is not guaranteed that this is called on the main thread.
*
* @discussion This callback is only executed once during the entire run of the program to avoid
* multiple callbacks if there are multiple crash events to send. This can happen when the program
* terminates with a crash before the SDK can send the crash event. You can look into beforeSend if
* you prefer a callback for every event.
*/
@property (nonatomic, copy) SentryOnCrashedLastRunCallback _Nullable onCrashedLastRun;

/**
* Array of integrations to install.
*/
Expand Down
40 changes: 40 additions & 0 deletions Sources/Sentry/SentryClient.m
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
#import "SentryClient.h"
#import "NSDictionary+SentrySanitize.h"
#import "SentryCrashAdapter.h"
#import "SentryCrashDefaultBinaryImageProvider.h"
#import "SentryCrashDefaultMachineContextWrapper.h"
#import "SentryDebugMetaBuilder.h"
Expand All @@ -14,9 +15,12 @@
#import "SentryId.h"
#import "SentryInstallation.h"
#import "SentryLog.h"
#import "SentryMechanism.h"
#import "SentryMessage.h"
#import "SentryMeta.h"
#import "SentryOptions.h"
#import "SentrySDK+Private.h"
#import "SentrySDK.h"
#import "SentryScope.h"
#import "SentryStacktraceBuilder.h"
#import "SentryThreadInspector.h"
Expand All @@ -38,6 +42,7 @@
@property (nonatomic, strong) SentryFileManager *fileManager;
@property (nonatomic, strong) SentryDebugMetaBuilder *debugMetaBuilder;
@property (nonatomic, strong) SentryThreadInspector *threadInspector;
@property (nonatomic, strong) SentryCrashAdapter *crashAdapter;

@end

Expand Down Expand Up @@ -79,6 +84,8 @@ - (_Nullable instancetype)initWithOptions:(SentryOptions *)options

self.transport = [SentryTransportFactory initTransport:self.options
sentryFileManager:self.fileManager];

self.crashAdapter = [[SentryCrashAdapter alloc] init];
}
return self;
}
Expand All @@ -87,11 +94,13 @@ - (_Nullable instancetype)initWithOptions:(SentryOptions *)options
- (instancetype)initWithOptions:(SentryOptions *)options
andTransport:(id<SentryTransport>)transport
andFileManager:(SentryFileManager *)fileManager
andCrashAdapter:(SentryCrashAdapter *)crashAdapter
{
self = [self initWithOptions:options];

self.transport = transport;
self.fileManager = fileManager;
self.crashAdapter = crashAdapter;

return self;
}
Expand Down Expand Up @@ -366,6 +375,8 @@ - (SentryEvent *_Nullable)prepareEvent:(SentryEvent *)event
event = self.options.beforeSend(event);
}

[self callOnCrashedLastRun:event];

return event;
}

Expand Down Expand Up @@ -422,6 +433,35 @@ - (void)setUserIdIfNoUserSet:(SentryEvent *)event
}
}

/**
* Calls onCrashedLastRun if the execution terminated with a crash.
philipphofmann marked this conversation as resolved.
Show resolved Hide resolved
*/
- (void)callOnCrashedLastRun:(SentryEvent *)event
{
if (nil != self.options.onCrashedLastRun && self.crashAdapter.crashedLastLaunch
&& !SentrySDK.crashedLastRunCalled && nil != event.exceptions) {

NSPredicate *unhandledExpeptions = [NSPredicate predicateWithBlock:^BOOL(
id _Nullable object, NSDictionary<NSString *, id> *_Nullable bindings) {
SentryException *exception = object;
return nil != exception.mechanism && nil != exception.mechanism.handled
&& ![exception.mechanism.handled boolValue];
}];
Copy link
Contributor

@marandaneto marandaneto Nov 23, 2020

Choose a reason for hiding this comment

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

H: I think this would work for most of the cases, but it's flakey IMO.
how do we know that the event calling onCrashedLastRun is actually the one that caused the crash?

I'd suggest another approach:

SentryCrash sets fixture.crashAdapter.internalCrashedLastLaunch so it knows the event that caused this flag to be toggled.
What about caching the eventId as well that caused the app to crash and later, we filter by eventId? so we'll be sure that the onCrashedLastRun is actually called with the event that actually crashed the App.

My concern is, if the app is offline and there are multiple and distinct hard crashes, the event called onCrashedLastRun might not be the event that actually caused the last crash and the user feedback won't be accurate.

Copy link
Member Author

@philipphofmann philipphofmann Nov 24, 2020

Choose a reason for hiding this comment

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

It's very good that you think about such edge cases, but I think we can take another simpler approach.

When a crash happens, SentryCrash writes a crash-report.json to disk. The next time the app is launched, the SDK installs the SentryCrashIntegration, which calls the SentryCrashReportConverter, who converts the crash-report.json to a SentryEvent. The SentryCrashReportSink then calls [SentrySDK captureCrashEvent:event];. At that point, SentrySDK knows that it is an event with a crash. The SentryHub also has a captureCrashEvent, but the SentryClient doesn't. Therefore I suggest that the SentryHub just passes this information to the SentryClient and in callOnCrashedLastRun we can simplify the logic a lot.

Good point I'm going to change this.


// We only want to filter the array if the above conditions are true to avoid unecessary
// work
BOOL eventContainsUnhandledExceptions =
[event.exceptions filteredArrayUsingPredicate:unhandledExpeptions].count > 0;

if (eventContainsUnhandledExceptions) {
// We only want to call the callback once. It can occur that multiple crash events are
// about to be sent.
SentrySDK.crashedLastRunCalled = YES;
self.options.onCrashedLastRun(event);
philipphofmann marked this conversation as resolved.
Show resolved Hide resolved
}
}
}

@end

NS_ASSUME_NONNULL_END
4 changes: 4 additions & 0 deletions Sources/Sentry/SentryOptions.m
Original file line number Diff line number Diff line change
Expand Up @@ -128,6 +128,10 @@ - (void)validateOptions:(NSDictionary<NSString *, id> *)options
self.beforeBreadcrumb = options[@"beforeBreadcrumb"];
}

if (nil != options[@"onCrashedLastRun"]) {
self.onCrashedLastRun = options[@"onCrashedLastRun"];
}

if (nil != options[@"integrations"]) {
self.integrations = options[@"integrations"];
}
Expand Down
11 changes: 11 additions & 0 deletions Sources/Sentry/SentrySDK.m
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
@implementation SentrySDK

static SentryHub *currentHub;
static BOOL crashedLastRunCalled;

@dynamic logLevel;

Expand All @@ -43,6 +44,16 @@ + (void)setCurrentHub:(SentryHub *)hub
}
}

+ (BOOL)crashedLastRunCalled
{
return crashedLastRunCalled;
}

+ (void)setCrashedLastRunCalled:(BOOL)value
{
crashedLastRunCalled = value;
}
marandaneto marked this conversation as resolved.
Show resolved Hide resolved

+ (void)startWithOptions:(NSDictionary<NSString *, id> *)optionsDict
{
NSError *error = nil;
Expand Down
5 changes: 5 additions & 0 deletions Sources/Sentry/include/SentrySDK+Private.h
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,11 @@ NS_ASSUME_NONNULL_BEGIN

+ (void)captureCrashEvent:(SentryEvent *)event;

/**
* SDK private field to store the state if onCrashedLastRun was called.
*/
@property (nonatomic, class) BOOL crashedLastRunCalled;

@end

NS_ASSUME_NONNULL_END
5 changes: 4 additions & 1 deletion Tests/SentryTests/SentryClient+TestInit.h
Original file line number Diff line number Diff line change
@@ -1,14 +1,17 @@
#import "SentryTransport.h"
#import <Sentry/Sentry.h>

@class SentryCrashAdapter;

NS_ASSUME_NONNULL_BEGIN

/** Expose the internal test init for testing. */
@interface SentryClient (TestInit)

- (instancetype)initWithOptions:(SentryOptions *)options
andTransport:(id<SentryTransport>)transport
andFileManager:(SentryFileManager *)fileManager;
andFileManager:(SentryFileManager *)fileManager
andCrashAdapter:(SentryCrashAdapter *)crashAdapter;

@end

Expand Down
57 changes: 56 additions & 1 deletion Tests/SentryTests/SentryClientTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,8 @@ class SentryClientTest: XCTestCase {
let user: User
let fileManager: SentryFileManager

let crashAdapter = TestSentryCrashWrapper()

init() {
session = SentrySession(releaseName: "release")
session.incrementErrors()
Expand All @@ -48,7 +50,7 @@ class SentryClientTest: XCTestCase {
])
configureOptions(options)

client = Client(options: options, andTransport: transport, andFileManager: fileManager)
client = Client(options: options, andTransport: transport, andFileManager: fileManager, andCrashAdapter: crashAdapter)
} catch {
XCTFail("Options could not be created")
}
Expand Down Expand Up @@ -76,6 +78,16 @@ class SentryClientTest: XCTestCase {
return scope
}
}

var eventWithCrash: Event {
let event = TestData.event
let exception = Exception(value: "value", type: "type")
let mechanism = Mechanism(type: "mechanism")
mechanism.handled = false
exception.mechanism = mechanism
event.exceptions = [exception]
return event
}
}

private let error = NSError(domain: "domain", code: -20, userInfo: [NSLocalizedDescriptionKey: "Object does not exist"])
Expand All @@ -90,6 +102,11 @@ class SentryClientTest: XCTestCase {
fixture.fileManager.deleteAllEnvelopes()
}

override func tearDown() {
super.tearDown()
SentrySDK.crashedLastRunCalled = false
}

func testCaptureMessage() {
let eventId = fixture.getSut().capture(message: fixture.messageAsString)

Expand Down Expand Up @@ -615,6 +632,44 @@ class SentryClientTest: XCTestCase {
XCTAssertEqual(1, fixture.fileManager.getAllEnvelopes().count)
}

func testOnCrashedLastRun_WithTwoCrashes_OnlyInvoceOnce() {
philipphofmann marked this conversation as resolved.
Show resolved Hide resolved
let event = fixture.eventWithCrash

var onCrashedLastRunCalled = false
let client = fixture.getSut(configureOptions: { options in
options.onCrashedLastRun = { crashEvent in
onCrashedLastRunCalled = true
XCTAssertEqual(event.eventId, crashEvent.eventId)
}
})
fixture.crashAdapter.internalCrashedLastLaunch = true

client.capture(event: event)
client.capture(event: fixture.eventWithCrash)

XCTAssertTrue(onCrashedLastRunCalled)
}

func testOnCrashedLastRun_WithHandledException() {
var onCrashedLastRunCalled = false
let client = fixture.getSut(configureOptions: { options in
options.onCrashedLastRun = { _ in
onCrashedLastRunCalled = true
}
})
fixture.crashAdapter.internalCrashedLastLaunch = true

client.capture(event: TestData.event)

XCTAssertFalse(onCrashedLastRunCalled)
}

func testOnCrashedLastRun_WithOutCallback_DoesNothing() {
philipphofmann marked this conversation as resolved.
Show resolved Hide resolved
let client = fixture.getSut()
fixture.crashAdapter.internalCrashedLastLaunch = true
client.capture(event: fixture.eventWithCrash)
}

private func givenEventWithDebugMeta() -> Event {
let event = Event(level: SentryLevel.fatal)
let debugMeta = DebugMeta()
Expand Down
22 changes: 22 additions & 0 deletions Tests/SentryTests/SentryOptionsTest.m
Original file line number Diff line number Diff line change
Expand Up @@ -204,6 +204,28 @@ - (void)testDefaultBeforeBreadcrumb
XCTAssertNil(options.beforeBreadcrumb);
}

- (void)testOnCrashedLastRun
{
__block BOOL onCrashedLastRunCalled = NO;
void (^callback)(SentryEvent *event) = ^(SentryEvent *event) {
onCrashedLastRunCalled = YES;
XCTAssertNotNil(event);
};
SentryOptions *options = [self getValidOptions:@{ @"onCrashedLastRun" : callback }];

options.onCrashedLastRun([[SentryEvent alloc] init]);

XCTAssertEqual(callback, options.onCrashedLastRun);
XCTAssertTrue(onCrashedLastRunCalled);
}

- (void)testDefaultOnCrashedLastRun
{
SentryOptions *options = [self getValidOptions:@{}];

XCTAssertNil(options.onCrashedLastRun);
}

- (void)testIntegrations
{
NSArray<NSString *> *integrations = @[ @"integration1", @"integration2" ];
Expand Down