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

Add stopSession() and resumeSession() to Bugsnag #325

Merged
merged 11 commits into from
Feb 27, 2019
Merged

Conversation

fractalwrench
Copy link
Contributor

Goal

Adds the ability to stop and resume sessions. This prevents the handled/unhandled error count from incrementing when a session is in the stopped state, which may be desirable if a user manually tracks sessions and does not care about crashes that occur in the background.

Changeset

Added the public interface for stopping/resuming a session, along with the initial implementation and unit test coverage.

NOTE: mazerunner scenarios and documentation will be added in separate PRs.

Adds the ability to stop and resume sessions. This prevents the handled/unhandled error count from
affecting the stability score when a session is in the stopped state as they will not be serialised
into error payloads. This can be useful when manually tracking sessions to avoid crashes in
background services from affecting the score.
Copy link
Contributor

@kattrali kattrali left a comment

Choose a reason for hiding this comment

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

Added a few things inline. As a more general comment which isn't limited to a specific line, there is a lot of switching between using the property and ivar accessors for SessionTracker.currentSession, which I think is intended to use or bypass the special behavior in -[SessionTracker currentSession] override implementation. This makes the implementation somewhat unclear and could be extracted into a function (e.g. [self runningSession]) and/or more heavily commented to explain what is being used and why.

Source/BugsnagSessionTracker.m Outdated Show resolved Hide resolved
Source/BugsnagSession.h Outdated Show resolved Hide resolved
Source/BugsnagSessionTracker.m Outdated Show resolved Hide resolved
@fractalwrench
Copy link
Contributor Author

Thanks for the feedback @kattrali - I've addressed the inline comments and have updated the BugsnagSessionTracker implementation to use a self.runningSession method.

Source/BugsnagSession.m Outdated Show resolved Hide resolved
Source/BugsnagSessionTracker.m Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants