On by default session tracking #286

fractalwrench · 2018-06-06T09:15:15Z

Goal

Enables automatic tracking of sessions by default, and adds a new API for setting the notify/session endpoints.

Design

This should prevent the unintentional scenario where a user sets the notify endpoint with a sessions endpoint.

Changeset

Added

setEndpointsForNotify:sessions:

Removed

Changed

Updated docs for sessionURL and notifyURL to indicate they should be only used to read the value, not write.
Auto capture flag is now true by default

Discussion

Outstanding Questions

What should our approach be to deprecating the notifyURL and sessionURL properties? We may want to keep these properties to allow users to retrieve the value they have set. One option would be to deprecate them entirely, another would be to change to readonly rather than readwrite.
What is our approach for writing mazerunner scenarios? We're missing scenarios for the session tracking feature entirely, so presumably we need to write these in addition to the new test cases in the design spec. Cocoa mazerunner scenarios also don't currently work on my machine, and I'm concerned that waiting 20-30 minutes for CI to run the scenarios is a very long feedback loop.

Review

For the submitter, initial self-review:

Commented on code changes inline explain the reasoning behind the approach
Reviewed the test cases added for completeness and possible points for discussion
A changelog entry was added for the goal of this pull request
Check the scope of the changeset - is everything in the diff required for the pull request?
This pull request is ready for:
- Initial review of the intended approach, not yet feature complete
- Structural review of the classes, functions, and properties modified
- Final review

For the pull request reviewer(s), this changeset has been reviewed for:

Consistency across platforms for structures or concepts added or modified
Consistency between the changeset and the goal stated above
Internal consistency with the rest of the library - is there any overlap between existing interfaces and any which have been added?
Usage friction - is the proposed change in usage cumbersome or complicated?
Performance and complexity - are there any cases of unexpected O(n^3) when iterating, recursing, flat mapping, etc?
Concurrency concerns - if components are accessed asynchronously, what issues will arise
Thoroughness of added tests and any missing edge cases
Idiomatic use of the language

…figuration

…hich should pass existing verif

…cocoa

fractalwrench · 2018-06-07T13:21:39Z

I've now added some mazerunner scenarios which should add coverage for session tracking. I believe I've found a couple of issues, the first being that the session request sends user.emailAddress rather than user.email. I've corrected this in e12ff70.

The second issue is that the AutoSessionScenario scenario appears to receive 2 requests, rather than 1. I'm not 100% sure why this occurs, but it's possible that (willEnterForeground)[https://github.com/bugsnag/bugsnag-cocoa/blob/master/Source/BugsnagNotifier.m#L360] is called multiple times, resulting in a duplicate number of sessions. I've created a separate ticket to investigate this further.

martin308

Added some comments on some things that might be good to look into

martin308 · 2018-06-11T20:08:00Z

Source/BugsnagConfiguration.h

- * 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.


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

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.

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

Might be worth trying:

Mark both properties as readonly

Add setter methods

Mark setters as deprecated

martin308 · 2018-06-11T20:08:06Z

Source/BugsnagConfiguration.h

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


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

martin308 · 2018-06-11T20:09:35Z

Source/BugsnagUser.m

@@ -14,7 +14,7 @@ @implementation BugsnagUser
 - (instancetype)initWithDictionary:(NSDictionary *)dict {
    if (self = [super init]) {
        _userId = dict[@"id"];
-        _emailAddress = dict[@"emailAddress"];
+        _emailAddress = dict[@"email"];


are the changes in this file related to this PR?

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.

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

martin308 · 2018-06-11T20:13:44Z

Tests/BugsnagConfigurationTests.m

+        BugsnagConfiguration *config = [BugsnagConfiguration new];
+        [config setEndpointsForNotify:@"" sessions:@"http://sessions.example.com"];
+        XCTFail();
+    } @catch(NSException * e) {


is it better to use something like XCTAssertThrowsSpecific here?

martin308 · 2018-06-11T20:13:52Z

Tests/BugsnagConfigurationTests.m

+        BugsnagConfiguration *config = [BugsnagConfiguration new];
+        [config setEndpointsForNotify:@"http://" sessions:@"http://sessions.example.com"];
+        XCTFail();
+    } @catch(NSException * e) {


is it better to use something like XCTAssertThrowsSpecific here?

Tests/BugsnagUserTest.m

@@ -19,7 +19,7 @@ - (void)testDictDeserialisation {

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


…ertions

Source/BugsnagConfiguration.m

@@ -249,6 +251,22 @@ - (NSDictionary *)sessionApiHeaders {
             };
 }

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


Tests/BugsnagConfigurationTests.m

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


Adds a semaphore that synchronises sending of sessions. This prevents the potential case where a session could be sent multiple times, and passes mazerunner session count scenarios

…flect nil by default

fractalwrench · 2018-06-14T15:56:07Z

I believe the outstanding issues with this PR are the question of what to do with the notifyURL and sessionURL properties, which it'd be good to get @kattrali's opinion on.

I found a few issues with the existing Cocoa session tracking implementation, which I've addressed in this PR and need review:

BugsnagUser incorrectly serialised the email address as user.emailAddress, whereas the Session Tracking API expects user.email. The fix has been to change the key in the relevant dictionary.
[BugsnagNotifier start] sets a boolean property self.started to indicate when the notifier has been fully initialised. Within this method, [self willEnterForeground:] is called to track an initial session, which resulted in a NOP. The fix here was to set the flag to true before starting the first session.
There appears to be an extra session recorded within [[BugsnagNotifier alloc] init], which has been removed so that only 1 session is captured on initialisation by default.
There is a potential for [BugsnagSessionTracker send] to send the same session multiple times. Although the method uses @synchronize, the underlying BugsnagApiClient uses NSURLSession, which is asynchronous. The proposed fix is to use a semaphore to control access, and to signal within the completion block when the request has finished. I'm not convinced that this is a likely scenario for a real-world application as I've only seen it in mazerunner scenarios which have their own hooks to flush sessions (reducing the time spent waiting) - however, it would be nice to address it.

kattrali

Looks good overall, had some concerns around concurrency and the test setup.

kattrali · 2018-06-15T17:52:30Z

features/fixtures/ios-swift-cocoapods/iOSTestApp/scenarios/AutoSessionScenario.m

+}
+
+- (void)run {
+    [self flushAllSessions];


By manually calling flush, this doesn't test whether automatically enabled sessions will be sent in a regular app environment. For example, if there is a problem where the session tracker never calls send, the test will pass but the session will never be sent in a release. Rather than calling it directly, the scenario should wait however long is reasonably expected for the session to be sent automatically. The same principle applies to the other scenarios, such as calling startSession; waiting for the expected behavior rather than forcing it to happen immediately reflects actual usage.

My main concern with this approach is that if we apply it to all the session scenarios, it's going to lead to very long CI build times, as it can take 60s for this to occur. I've updated the scenarios to send sessions naturally for now, but think we'll have to revisit this approach if it gets to the point where we have >5 session scenarios.

Can you shorten the interval in tests? It sounds similar to something that I did in the tests bugsnag-node.

Also did some casual time travelling https://github.com/bugsnag/bugsnag-node/blob/7f4ee1c1219216b9f19ab7ce5ea75c33e555388d/test/sessions/tracker.js#L12

kattrali · 2018-06-15T17:58:48Z

features/fixtures/ios-swift-cocoapods/iOSTestApp/scenarios/AutoSessionScenario.m

+@implementation AutoSessionScenario
+
+- (void)startBugsnag {
+    self.config.shouldAutoCaptureSessions = YES;


Shouldn't this be unneeded since YES is the default?

Ordinarily this would be true, but the config object passed to the scenario has shouldAutoCaptureSessions set to false. This avoids needing to update all the other scenarios to anticipate an additional request.

features/fixtures/ios-swift-cocoapods/iOSTestApp/scenarios/DisabledSessionTrackingScenario.m

+@implementation DisabledSessionTrackingScenario
+
+- (void)run {
+    [self flushAllSessions];


kattrali · 2018-06-15T18:07:13Z

Source/BugsnagSessionTracker.m

    }
+    dispatch_semaphore_wait(requestSemaphore, DISPATCH_TIME_FOREVER);


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.

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

…g semaphore

…ng rather than manual hooks

fractalwrench · 2018-06-18T11:16:11Z

Addressed the inline feedback and added a changelog entry. I've decided to make notifyURL and sessionURL readonly, as the suggested approach in the comments wasn't successful.

kattrali · 2018-06-18T11:46:45Z

Adding - (void)setNotifyURL etc wasn’t successful?

fractalwrench · 2018-06-18T12:19:55Z

@kattrali nope - not in the example Swift project. I attempted adding:

- (void)setNotifyURL:(NSURL *)url {
    _notifyURL = url;
}

Then ran pod install, and attempted to compile config.notifyURL = URL(string: "")

kattrali · 2018-06-18T12:39:08Z

What did the message say? Could be something that needs to be aliased for swift. If resolving that doesn’t work, it would make more sense to skip setting readonly to avoid the breaking change and keep the setters as deprecated.

fractalwrench · 2018-06-18T12:49:21Z

@kattrali the exact error message is: Error:(30, 42) cannot assign to property: 'notifyURL' is a get-only property

For the following declaration: - (void)setNotifyURL:(NSURL *)url NS_SWIFT_NAME(notifyURL);

It seems to compile ok for Objective-C, although there is a lint warning.

kattrali · 2018-06-18T12:59:44Z

I see. And if you don’t change the readwrite property but add the setter?

fractalwrench · 2018-06-18T13:15:05Z

@kattrali that seems to work, although there's another warning:

Writable atomic property 'notifyURL' cannot pair a synthesized getter with a user defined setter

I can ignore that and proceed with the above approach if you're happy?

kattrali · 2018-06-18T13:32:16Z

Haha my happiness is compiler happiness + making usage intent clear :)

Does defining the getter + making the setter deprecated resolve everything?

fractalwrench · 2018-06-18T14:37:42Z

@kattrali for Objective-C yes, that resolves the issues, but the deprecation notice isn't shown in Swift.

…ugsnag/bugsnag-cocoa into on-by-default-session-tracking

fractalwrench · 2018-06-20T10:13:33Z

After discussing this with @snmaynard we decided that of the options below, (2) would be the best:

Keep the properties as readwrite and document not to use them
Make the properties readonly (breaking change)
Deprecate the properties entirely

I'm not aware of anything else outstanding on this PR.

CodeHex · 2018-06-20T13:08:28Z

Tests/BugsnagSinkTests.m

@@ -39,7 +39,7 @@ - (void)setUp {
    config.autoNotify = NO;
    config.apiKey = @"apiKeyHere";
    config.releaseStage = @"MagicalTestingTime";
-    config.notifyURL = nil;
+    [config setEndpointsForNotify:@"http://localhost:8000" sessions:@"http://localhost:8000"];


The notifyURL was cleared previous, but now this is being set to localhost:8000, is this correct? Also sessions endpoint is localhost:10000

$fractalwrench$

fractalwrench added 12 commits May 14, 2018 15:25

$@fractalwrench$

feat: add setEndpoints interface to configuration

831f63d

$@fractalwrench$

test: Add test for legacy notifyURL property

dceac81

$@fractalwrench$

test: test setEndpoints notify url validation

b6d16bf

$@fractalwrench$

feat: Ignore tracking session if invalid session URL supplied

48e267f

$@fractalwrench$

feat: enable session tracking by default

b3d5420

$@fractalwrench$

refactor: use nsassert rather than nsexception.raise for endpoint con…

abb08c2

…figuration

$@fractalwrench$

test: make mazerunner scenario NOT autocapture sessions by default, w…

4b7277d

…hich should pass existing verif

$@fractalwrench$

docs: update documentation of session tracking on by default

f3922c6

$@fractalwrench$

use string rather than url for mazerunner scenario

26a2c37

$@fractalwrench$

docs: update session tracking obd docs

57722ed

$@fractalwrench$

add missing comma in method sig

0645bad

$@fractalwrench$

fix minor docs typo

8980ea3

$@fractalwrench$ fractalwrench requested review from kattrali and a team June 6, 2018 09:15

fractalwrench added 4 commits June 7, 2018 11:12

$@fractalwrench$

test: begin porting android session tracking mazerunner scenarios to …

0677b66

…cocoa

$@fractalwrench$

test: add manual and disabled session tracking scenarios

fda541e

$@fractalwrench$

fix: serialise user email using 'email' key for session requests

e12ff70

$@fractalwrench$

test: fix user serialisation test

d813cef

$@fractalwrench$

Merge branch 'next' into on-by-default-session-tracking

c89979d

martin308 reviewed Jun 11, 2018

View reviewed changes

$@fractalwrench$

refactor: use xctassertthrows rather than try/catch block in test ass…

ec307f1

…ertions

martin308 reviewed Jun 12, 2018

View reviewed changes

fractalwrench added 2 commits June 13, 2018 15:26

$@fractalwrench$

fix: prevent duplicate automatic session count in constructor

e84d311

$@fractalwrench$

fix: ensure session is captured on notifier start

0ff92ec

$@fractalwrench$ fractalwrench added the wip label Jun 14, 2018

fractalwrench added 2 commits June 14, 2018 15:02

$@fractalwrench$

fix: use semaphore to synchronise access to session flushing

57044de

Adds a semaphore that synchronises sending of sessions. This prevents the potential case where a session could be sent multiple times, and passes mazerunner session count scenarios

$@fractalwrench$

test: update mazerunner auto session scenario user.id assertion to re…

348e095

…flect nil by default

$@fractalwrench$ fractalwrench removed the wip label Jun 15, 2018

kattrali reviewed Jun 15, 2018

View reviewed changes

fractalwrench added 4 commits June 18, 2018 11:18

$@fractalwrench$

fix: prevent potential deadlock when payload count == 0, by signallin…

3bf9f72

…g semaphore

$@fractalwrench$

test: update mazerunner session scenarios to rely on automatic flushi…

8a65049

…ng rather than manual hooks

$@fractalwrench$

feat: make notifyURL and sessionURL readonly

f4bffac

$@fractalwrench$

docs: add changelog entry for cocoa session tracking

19d23a4

$@fractalwrench$

Merge branch 'next' into on-by-default-session-tracking

70c6e76

fractalwrench added 2 commits June 20, 2018 09:59

$@fractalwrench$

test: pass readonly notifyURL + sessionURL tests

00bb48b

$@fractalwrench$

Merge branch 'on-by-default-session-tracking' of https://github.com/b…

8bd994f

…ugsnag/bugsnag-cocoa into on-by-default-session-tracking

CodeHex reviewed Jun 20, 2018

View reviewed changes

$@fractalwrench$

docs: note that overriding endpoints is for test purposes only

d05198d

CodeHex approved these changes Jun 21, 2018

View reviewed changes

$@fractalwrench$ fractalwrench merged commit dbc380f into next Jun 21, 2018

$@fractalwrench$ fractalwrench deleted the on-by-default-session-tracking branch June 21, 2018 09:48

		}
		dispatch_semaphore_wait(requestSemaphore, DISPATCH_TIME_FOREVER);

On by default session tracking #286

On by default session tracking #286

Conversation

fractalwrench commented Jun 6, 2018 • edited Loading

Goal

Design

Changeset

Added

Removed

Changed

Discussion

Outstanding Questions

Review

fractalwrench commented Jun 7, 2018

martin308 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kattrali Jun 15, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as outdated.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

fractalwrench commented Jun 14, 2018

kattrali left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as resolved.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fractalwrench commented Jun 18, 2018

kattrali commented Jun 18, 2018

fractalwrench commented Jun 18, 2018

kattrali commented Jun 18, 2018

fractalwrench commented Jun 18, 2018

kattrali commented Jun 18, 2018

fractalwrench commented Jun 18, 2018

kattrali commented Jun 18, 2018

fractalwrench commented Jun 18, 2018

fractalwrench commented Jun 20, 2018

Choose a reason for hiding this comment

$@fractalwrench$ fractalwrench commented Jun 6, 2018 •

edited

Loading

kattrali Jun 15, 2018 •

edited

Loading