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

On by default session tracking #286

Merged
merged 30 commits into from
Jun 21, 2018
Merged
Show file tree
Hide file tree
Changes from 22 commits
Commits
Show all changes
30 commits
Select commit Hold shift + click to select a range
831f63d
feat: add setEndpoints interface to configuration
fractalwrench May 14, 2018
dceac81
test: Add test for legacy notifyURL property
fractalwrench May 14, 2018
b6d16bf
test: test setEndpoints notify url validation
fractalwrench May 14, 2018
48e267f
feat: Ignore tracking session if invalid session URL supplied
fractalwrench May 14, 2018
b3d5420
feat: enable session tracking by default
fractalwrench May 14, 2018
abb08c2
refactor: use nsassert rather than nsexception.raise for endpoint con…
fractalwrench Jun 5, 2018
4b7277d
test: make mazerunner scenario NOT autocapture sessions by default, w…
fractalwrench Jun 5, 2018
f3922c6
docs: update documentation of session tracking on by default
fractalwrench Jun 5, 2018
26a2c37
use string rather than url for mazerunner scenario
fractalwrench Jun 5, 2018
57722ed
docs: update session tracking obd docs
fractalwrench Jun 6, 2018
0645bad
add missing comma in method sig
fractalwrench Jun 6, 2018
8980ea3
fix minor docs typo
fractalwrench Jun 6, 2018
0677b66
test: begin porting android session tracking mazerunner scenarios to …
fractalwrench Jun 7, 2018
fda541e
test: add manual and disabled session tracking scenarios
fractalwrench Jun 7, 2018
e12ff70
fix: serialise user email using 'email' key for session requests
fractalwrench Jun 7, 2018
d813cef
test: fix user serialisation test
fractalwrench Jun 7, 2018
c89979d
Merge branch 'next' into on-by-default-session-tracking
fractalwrench Jun 11, 2018
ec307f1
refactor: use xctassertthrows rather than try/catch block in test ass…
fractalwrench Jun 12, 2018
e84d311
fix: prevent duplicate automatic session count in constructor
fractalwrench Jun 13, 2018
0ff92ec
fix: ensure session is captured on notifier start
fractalwrench Jun 13, 2018
57044de
fix: use semaphore to synchronise access to session flushing
fractalwrench Jun 14, 2018
348e095
test: update mazerunner auto session scenario user.id assertion to re…
fractalwrench Jun 14, 2018
3bf9f72
fix: prevent potential deadlock when payload count == 0, by signallin…
fractalwrench Jun 18, 2018
8a65049
test: update mazerunner session scenarios to rely on automatic flushi…
fractalwrench Jun 18, 2018
f4bffac
feat: make notifyURL and sessionURL readonly
fractalwrench Jun 18, 2018
19d23a4
docs: add changelog entry for cocoa session tracking
fractalwrench Jun 18, 2018
70c6e76
Merge branch 'next' into on-by-default-session-tracking
fractalwrench Jun 18, 2018
00bb48b
test: pass readonly notifyURL + sessionURL tests
fractalwrench Jun 20, 2018
8bd994f
Merge branch 'on-by-default-session-tracking' of https://github.com/b…
fractalwrench Jun 20, 2018
d05198d
docs: note that overriding endpoints is for test purposes only
fractalwrench Jun 20, 2018
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
40 changes: 33 additions & 7 deletions Source/BugsnagConfiguration.h
Original file line number Diff line number Diff line change
Expand Up @@ -73,10 +73,6 @@ typedef NSDictionary *_Nullable (^BugsnagBeforeNotifyHook)(
* The API key of a Bugsnag project
*/
@property(readwrite, retain, nullable) NSString *apiKey;
/**
* The URL used to notify Bugsnag
*/
@property(readwrite, retain, nullable) NSURL *notifyURL;
/**
* The release stage of the application, such as production, development, beta
* et cetera
Expand Down Expand Up @@ -142,16 +138,46 @@ BugsnagBreadcrumbs *breadcrumbs;
@property BOOL autoNotify;

/**
* Determines whether app sessions should be tracked automatically. By default this value is false.
* Determines whether app sessions should be tracked automatically. By default this value is true.
*/
@property BOOL shouldAutoCaptureSessions;

/**
* Set the endpoint to which tracked sessions reports are sent. This defaults to https://sessions.bugsnag.com,
* but should be overridden if you are using Bugsnag On-premise, to point to your own Bugsnag endpoint.
* Retrieves the endpoint used to notify Bugsnag of errors
*
* NOTE: it is strongly recommended that you set this value via setEndpointsForNotify:sessions: instead.

Choose a reason for hiding this comment

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

is it worth using something like this to make this as deprecated?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's one of the outstanding questions - what are your thoughts on this? If we add setEndpointsForNotify:sessions:, then developers are able to set the value, but won't be able to read it without notifyURL and sessionsURL.

The way I see it there are a couple of options:

  • Leave notifyURL and sessionsURL as is and document they shouldn't be set
  • Deprecate then remove the properties
  • Make the properties readonly (breaking change)

I've currently leant towards the first option.

Choose a reason for hiding this comment

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

I see the dilemma. Is it possible to split the property up and only deprecate the write? I only jumped on this as I saw in the android notifier that we were deprecating bits like this as part of similar changes. If it doesn't make sense for bugsnag-cocoa then that is cool

Copy link
Contributor

@kattrali kattrali Jun 15, 2018

Choose a reason for hiding this comment

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

Might be worth trying:

  • Mark both properties as readonly
  • Add setter methods
  • Mark setters as deprecated

*
* @see setEndpointsForNotify:sessions:
*/
@property(readwrite, retain, nullable) NSURL *notifyURL;

/**
* Retrieves the endpoint used to send tracked sessions to Bugsnag
*
* NOTE: it is strongly recommended that you set this value via setEndpointsForNotify:sessions: instead.

Choose a reason for hiding this comment

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

is it worth using something like this to make this as deprecated?

*
* @see setEndpointsForNotify:sessions:
*/
@property(readwrite, retain, nullable) NSURL *sessionURL;

/**
* Set the endpoints to send data to. By default we'll send error reports to
* https://notify.bugsnag.com, and sessions to https://sessions.bugsnag.com, but you can
* override this if you are using Bugsnag Enterprise to point to your own Bugsnag endpoint.
*
* Please note that it is recommended that you set both endpoints. If the notify endpoint is
* missing, an assertion will be thrown. If the session endpoint is missing, a warning will be
* logged and sessions will not be sent automatically.
*
* @param notify the notify endpoint
* @param sessions the sessions endpoint
*
* @throws an assertion if the notify endpoint is not a valid URL
*/

- (void)setEndpointsForNotify:(NSString *_Nonnull)notify
sessions:(NSString *_Nonnull)sessions NS_SWIFT_NAME(setEndpoints(notify:sessions:));

/**
* Set user metadata
*
Expand Down
22 changes: 20 additions & 2 deletions Source/BugsnagConfiguration.m
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,8 @@ - (id)init {
_notifyReleaseStages = nil;
_breadcrumbs = [BugsnagBreadcrumbs new];
_automaticallyCollectBreadcrumbs = YES;
_shouldAutoCaptureSessions = YES;

if ([NSURLSession class]) {
_session = [NSURLSession
sessionWithConfiguration:[NSURLSessionConfiguration
Expand All @@ -87,7 +89,7 @@ - (BOOL)shouldSendReports {
- (void)setUser:(NSString *)userId
withName:(NSString *)userName
andEmail:(NSString *)userEmail {

self.currentUser = [[BugsnagUser alloc] initWithUserId:userId name:userName emailAddress:userEmail];

[self.metaData addAttribute:BSGKeyId withValue:userId toTabWithName:BSGKeyUser];
Expand Down Expand Up @@ -225,7 +227,7 @@ - (BOOL)shouldAutoCaptureSessions {
- (void)setShouldAutoCaptureSessions:(BOOL)shouldAutoCaptureSessions {
@synchronized (self) {
_shouldAutoCaptureSessions = shouldAutoCaptureSessions;

if (shouldAutoCaptureSessions) { // track any existing sessions
BugsnagSessionTracker *sessionTracker = [Bugsnag notifier].sessionTracker;
[sessionTracker onAutoCaptureEnabled];
Expand All @@ -249,6 +251,22 @@ - (NSDictionary *)sessionApiHeaders {
};
}

- (void)setEndpointsForNotify:(NSString *_Nonnull)notify sessions:(NSString *_Nonnull)sessions {

This comment was marked as resolved.

This comment was marked as resolved.

_notifyURL = [NSURL URLWithString:notify];
_sessionURL = [NSURL URLWithString:sessions];

NSAssert([self isValidUrl:_notifyURL], @"Invalid URL supplied for notify endpoint");

if (![self isValidUrl:_sessionURL]) {
_sessionURL = nil;
}
}

- (BOOL)isValidUrl:(NSURL *)url {
return url != nil && url.scheme != nil && url.host != nil;
}


- (BOOL)hasValidApiKey {
return [_apiKey length] > 0;
}
Expand Down
6 changes: 2 additions & 4 deletions Source/BugsnagNotifier.m
Original file line number Diff line number Diff line change
Expand Up @@ -216,9 +216,6 @@ - (id)initWithConfiguration:(BugsnagConfiguration *)initConfiguration {
hasRecordedSessions = true;
}];


[self.sessionTracker startNewSession:[NSDate date] withUser:nil autoCaptured:YES];

[self metaDataChanged:self.configuration.metaData];
[self metaDataChanged:self.configuration.config];
[self metaDataChanged:self.state];
Expand Down Expand Up @@ -356,9 +353,10 @@ - (void)start {
object:nil];
#endif

_started = YES;

// notification not received in time on initial startup, so trigger manually
[self willEnterForeground:self];
_started = YES;
}

- (void)watchLifecycleEvents:(NSNotificationCenter *)center {
Expand Down
60 changes: 35 additions & 25 deletions Source/BugsnagSessionTracker.m
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
#import "BSG_KSLogger.h"
#import "BugsnagSessionTrackingPayload.h"
#import "BugsnagSessionTrackingApiClient.h"
#import "BugsnagLogger.h"

@interface BugsnagSessionTracker ()
@property BugsnagConfiguration *config;
Expand Down Expand Up @@ -43,6 +44,10 @@ - (instancetype)initWithConfig:(BugsnagConfiguration *)config
- (void)startNewSession:(NSDate *)date
withUser:(BugsnagUser *)user
autoCaptured:(BOOL)autoCaptured {
if (self.config.sessionURL == nil) {
bsg_log_err(@"The session tracking endpoint has not been set. Session tracking is disabled");
return;
}

_currentSession = [[BugsnagSession alloc] initWithId:[[NSUUID UUID] UUIDString]
startDate:date
Expand Down Expand Up @@ -87,34 +92,39 @@ - (void)incrementHandledError {
}

- (void)send {
@synchronized (self.sessionStore) {
NSMutableArray *sessions = [NSMutableArray new];
NSArray *fileIds = [self.sessionStore fileIds];
NSArray *fileIds = [self.sessionStore fileIds];

for (NSDictionary *dict in [self.sessionStore allFiles]) {
[sessions addObject:[[BugsnagSession alloc] initWithDictionary:dict]];
}
BugsnagSessionTrackingPayload *payload = [[BugsnagSessionTrackingPayload alloc] initWithSessions:sessions];

if (payload.sessions.count > 0) {
[self.apiClient sendData:payload
withPayload:[payload toJson]
toURL:self.config.sessionURL
headers:self.config.sessionApiHeaders
onCompletion:^(id data, BOOL success, NSError *error) {

if (success && error == nil) {
NSLog(@"Sent sessions to Bugsnag");

for (NSString *fileId in fileIds) {
[self.sessionStore deleteFileWithId:fileId];
}
} else {
NSLog(@"Failed to send sessions to Bugsnag: %@", error);
if (fileIds.count <= 0) {
return;
}

dispatch_semaphore_t requestSemaphore = dispatch_semaphore_create(0);
NSMutableArray *sessions = [NSMutableArray new];

for (NSDictionary *dict in [self.sessionStore allFiles]) {
[sessions addObject:[[BugsnagSession alloc] initWithDictionary:dict]];
}
BugsnagSessionTrackingPayload *payload = [[BugsnagSessionTrackingPayload alloc] initWithSessions:sessions];

if (payload.sessions.count > 0) {
[self.apiClient sendData:payload
withPayload:[payload toJson]
toURL:self.config.sessionURL
headers:self.config.sessionApiHeaders
onCompletion:^(id data, BOOL success, NSError *error) {
if (success && error == nil) {
NSLog(@"Sent sessions to Bugsnag");

for (NSString *fileId in fileIds) {
[self.sessionStore deleteFileWithId:fileId];
}
}];
}
} else {
NSLog(@"Failed to send sessions to Bugsnag: %@", error);
}
dispatch_semaphore_signal(requestSemaphore);
}];
}
dispatch_semaphore_wait(requestSemaphore, DISPATCH_TIME_FOREVER);
Copy link
Contributor

Choose a reason for hiding this comment

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

What happens here if payload.sessions.count == 0 (and the previous block is never triggered)? Might need to be accounted for to prevent a deadlock.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed by sending a signal to the semaphore immediately in this case.

}

@end
6 changes: 3 additions & 3 deletions Source/BugsnagUser.m
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ @implementation BugsnagUser
- (instancetype)initWithDictionary:(NSDictionary *)dict {
if (self = [super init]) {
_userId = dict[@"id"];
_emailAddress = dict[@"emailAddress"];
_emailAddress = dict[@"email"];

Choose a reason for hiding this comment

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

are the changes in this file related to this PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This addresses an issue in the session request serialisation which was found when adding mazerunner scenarios. Currently the session request sends user.emailAddress rather than user.email, which is corrected in e12ff70.

I can split this off into a separate PR if you think it makes more sense.

Choose a reason for hiding this comment

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

that makes sense, I think it would be good to have it in a separate PR to track it more easily

_name = dict[@"name"];
}
return self;
Expand All @@ -34,12 +34,12 @@ + (instancetype)userWithUserId:(NSString *)userId name:(NSString *)name emailAdd
return [[self alloc] initWithUserId:userId name:name emailAddress:emailAddress];
}


- (NSDictionary *)toJson {
NSMutableDictionary *dict = [NSMutableDictionary new];
BSGDictInsertIfNotNil(dict, self.userId, @"id");
BSGDictInsertIfNotNil(dict, self.emailAddress, @"emailAddress");
BSGDictInsertIfNotNil(dict, self.emailAddress, @"email");
BSGDictInsertIfNotNil(dict, self.name, @"name");
return [NSDictionary dictionaryWithDictionary:dict];
}

@end
52 changes: 51 additions & 1 deletion Tests/BugsnagConfigurationTests.m
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
#import "Bugsnag.h"
#import "BugsnagConfiguration.h"
#import "BugsnagSessionTracker.h"
#import "BugsnagUser.h"

#import <XCTest/XCTest.h>
Expand Down Expand Up @@ -56,7 +57,7 @@ - (void)testNotifyReleaseStagesExcludedSkipsSending {

- (void)testDefaultSessionConfig {
BugsnagConfiguration *config = [BugsnagConfiguration new];
XCTAssertFalse([config shouldAutoCaptureSessions]);
XCTAssertTrue([config shouldAutoCaptureSessions]);
}

- (void)testErrorApiHeaders {
Expand Down Expand Up @@ -87,6 +88,55 @@ - (void)testSessionEndpoints {
XCTAssertEqualObjects(endpoint, config.sessionURL);
}

- (void)testNotifyEndpoint {
BugsnagConfiguration *config = [BugsnagConfiguration new];
XCTAssertEqualObjects([NSURL URLWithString:@"https://notify.bugsnag.com/"], config.notifyURL);
NSURL *endpoint = [NSURL URLWithString:@"http://localhost:8000"];
config.notifyURL = endpoint;
XCTAssertEqualObjects(endpoint, config.notifyURL);
}

- (void)testSetEndpoints {
BugsnagConfiguration *config = [BugsnagConfiguration new];
[config setEndpointsForNotify:@"http://notify.example.com" sessions:@"http://sessions.example.com"];
XCTAssertEqualObjects([NSURL URLWithString:@"http://notify.example.com"], config.notifyURL);
XCTAssertEqualObjects([NSURL URLWithString:@"http://sessions.example.com"], config.sessionURL);
}

- (void)testSetEmptyNotifyEndpoint {
BugsnagConfiguration *config = [BugsnagConfiguration new];
XCTAssertThrowsSpecificNamed([config setEndpointsForNotify:@"" sessions:@"http://sessions.example.com"],
NSException, NSInternalInconsistencyException);

This comment was marked as resolved.

}

- (void)testSetMalformedNotifyEndpoint {
BugsnagConfiguration *config = [BugsnagConfiguration new];
XCTAssertThrowsSpecificNamed([config setEndpointsForNotify:@"http://" sessions:@"http://sessions.example.com"],
NSException, NSInternalInconsistencyException);
}

- (void)testSetEmptySessionsEndpoint {
BugsnagConfiguration *config = [BugsnagConfiguration new];
[config setEndpointsForNotify:@"http://notify.example.com" sessions:@""];
BugsnagSessionTracker *sessionTracker
= [[BugsnagSessionTracker alloc] initWithConfig:config apiClient:nil callback:nil];

XCTAssertNil(sessionTracker.currentSession);
[sessionTracker startNewSession:[NSDate date] withUser:nil autoCaptured:NO];
XCTAssertNil(sessionTracker.currentSession);
}

- (void)testSetMalformedSessionsEndpoint {
BugsnagConfiguration *config = [BugsnagConfiguration new];
[config setEndpointsForNotify:@"http://notify.example.com" sessions:@"f"];
BugsnagSessionTracker *sessionTracker
= [[BugsnagSessionTracker alloc] initWithConfig:config apiClient:nil callback:nil];

XCTAssertNil(sessionTracker.currentSession);
[sessionTracker startNewSession:[NSDate date] withUser:nil autoCaptured:NO];
XCTAssertNil(sessionTracker.currentSession);
}

- (void)testUser {
BugsnagConfiguration *config = [BugsnagConfiguration new];
XCTAssertNil(config.currentUser);
Expand Down
4 changes: 2 additions & 2 deletions Tests/BugsnagUserTest.m
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ - (void)testDictDeserialisation {

NSDictionary *dict = @{
@"id": @"test",
@"emailAddress": @"fake@example.com",
@"email": @"fake@example.com",

This comment was marked as outdated.

@"name": @"Tom Bombadil"
};
BugsnagUser *user = [[BugsnagUser alloc] initWithDictionary:dict];
Expand All @@ -41,7 +41,7 @@ - (void)testPayloadSerialisation {
XCTAssertEqual(3, [rootNode count]);

XCTAssertEqualObjects(@"test", rootNode[@"id"]);
XCTAssertEqualObjects(@"fake@example.com", rootNode[@"emailAddress"]);
XCTAssertEqualObjects(@"fake@example.com", rootNode[@"email"]);
XCTAssertEqualObjects(@"Tom Bombadil", rootNode[@"name"]);
}

Expand Down
8 changes: 4 additions & 4 deletions examples/swift-ios/Podfile.lock
Original file line number Diff line number Diff line change
@@ -1,16 +1,16 @@
PODS:
- Bugsnag (5.15.4)
- Bugsnag (5.15.6)

DEPENDENCIES:
- Bugsnag (from `../..`)

EXTERNAL SOURCES:
Bugsnag:
:path: ../..
:path: "../.."

SPEC CHECKSUMS:
Bugsnag: 904211a0230f254808b47f3adb4b684900772962
Bugsnag: ff5f5e3059e6a9c9d27a899f3bf3774067553483

PODFILE CHECKSUM: 2107babfbfdb18f0288407b9d9ebd48cbee8661c

COCOAPODS: 1.4.0
COCOAPODS: 1.5.0
4 changes: 2 additions & 2 deletions features/fixtures/ios-swift-cocoapods/Podfile.lock
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
PODS:
- Bugsnag (5.15.4)
- Bugsnag (5.15.6)

DEPENDENCIES:
- Bugsnag (from `../../..`)
Expand All @@ -9,7 +9,7 @@ EXTERNAL SOURCES:
:path: ../../..

SPEC CHECKSUMS:
Bugsnag: 904211a0230f254808b47f3adb4b684900772962
Bugsnag: ff5f5e3059e6a9c9d27a899f3bf3774067553483

PODFILE CHECKSUM: 4d026fb83571f098c9fb4fa71c1564c72c55ab1a

Expand Down
Loading