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

feat: Close session for unhandled error #3091

Merged
merged 9 commits into from
Jun 15, 2023
Merged

feat: Close session for unhandled error #3091

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
69c1637
feat: Close session for unhandled error
brustolin Jun 9, 2023
a187d02
Update SentryHubTests.swift
brustolin Jun 9, 2023
7a33aa9
suggestions
brustolin Jun 13, 2023
078d671
Format code
getsentry-bot Jun 13, 2023
2ce0cbe
Update SentryHubTests.swift
brustolin Jun 13, 2023
edca2c6
Update SentryHub.m
brustolin Jun 13, 2023
aab4503
Format code
getsentry-bot Jun 13, 2023
99177cf
Update SentryHubTests.swift
brustolin Jun 14, 2023
1c55ca4
Apply suggestions from code review
brustolin Jun 15, 2023
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
38 changes: 33 additions & 5 deletions Sources/Sentry/SentryHub.m
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
#import "SentryEvent+Private.h"
#import "SentryFileManager.h"
#import "SentryId.h"
#import "SentryLevelMapper.h"
#import "SentryLog.h"
#import "SentryNSTimerWrapper.h"
#import "SentryPerformanceTracker.h"
Expand Down Expand Up @@ -590,37 +591,64 @@ - (void)captureEnvelope:(SentryEnvelope *)envelope

- (SentryEnvelope *)updateSessionState:(SentryEnvelope *)envelope
{
if ([self envelopeContainsEventWithErrorOrHigher:envelope.items]) {
BOOL handled = YES;
if ([self envelopeContainsEventWithErrorOrHigher:envelope.items wasHandled:&handled]) {
SentrySession *currentSession = [self incrementSessionErrors];

if (currentSession != nil) {
if (!handled) {
[currentSession endSessionCrashedWithTimestamp:[_currentDateProvider date]];

_session = [[SentrySession alloc] initWithReleaseName:_client.options.releaseName];
_session.environment = _client.options.environment;
[self.scope applyToSession:_session];
Copy link
Member

Choose a reason for hiding this comment

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

h: That's not how we safely end and start a new session. I think we should call the following instead.

[self endSessionWithTimestamp:[_currentDateProvider date]];
[hub startSession];

As we also do in the session tracker

sentry-cocoa/Sources/Sentry/SentrySessionTracker.m

Lines 173 to 174 in 60dd0f5

[hub endSessionWithTimestamp:self.lastInForeground];
[hub startSession];

Or am I missing something?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe I'm missing something, but this is what I get:

  • currentSession is a clone of the current session, If a call self endSessionWithTimestamp this will not end currentSession.
  • If I end it before cloning it I cannot clone it.
  • By calling [hub endSession] the session is immediately captured, but here, the session was being sent in the same envelope.
  • By calling startSession the last session is also capture, which would duplicate the capture

Copy link
Member

Choose a reason for hiding this comment

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

Okay, then what I proposed doesn't seem correct. Your solution definitely needs a lock with @synchronized(_sessionLock) when creating the new session, you need to handle _errorsBeforeSession, you need to handle the edge case when there is no releaseName, and you need to store the new session and also capture the new session. Similar to what startSession does. I think it would be better to maybe refacrot startSession so you can also use it here.

Furthermore, I think it would make sense to not increment the session error when the event is unhandled. We should just mark it as crashed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Please, take a look at the new approach.

}

// Create a new envelope with the session update
NSMutableArray<SentryEnvelopeItem *> *itemsToSend =
[[NSMutableArray alloc] initWithArray:envelope.items];
[itemsToSend addObject:[[SentryEnvelopeItem alloc] initWithSession:currentSession]];

return [[SentryEnvelope alloc] initWithHeader:envelope.header items:itemsToSend];
}
}

return envelope;
}

- (BOOL)envelopeContainsEventWithErrorOrHigher:(NSArray<SentryEnvelopeItem *> *)items
wasHandled:(BOOL *)handled;
{
for (SentryEnvelopeItem *item in items) {
if ([item.header.type isEqualToString:SentryEnvelopeItemTypeEvent]) {
// If there is no level the default is error
SentryLevel level = [SentrySerialization levelFromData:item.data];
NSDictionary *eventJson = [SentrySerialization deserializeEventEnvelopeItem:item.data];
if (eventJson == nil) {
return NO;
philipphofmann marked this conversation as resolved.
Show resolved Hide resolved
}

SentryLevel level = sentryLevelForString(eventJson[@"level"]);
if (level >= kSentryLevelError) {
*handled = [self eventContainsUnhandledError:eventJson];
return YES;
}
}
}

return NO;
}

- (BOOL)eventContainsUnhandledError:(NSDictionary *)eventDictionary
{
NSArray *exceptions = eventDictionary[@"exception"][@"values"];
for (NSDictionary *exception in exceptions) {
NSDictionary *mechanism = exception[@"mechanism"];
NSNumber *handled = mechanism[@"handled"];

if ([handled boolValue] == NO) {
return NO;
}
}
return YES;
}

- (void)reportFullyDisplayed
{
#if SENTRY_HAS_UIKIT
Expand Down
17 changes: 17 additions & 0 deletions Sources/Sentry/SentrySerialization.m
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -339,6 +339,23 @@ + (SentryAppState *_Nullable)appStateWithData:(NSData *)data
return [[SentryAppState alloc] initWithJSONObject:appSateDictionary];
}

+ (NSDictionary *)deserializeEventEnvelopeItem:(NSData *)eventEnvelopeItemData
{
NSError *error = nil;
NSDictionary *eventDictionary = [NSJSONSerialization JSONObjectWithData:eventEnvelopeItemData
options:0
error:&error];
if (nil != error) {
[SentryLog
logWithMessage:[NSString
stringWithFormat:@"Failed to deserialize envelope item data: %@",
error]
andLevel:kSentryLevelError];
}

return eventDictionary;
}

+ (SentryLevel)levelFromData:(NSData *)eventEnvelopeItemData
{
NSError *error = nil;
Expand Down
3 changes: 2 additions & 1 deletion Sources/Sentry/include/SentryHub+Private.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
#import "SentryTracer.h"

@class SentryEnvelopeItem, SentryId, SentryScope, SentryTransaction, SentryDispatchQueueWrapper,
SentryEnvelope, SentryNSTimerWrapper;
SentryEnvelope, SentryNSTimerWrapper, SentrySession;

NS_ASSUME_NONNULL_BEGIN

Expand All @@ -11,6 +11,7 @@ SentryHub (Private)

@property (nonatomic, strong) NSArray<id<SentryIntegrationProtocol>> *installedIntegrations;
@property (nonatomic, strong) NSSet<NSString *> *installedIntegrationNames;
@property (nullable, nonatomic, strong) SentrySession *session;

- (void)addInstalledIntegration:(id<SentryIntegrationProtocol>)integration name:(NSString *)name;
- (void)removeAllIntegrations;
Expand Down
5 changes: 5 additions & 0 deletions Sources/Sentry/include/SentrySerialization.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,11 @@ static int const SENTRY_BAGGAGE_MAX_SIZE = 8192;

+ (SentryAppState *_Nullable)appStateWithData:(NSData *)sessionData;

/**
* Retrieves the json object from an event envelope item data.
*/
+ (NSDictionary *)deserializeEventEnvelopeItem:(NSData *)eventEnvelopeItemData;
brustolin marked this conversation as resolved.
Show resolved Hide resolved

/**
* Extract the level from data of an envelopte item containing an event. Default is the 'error'
* level, see https://develop.sentry.dev/sdk/event-payloads/#optional-attributes
Expand Down
Loading