Enable automatic session tracking by default #314

fractalwrench · 2018-05-10T10:01:43Z

Goal

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

Design

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

Changeset

Added

config.setEndpoints(String notify, String sessions)

Changed

Deprecation notice for:
config.setEndpoint
config.setSessionsEndpoint
client.setEndpoint

Manifest meta-data parsing logic

Tests

An additional Mazerunner test for ensuring a session is automatically tracked on app launch, and unit tests for ensuring:

Setting autoCaptureSessions to false prevents a session from being sent at app load
Manually calling sendSession() sends a session regardless of the state of autoCaptureSessions
Manually calling sendSession() with endpoint configured but not sessionsEndpoint issues a warning and does not send a session

This doesn't automatically test that configuring endpoint without session endpoint logs a warning, as this is difficult to achieve on Android.

Discussion

Outstanding Questions

The deprecated options are not aliased onto the new API - I believe this is ok.

N.B. CI may currently fail due to unrelated flaky tests which are addressed in a separate PR.

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

…vered elsewhere)

…e state of autoCaptureSessions

…bugsnag-android into enable-session-tracking

coveralls · 2018-05-10T12:56:41Z

Coverage increased (+0.6%) to 77.014% when pulling 3334063 on enable-session-tracking into f76d408 on next.

coveralls · 2018-05-10T12:56:41Z

Coverage increased (+0.3%) to 77.756% when pulling 6432b4c on enable-session-tracking into 48a807f on next.

kattrali

Short and sweet 🎉

kattrali · 2018-05-11T05:21:59Z

sdk/src/main/java/com/bugsnag/android/Configuration.java

+            autoCaptureSessions = false;
+        }
+        this.endpoint = notify;
+        this.sessionEndpoint = sessions;


if invalidSessionsEndpoint is true, then shouldn't this be set to null? Whether to send a session could then be determined by whether the endpoint is null rather than adding a new property.

It would also reduce the odds of unexpected behavior if Bugsnag.setAutoCaptureSessions() is called after configuration.

… flag when checking whether to

fractalwrench · 2018-05-11T10:14:40Z

Refactored to check that sessionEndpoint is null rather than an adding an additional boolean flag, when starting a new sessions.

Also, here is the related Docs PR: https://github.com/bugsnag/docs.bugsnag.com/pull/720

Cawllec · 2018-05-16T10:20:55Z

sdk/src/main/java/com/bugsnag/android/Client.java

        }

        config.setSendThreads(data.getBoolean(MF_SEND_THREADS, true));
        config.setPersistUserBetweenSessions(
            data.getBoolean(MF_PERSIST_USER_BETWEEN_SESSIONS, false));
-        config.setAutoCaptureSessions(data.getBoolean(MF_AUTO_CAPTURE_SESSIONS, false));
+
+        if (data.containsKey(MF_AUTO_CAPTURE_SESSIONS)) {


What's the advantage of this over the previous with the default set to true?

This method no longer requires any knowledge of whether sessions should be automatically captured or not, and relies on Configuration instead to specify a sensible default.

Cawllec · 2018-05-16T10:24:13Z

sdk/src/main/java/com/bugsnag/android/Configuration.java

+        if (invalidSessionsEndpoint) {
+            Logger.warn("The session tracking endpoint has not been set. "
+                + "Session tracking is disabled");
+            autoCaptureSessions = false;


Does this require a this. prefixing it as below?

It's not necessarily required but I think that's a good suggestion as it makes the semantics clearer that autoCaptureSessions is a field, and would be more consistent with the rest of the method.

Cawllec · 2018-05-16T10:47:30Z

sdk/src/main/java/com/bugsnag/android/Configuration.java

+        } else {
+            this.sessionEndpoint = sessions;
+        }
+        this.autoCaptureSessions = !invalidSessionsEndpoint;


If this is being set in the if/else above, does it need to be set again here? It may end up being set to true after it's been manually disabled.

Both are not required, I've removed autoCaptureSessions = false

There could be an issue where someone sets autoCaptureSessions to false, then inputs a valid sessions endpoint. The:

this.autoCaptureSessions = !invalidSessionsEndpoint;

would then incorrectly set the autoCaptureSessions back to true, overriding the user's configuration. Is that a possibility?

Good point - I've added an explicit test case for this scenario.

…d, add explicit test coverage

Cawllec · 2018-05-16T14:41:22Z

sdk/src/main/java/com/bugsnag/android/SessionTracker.java

@@ -64,6 +64,11 @@
     * @param user the session user (if any)
     */
    void startNewSession(@NonNull Date date, @Nullable User user, boolean autoCaptured) {
+        if (configuration.getSessionEndpoint() == null) {
+            Logger.warn("The session tracking endpoint has not been set. "


Should have mentioned this earlier, sorry - Should this warning be a one-time thing? We could end up adding a lot of logger noise.

The Logger class will automatically disable itself in production if that's a concern? The default implementation of session tracking waits at least 30 seconds before starting a new session on Android, so for the default use case I'd imagine this message would only be logged once or twice.

I guess that's ok, are there any similar cases already present in the notifier?

This one is somewhat similar.

I mean for the logging repeatedly when in debug mode?

@Cawllec it's not particularly easy to test log messages in Android unfortunately

martin308

Added one suggestion and one question

martin308 · 2018-05-16T17:43:31Z

sdk/src/main/java/com/bugsnag/android/Configuration.java

+     *
+     * @throws IllegalArgumentException if the notify endpoint is empty or null
+     */
+    @SuppressWarnings("ConstantConditions")


Might be worth adding a comment on the reasoning behind suppressing specific warnings

Added an inline comment

martin308 · 2018-05-16T17:47:22Z

sdk/src/main/java/com/bugsnag/android/Configuration.java

+     */
+    @SuppressWarnings("ConstantConditions")
+    public void setEndpoints(@NonNull String notify, @NonNull String sessions)
+        throws IllegalArgumentException {


This seems like a big change in behavior? The other methods on configuration just accept whatever was set

IIRC the design doc for this change states that we should throw an exception if notify is not a valid URL, and log a warning if sessions is not. The rationale being that the notifier can't really do anything useful if the notify endpoint has been misconfigured, but can still send errors if session tracking is not in use. I think it makes sense to fail fast in this instance if a programmer supplies an invalid value.

Cawllec · 2018-05-17T09:33:17Z

Looks good, just the CI that's not working ATM.

fractalwrench · 2018-05-17T13:35:17Z

CI has passed now (the test flakiness seems to be back on Travis)

Cawllec

LGTM assuming @martin308 and @kattrali are now happy

martin308 · 2018-05-17T17:56:46Z

sdk/src/main/java/com/bugsnag/android/Configuration.java

+     *
+     * @throws IllegalArgumentException if the notify endpoint is empty or null
+     */
+    @SuppressWarnings("ConstantConditions") // ignore checkstyle nullability warnings


I was thinking more along the lines of the reasoning behind suppressing it. I'm not sure what this does exactly but here is a possibly terrible example "Suppress this warning as we know that the "notify" parameter can be null due to this amazing reason... but we can ignore it because of some other scenario..."

I've removed this annotation as it appears it's no longer needed. I believe it was originally included because a null check was performed on notify and sessions, which are annotated as non-null. Checkstyle picks up the nullability annotation, but unfortunately this isn't enforced by the compiler for Java, so it made sense to perform a null check.

This code now uses TextUtils.isEmpty, which also checks for an empty string along with null, which isn't a constant condition.

bengourley · 2018-05-31T09:53:07Z

CHANGELOG.md

@@ -1,5 +1,13 @@
 # Changelog

+## 4.X.X (TBD)
+
+**Note**: this release alters the behaviour of the notifier to track session automatically. If you


I'm reusing this copy for the js changelog and noticed a little typo:

- track session automatically + track sessions automatically

…scenarios Create additional E2E scenarios for Unity SO file upload

Two stage builds

$fractalwrench$

fractalwrench added 11 commits May 9, 2018 14:40

$@fractalwrench$

feat: enable session tracking by default

461eebb

$@fractalwrench$

test: add tests for setEndpoints behaviour

307270c

$@fractalwrench$

feat: implement setEndpoints config option

59a8164

$@fractalwrench$

feat: enable on-by-default for manifest configuration

1b14ae9

$@fractalwrench$

test: fix auto context test to disable automatic session tracking (co…

711ab74

…vered elsewhere)

$@fractalwrench$

docs: document deprecated config apis

6886111

$@fractalwrench$

docs: add javadoc for new setEndpoints API

84efa1e

$@fractalwrench$

test: Manually calling sendSession() sends a session regardless of th…

e276037

…e state of autoCaptureSessions

$@fractalwrench$

test: A single app load sends a single session by default

8e38483

$@fractalwrench$

docs: add changelog for on-by-default session tracking

f89c322

$@fractalwrench$

Merge branch 'master' into enable-session-tracking

145c45a

$@fractalwrench$ fractalwrench changed the base branch from master to next May 10, 2018 12:31

fractalwrench added 2 commits May 10, 2018 13:34

$@fractalwrench$

style: pass checkstyle for android on-by-default changes

4dcf7ad

$@fractalwrench$

Merge branch 'enable-session-tracking' of https://github.com/bugsnag/…

3334063

…bugsnag-android into enable-session-tracking

$@fractalwrench$ fractalwrench requested a review from a team May 10, 2018 12:35

kattrali reviewed May 11, 2018

View reviewed changes

$@fractalwrench$

refactor: check if sessionsEndpoint is null rather than extra boolean…

66ce062

… flag when checking whether to

$@fractalwrench$ fractalwrench requested a review from a team May 11, 2018 16:12

Cawllec reviewed May 16, 2018

View reviewed changes

fractalwrench added 2 commits May 16, 2018 13:30

$@fractalwrench$

refactor: remove redundant autoCaptureSessions assignment

397ac7f

$@fractalwrench$

test: don't override autocapturesessions if sessionsEndpoints is vali…

b03b1d6

…d, add explicit test coverage

Cawllec reviewed May 16, 2018

View reviewed changes

martin308 reviewed May 16, 2018

View reviewed changes

$@fractalwrench$

docs: Add rationale for suppressing checkstyle warnings

ceb8cfe

$@fractalwrench$

Merge branch 'next' into enable-session-tracking

abbcc28

$@fractalwrench$ fractalwrench requested a review from a team May 17, 2018 13:46

Cawllec approved these changes May 17, 2018

View reviewed changes

martin308 reviewed May 17, 2018

View reviewed changes

$@fractalwrench$

remove unneeded suppresswarnings

58baad8

bengourley reviewed May 31, 2018

View reviewed changes

fractalwrench added 2 commits June 6, 2018 10:30

$@fractalwrench$

docs: fix some session tracking docs typos

6432b4c

$@fractalwrench$

Merge branch 'next' into enable-session-tracking

0706d25

$@fractalwrench$ fractalwrench merged commit 9ec00e2 into next Jun 12, 2018

$@fractalwrench$ fractalwrench deleted the enable-session-tracking branch June 12, 2018 09:26

lemnik pushed a commit that referenced this pull request Jun 2, 2021

$@fractalwrench$

Merge pull request #314 from bugsnag/PLAT-4968/additional-mazerunner-…

e491baf

…scenarios Create additional E2E scenarios for Unity SO file upload

rich-bugsnag pushed a commit that referenced this pull request Sep 3, 2021

Merge pull request #314 from bugsnag/tms/staged-builds

1a779fb

Two stage builds

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable automatic session tracking by default #314

Enable automatic session tracking by default #314

$@fractalwrench$ fractalwrench commented May 10, 2018 •

edited

Loading

coveralls commented May 10, 2018

coveralls commented May 10, 2018 •

edited

Loading

kattrali left a comment

kattrali May 11, 2018

fractalwrench commented May 11, 2018

Cawllec May 16, 2018

$@fractalwrench$ fractalwrench May 16, 2018

Cawllec May 16, 2018

$@fractalwrench$ fractalwrench May 16, 2018

Cawllec May 16, 2018

$@fractalwrench$ fractalwrench May 16, 2018

Cawllec May 16, 2018

$@fractalwrench$ fractalwrench May 16, 2018

Cawllec May 16, 2018

$@fractalwrench$ fractalwrench May 16, 2018

Cawllec May 16, 2018

$@fractalwrench$ fractalwrench May 16, 2018

Cawllec May 16, 2018

$@fractalwrench$ fractalwrench May 17, 2018

martin308 left a comment

martin308 May 16, 2018

$@fractalwrench$ fractalwrench May 17, 2018

martin308 May 16, 2018

$@fractalwrench$ fractalwrench May 17, 2018

Cawllec commented May 17, 2018

fractalwrench commented May 17, 2018

Cawllec left a comment

martin308 May 17, 2018

$@fractalwrench$ fractalwrench May 18, 2018

bengourley May 31, 2018 •

edited

Loading

Enable automatic session tracking by default #314

Enable automatic session tracking by default #314

Conversation

fractalwrench commented May 10, 2018 • edited Loading

Goal

Design

Changeset

Added

Changed

Tests

Discussion

Outstanding Questions

Review

coveralls commented May 10, 2018

coveralls commented May 10, 2018 • edited Loading

kattrali left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fractalwrench commented May 11, 2018

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

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

Choose a reason for hiding this comment

Cawllec commented May 17, 2018

fractalwrench commented May 17, 2018

Cawllec left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bengourley May 31, 2018 • edited Loading

Choose a reason for hiding this comment

$@fractalwrench$ fractalwrench commented May 10, 2018 •

edited

Loading

coveralls commented May 10, 2018 •

edited

Loading

bengourley May 31, 2018 •

edited

Loading