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

Enable automatic session tracking by default #314

Merged
merged 21 commits into from
Jun 12, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
461eebb
feat: enable session tracking by default
fractalwrench May 9, 2018
307270c
test: add tests for setEndpoints behaviour
fractalwrench May 9, 2018
59a8164
feat: implement setEndpoints config option
fractalwrench May 9, 2018
1b14ae9
feat: enable on-by-default for manifest configuration
fractalwrench May 9, 2018
711ab74
test: fix auto context test to disable automatic session tracking (co…
fractalwrench May 9, 2018
6886111
docs: document deprecated config apis
fractalwrench May 9, 2018
84efa1e
docs: add javadoc for new setEndpoints API
fractalwrench May 9, 2018
e276037
test: Manually calling sendSession() sends a session regardless of th…
fractalwrench May 10, 2018
8e38483
test: A single app load sends a single session by default
fractalwrench May 10, 2018
f89c322
docs: add changelog for on-by-default session tracking
fractalwrench May 10, 2018
145c45a
Merge branch 'master' into enable-session-tracking
fractalwrench May 10, 2018
4dcf7ad
style: pass checkstyle for android on-by-default changes
fractalwrench May 10, 2018
3334063
Merge branch 'enable-session-tracking' of https://github.com/bugsnag/…
fractalwrench May 10, 2018
66ce062
refactor: check if sessionsEndpoint is null rather than extra boolean…
fractalwrench May 11, 2018
397ac7f
refactor: remove redundant autoCaptureSessions assignment
fractalwrench May 16, 2018
b03b1d6
test: don't override autocapturesessions if sessionsEndpoints is vali…
fractalwrench May 16, 2018
ceb8cfe
docs: Add rationale for suppressing checkstyle warnings
fractalwrench May 17, 2018
abbcc28
Merge branch 'next' into enable-session-tracking
fractalwrench May 17, 2018
58baad8
remove unneeded suppresswarnings
fractalwrench May 18, 2018
6432b4c
docs: fix some session tracking docs typos
fractalwrench Jun 6, 2018
0706d25
Merge branch 'next' into enable-session-tracking
fractalwrench Jun 12, 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
9 changes: 7 additions & 2 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,6 +1,12 @@
# Changelog

## 4.4.1 (TBD)
## 4.X.X (TBD)

**Note**: this release alters the behaviour of the notifier to track sessions automatically. If you
use Bugsnag On-Premise, it is now also recommended that you set your notify and session endpoints
via `config.setEndpoints(String notify, String sessions)`.

* Enable automatic session tracking by default [#314](https://github.com/bugsnag/bugsnag-android/pull/314)

### Bug fixes

Expand Down Expand Up @@ -35,7 +41,6 @@ and [class documentation for `Delivery`](https://bugsnag.github.io/bugsnag-andro
* Use buffered streams for IO (perf improvement)
[#307](https://github.com/bugsnag/bugsnag-android/pull/307)


## 4.3.4 (2018-05-02)

### Bug fixes
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -42,8 +42,7 @@ class MainActivity : Activity() {
private fun prepareConfig(): Configuration {
val config = Configuration(intent.getStringExtra("BUGSNAG_API_KEY"))
val port = intent.getStringExtra("BUGSNAG_PORT")
config.endpoint = "${findHostname()}:$port"
config.sessionEndpoint = "${findHostname()}:$port"
config.setEndpoints("${findHostname()}:$port", "${findHostname()}:$port")
return config
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ internal class AutoContextScenario(config: Configuration,
context: Context) : Scenario(config, context) {

override fun run() {
config.setAutoCaptureSessions(false)
super.run()
context.startActivity(Intent(context, SecondActivity::class.java))

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@ internal class AutoSessionScenario(config: Configuration,
context: Context) : Scenario(config, context) {

override fun run() {
config.setAutoCaptureSessions(true)
super.run()
Bugsnag.setUser("123", "user@example.com", "Joe Bloggs")
context.startActivity(Intent(context, SecondActivity::class.java))
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,50 @@ public void setUp() throws Exception {

@Test
public void testEndpoints() {
String notify = "https://notify.myexample.com";
String sessions = "https://sessions.myexample.com";
config.setEndpoints(notify, sessions);

assertEquals(notify, config.getEndpoint());
assertEquals(sessions, config.getSessionEndpoint());
}

@Test(expected = IllegalArgumentException.class)
public void testNullNotifyEndpoint() {
//noinspection ConstantConditions
config.setEndpoints(null, "http://example.com");
}

@Test(expected = IllegalArgumentException.class)
public void testEmptyNotifyEndpoint() {
config.setEndpoints("", "http://example.com");
}

@Test
public void testInvalidSessionEndpoint() {
//noinspection ConstantConditions
config.setEndpoints("http://example.com", null);
assertFalse(config.shouldAutoCaptureSessions());
assertNull(config.getSessionEndpoint());

config.setEndpoints("http://example.com", "");
assertFalse(config.shouldAutoCaptureSessions());
assertNull(config.getSessionEndpoint());

config.setEndpoints("http://example.com", "http://sessions.example.com");
assertFalse(config.shouldAutoCaptureSessions());
assertEquals("http://sessions.example.com", config.getSessionEndpoint());
}

@Test
public void testAutoCaptureOverride() {
config.setAutoCaptureSessions(false);
config.setEndpoints("http://example.com", "http://example.com");
assertFalse(config.shouldAutoCaptureSessions());
}

@Test
public void testEndpoint() {
// Default endpoints
assertEquals("https://notify.bugsnag.com", config.getEndpoint());

Expand All @@ -39,7 +83,7 @@ public void testEndpoints() {
}

@Test
public void testSessionEndpoints() {
public void testSessionEndpoint() {
// Default endpoints
assertEquals("https://sessions.bugsnag.com", config.getSessionEndpoint());

Expand Down Expand Up @@ -110,7 +154,9 @@ public void testLaunchThreshold() throws Exception {
}

@Test
public void testDefaults() throws Exception {
public void testAutoCaptureSessions() throws Exception {
assertTrue(config.shouldAutoCaptureSessions());
config.setAutoCaptureSessions(false);
assertFalse(config.shouldAutoCaptureSessions());
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -182,4 +182,26 @@ public void testSessionTimeout() throws Exception {
assertNotEquals(firstSession, sessionTracker.getCurrentSession());
}

@Test
public void startSessionNoEndpoint() throws Exception {
assertNull(sessionTracker.getCurrentSession());
configuration.setEndpoints("http://localhost:1234", "");
sessionTracker.startNewSession(new Date(), user, false);
assertNull(sessionTracker.getCurrentSession());
}

@Test
public void startSessionAutoCaptureEnabled() {
assertNull(sessionTracker.getCurrentSession());
sessionTracker.startNewSession(new Date(), user, false);
assertNotNull(sessionTracker.getCurrentSession());
}

@Test
public void startSessionAutoCaptureDisabled() {
configuration.setAutoCaptureSessions(false);
assertNull(sessionTracker.getCurrentSession());
sessionTracker.startNewSession(new Date(), user, false);
assertNotNull(sessionTracker.getCurrentSession());
}
}
3 changes: 2 additions & 1 deletion sdk/src/main/java/com/bugsnag/android/Bugsnag.java
Original file line number Diff line number Diff line change
Expand Up @@ -117,7 +117,8 @@ public static void setContext(final String context) {
* endpoint.
*
* @param endpoint the custom endpoint to send report to
* @deprecated use {@link com.bugsnag.android.Configuration#setEndpoint(String)} instead.
* @deprecated use {@link com.bugsnag.android.Configuration#setEndpoints(String, String)}
* instead.
*/
@Deprecated
public static void setEndpoint(final String endpoint) {
Expand Down
23 changes: 13 additions & 10 deletions sdk/src/main/java/com/bugsnag/android/Client.java
Original file line number Diff line number Diff line change
Expand Up @@ -310,21 +310,21 @@ static Configuration populateConfigFromManifest(@NonNull Configuration config,
config.setAppVersion(data.getString(MF_APP_VERSION));
config.setReleaseStage(data.getString(MF_RELEASE_STAGE));

String endpoint = data.getString(MF_ENDPOINT);

if (endpoint != null) {
config.setEndpoint(endpoint);
}
String sessionEndpoint = data.getString(MF_SESSIONS_ENDPOINT);

if (sessionEndpoint != null) {
config.setSessionEndpoint(sessionEndpoint);
if (data.containsKey(MF_ENDPOINT)) {
String endpoint = data.getString(MF_ENDPOINT);
String sessionEndpoint = data.getString(MF_SESSIONS_ENDPOINT);
//noinspection ConstantConditions (pass in null/empty as this function will warn)
config.setEndpoints(endpoint, sessionEndpoint);
}

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)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

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

config.setAutoCaptureSessions(data.getBoolean(MF_AUTO_CAPTURE_SESSIONS));
}

config.setEnableExceptionHandler(
data.getBoolean(MF_ENABLE_EXCEPTION_HANDLER, true));
return config;
Expand Down Expand Up @@ -388,7 +388,10 @@ public void setContext(String context) {
* endpoint.
*
* @param endpoint the custom endpoint to send report to
* @deprecated use {@link com.bugsnag.android.Configuration#setEndpoints(String, String)}
* instead.
*/
@Deprecated
public void setEndpoint(String endpoint) {
config.setEndpoint(endpoint);
}
Expand Down
48 changes: 44 additions & 4 deletions sdk/src/main/java/com/bugsnag/android/Configuration.java
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
import android.content.Context;
import android.support.annotation.NonNull;
import android.support.annotation.Nullable;
import android.text.TextUtils;

import java.util.Arrays;
import java.util.Collection;
Expand All @@ -29,8 +30,8 @@ public class Configuration extends Observable implements Observer {
private String buildUuid;
private String appVersion;
private String context;
private String endpoint = "https://notify.bugsnag.com";
private String sessionEndpoint = "https://sessions.bugsnag.com";
private volatile String endpoint = "https://notify.bugsnag.com";
private volatile String sessionEndpoint = "https://sessions.bugsnag.com";

private String[] ignoreClasses;
@Nullable
Expand All @@ -41,7 +42,7 @@ public class Configuration extends Observable implements Observer {
private boolean enableExceptionHandler = true;
private boolean persistUserBetweenSessions = false;
private long launchCrashThresholdMs = 5 * 1000;
private boolean autoCaptureSessions = false;
private boolean autoCaptureSessions = true;
private boolean automaticallyCollectBreadcrumbs = true;

@NonNull
Expand Down Expand Up @@ -135,11 +136,48 @@ public String getEndpoint() {
* endpoint.
*
* @param endpoint the custom endpoint to send report to
* @deprecated use {@link com.bugsnag.android.Configuration#setEndpoints(String, String)}
*/
@Deprecated
public void setEndpoint(String endpoint) {
this.endpoint = endpoint;
}

/**
* 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 exception 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 IllegalArgumentException if the notify endpoint is empty or null
*/
public void setEndpoints(@NonNull String notify, @NonNull String sessions)
throws IllegalArgumentException {

if (TextUtils.isEmpty(notify)) {
throw new IllegalArgumentException("Notify endpoint cannot be empty or null.");
} else {
this.endpoint = notify;
}

boolean invalidSessionsEndpoint = TextUtils.isEmpty(sessions);

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

/**
* Gets the Session Tracking API endpoint
*
Expand All @@ -151,12 +189,14 @@ public String getSessionEndpoint() {

/**
* Set the endpoint to send Session Tracking data to. By default we'll send reports to
* the standard https://session.bugsnag.com endpoint, but you can override
* the standard https://sessions.bugsnag.com endpoint, but you can override
* this if you are using Bugsnag Enterprise to point to your own Bugsnag
* endpoint.
*
* @param endpoint the custom endpoint to send session data to
* @deprecated use {@link com.bugsnag.android.Configuration#setEndpoints(String, String)}
*/
@Deprecated
public void setSessionEndpoint(String endpoint) {
this.sessionEndpoint = endpoint;
}
Expand Down
5 changes: 5 additions & 0 deletions sdk/src/main/java/com/bugsnag/android/SessionTracker.java
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,11 @@ class SessionTracker implements Application.ActivityLifecycleCallbacks {
* @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. "
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

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 one is somewhat similar.

Copy link
Contributor

Choose a reason for hiding this comment

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

I mean for the logging repeatedly when in debug mode?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

+ "Session tracking is disabled");
return;
}
Session session = new Session(UUID.randomUUID().toString(), date, user, autoCaptured);
currentSession.set(session);
trackSessionIfNeeded(session);
Expand Down