Fix session delivery logic to always use the delivery queue #295

kattrali · 2018-07-13T15:09:23Z

Goal

Streamline session delivery, ensuring requests are sent on the API client delivery queue.

Changeset

`SessionTracker`

Moved the event handling for entering/leaving the foreground here
Separated starting a session manually from automatic session tracking logic
Removed delivery logic

`Notifier`

Manually starts a new session in the start() method
Calls new session tracker event hooks

`SessionTrackingApiClient`

Moved all of the logic for delivering sessions here

Tests

Added new unit test cases for starting sessions manually vs auto-capture to ensure sessions are only recorded when shouldAutoCaptureSessions is true.
Refactored integration test cases for starting sessions and automatic session tracking to test the default behavior without setting shouldAutoCaptureSessions

Discussion

This changeset removes the logic in BugsnagConfiguration to start a session if no session has ever been started in the setter for shouldAutoCaptureSessions because of a possible race condition. The inline docs are updated accordingly, but is this behavior needed?

Alternative Approaches

I considered only doing the SessionTrackingApiClient change, however the larger changeset makes the flow easier to follow and removes the timer logic. This is still a possibility if reviewing a smaller changeset is preferable.

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

* Remove timer logic * Remove unused properties * Separate capturing an auto-session from a manual one * Move event handling logic into session tracker. Encapsulates the logic for determining whether to record/send a new session. * Rename tracker event handlers to be consistent with each other * Move delivery logic into API client, remove store semaphore

If autoCaptureSessions is updated after Bugsnag is started, only capture subsequent sessions rather than trying to retrofit an existing one. This reduces the possibility of making assumptions about whether Bugsnag has started already.

Updated individual scenarios to reflect that session tracking is not accounted for in the request count. Updated AppDelegate scenario loading logic to always use the same configuration

fractalwrench

I left a few comments inline, mostly I think this makes sense.

I think it'd be good to test what the effect of these changes are on React Native/Unity, and also to clarify whether entering the foreground can record a duplicate session.

fractalwrench · 2018-07-13T15:19:39Z

Source/BugsnagConfiguration.m

-    @synchronized (self) {
-        _shouldAutoCaptureSessions = shouldAutoCaptureSessions;
-
-        if (shouldAutoCaptureSessions) { // track any existing sessions


IIRC this logic (or similar) may have been added to enable automatic session tracking on React Native. Have we tested the effects of these changes on downstream notifiers?

bugsnag-react-native will need a slight tweak in the initialization logic. While configuring the JS options, when all of the following are true:

Bugsnag has already been configured on the native side

The native configuration has shouldAutoCaptureSessions set to NO

The JS configuration has autoCaptureSessions set to true

Then [BugsnagReactNative start] should call startSession

@martin308 do you foresee a problem with this on the unity side? My hunch is that it’s even less of a problem since the automatic session logic now lives on the C# side.

Yeh there wouldn't be an issue with it the way it is setup right now. For the native notifiers we turn off automatic session tracking

fractalwrench · 2018-07-13T15:23:09Z

Source/BugsnagSessionTracker.h

 */
-@property SessionTrackerCallback callback;
+- (void)handleHandledErrorEvent;


Would it also make sense to have an unhandled equivalent of this method?

Not really; if an unhandled event occurs, the app is crashing. There can’t be more than 1 unhandled event in a single session.

fractalwrench · 2018-07-13T15:26:43Z

Source/BugsnagNotifier.m

-                                                        repeats:YES];
-        [self sessionTick:self];
-    }
+    [self.sessionTracker handleAppForegroundEvent];


Will this capture 2 sessions on application startup rather than 1? startNewSessionIfAutoCaptureEnabled will also start a session, unless I'm mistaken.

It only captures 1 event, since the foreground session only happens when coming from the background and having backgroundTime more than 60s before.

fractalwrench · 2018-07-13T15:32:15Z

Source/BugsnagSessionTracker.m

    }
+    self.backgroundStartTime = nil;


If this can be nil and we're expecting sessions to be started from multiple threads, would it make sense to make this property atomic/make any checks compound operations?

While sessions can be started from multiple threads, the notifications for UIApplicationWillEnterForeground/Background only happen on the main thread and are the only place invoking these handlers. We can document this though and enforce it with assertions in debug mode though.

fractalwrench · 2018-07-13T15:33:57Z

Source/BugsnagSessionTrackingApiClient.m

+- (void)deliverSessionsInStore:(BugsnagSessionFileStore *)store {
+    [self.sendQueue addOperationWithBlock:^{
+        if (!self.config.apiKey)
+            return;


Is it worth logging a warning if this is the case? I believe this state shouldn't really be allowed.

Logging is a good idea. It shouldn’t happen but this is a guard to prevent either crashing or sending an invalid payload if it hasn’t been set.

fractalwrench · 2018-07-13T15:37:44Z

Tests/BugsnagSessionTrackerTest.m

-    [self.sessionTracker startNewSession:[NSDate date]
-                                withUser:self.user
-                            autoCaptured:NO];
+    [self.sessionTracker startNewSession];

    BugsnagSession *secondSession = self.sessionTracker.currentSession;
    XCTAssertNotEqualObjects(firstSession.sessionId, secondSession.sessionId);
 }

 - (void)testIncrementCounts {


Is it possible to test unhandled counts? Or is that already covered elsewhere?

It needs an integration test case. I’ll note to add one.

fractalwrench · 2018-07-13T15:38:17Z

features/fixtures/ios-swift-cocoapods/iOSTestApp/AppDelegate.swift

@@ -15,6 +15,7 @@ class AppDelegate: UIResponder, UIApplicationDelegate {
        let arguments = ProcessInfo.processInfo.arguments
        var delay: TimeInterval = 0
        var eventType = "none"
+        var eventMode = "regular"


What's the difference between an event type and an event mode? Do they need to be separate concepts?

There needs to be separation between “run this scenario’s crashing code” and “configure Bugsnag for this scenario (and not run the crashy code)”. Without this, an individual scenario can’t configure Bugsnag for relaunching the app.

fractalwrench · 2018-07-13T15:40:25Z

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

@@ -32,6 +32,11 @@
 */
 @implementation ReadGarbagePointerScenario

+- (void)startBugsnag {
+    self.config.shouldAutoCaptureSessions = NO;


Is it worth making this the default in the mazerunner fixture's AppDelegate, seeing as most of the scenarios already assume this?

No, because having it there made it difficult to test the default behavior for session tracking. For example, the auto-capture scenarios should test the behavior when not setting the value at all to ensure the default behavior is to send a session request. It’s also easier to debug an individual case when only setting the minimal values for running the scenario by default. I lost a bit of time to confusion when I couldn’t figure out where a config value was being set between AppDelegate, Scenario, and the Scenario subclass.

kattrali · 2018-07-16T14:18:16Z

I think the review feedback has been addressed:

Added logging to the unlikely error cases
Documented the main thread-based hooks
Added tests for the handled/unhandled event counts in report payloads

kattrali added 4 commits July 13, 2018 10:43

chore: Delete unused duplicate files

7bfe374

fix: Remove config session tracking logic

371040c

If autoCaptureSessions is updated after Bugsnag is started, only capture subsequent sessions rather than trying to retrofit an existing one. This reduces the possibility of making assumptions about whether Bugsnag has started already.

refactor: Remove unneeded argument from file store init

b148a55

kattrali changed the title ~~Kattrali/fix session delivery queue~~ Fix session delivery logic to always use the delivery queue Jul 13, 2018

kattrali requested a review from a team July 13, 2018 15:16

tests: Use default value for auto-capturing sessions by default

5a77351

Updated individual scenarios to reflect that session tracking is not accounted for in the request count. Updated AppDelegate scenario loading logic to always use the same configuration

kattrali force-pushed the kattrali/fix-session-delivery-queue branch from 567cea8 to 5a77351 Compare July 13, 2018 15:27

$fractalwrench$

fractalwrench reviewed Jul 13, 2018

View reviewed changes

kattrali added 4 commits July 13, 2018 19:46

tests: Add checks for handled counts in payloads

a9e30b9

docs: Document app forground state hooks require main thread

7315fcb

tests: Add test for unhandled counts

fbd907b

docs: Log if API client is in an invalid state

7ed032b

kattrali force-pushed the kattrali/fix-session-delivery-queue branch from 9681e17 to 7ed032b Compare July 16, 2018 13:13

kattrali requested a review from a team July 16, 2018 14:18

$fractalwrench$

fractalwrench approved these changes Jul 16, 2018

View reviewed changes

docs: Add changelog entry

8e6db0f

kattrali merged commit 9387174 into master Jul 16, 2018

kattrali deleted the kattrali/fix-session-delivery-queue branch July 16, 2018 16:20

This was referenced Jul 17, 2018

Ensure sessions are sent in the background #294

Closed

Fix possible release stage mismatch between sessions and error reports bugsnag/bugsnag-react-native#260

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix session delivery logic to always use the delivery queue #295

Fix session delivery logic to always use the delivery queue #295

kattrali commented Jul 13, 2018 •

edited

Loading

$@fractalwrench$ fractalwrench left a comment

$@fractalwrench$ fractalwrench Jul 13, 2018

kattrali Jul 13, 2018

kattrali Jul 13, 2018

martin308 Jul 13, 2018

$@fractalwrench$ fractalwrench Jul 13, 2018

kattrali Jul 13, 2018

$@fractalwrench$ fractalwrench Jul 13, 2018

kattrali Jul 13, 2018

$@fractalwrench$ fractalwrench Jul 13, 2018

kattrali Jul 13, 2018

$@fractalwrench$ fractalwrench Jul 13, 2018

kattrali Jul 13, 2018

$@fractalwrench$ fractalwrench Jul 13, 2018

kattrali Jul 13, 2018

$@fractalwrench$ fractalwrench Jul 13, 2018

kattrali Jul 13, 2018 •

edited

Loading

$@fractalwrench$ fractalwrench Jul 13, 2018

kattrali Jul 13, 2018

kattrali commented Jul 16, 2018

Fix session delivery logic to always use the delivery queue #295

Fix session delivery logic to always use the delivery queue #295

Conversation

kattrali commented Jul 13, 2018 • edited Loading

Goal

Changeset

SessionTracker

Notifier

SessionTrackingApiClient

Tests

Discussion

Alternative Approaches

Review

fractalwrench 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kattrali Jul 13, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kattrali commented Jul 16, 2018

kattrali commented Jul 13, 2018 •

edited

Loading

`SessionTracker`

`Notifier`

`SessionTrackingApiClient`

$@fractalwrench$ fractalwrench left a comment

kattrali Jul 13, 2018 •

edited

Loading