-
Notifications
You must be signed in to change notification settings - Fork 130
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
On by default session tracking #286
Conversation
…hich should pass existing verif
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 The second issue is that the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added some comments on some things that might be good to look into
Source/BugsnagConfiguration.h
Outdated
* 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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is it worth using something like this to make this as deprecated?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
andsessionsURL
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might be worth trying:
- Mark both properties as
readonly
- Add setter methods
- Mark setters as deprecated
Source/BugsnagConfiguration.h
Outdated
/** | ||
* Retrieves the endpoint used to send tracked sessions to Bugsnag | ||
* | ||
* NOTE: it is strongly recommended that you set this value via setEndpointsForNotify:sessions: instead. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is it worth using something like this to make this as deprecated?
@@ -14,7 +14,7 @@ @implementation BugsnagUser | |||
- (instancetype)initWithDictionary:(NSDictionary *)dict { | |||
if (self = [super init]) { | |||
_userId = dict[@"id"]; | |||
_emailAddress = dict[@"emailAddress"]; | |||
_emailAddress = dict[@"email"]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
are the changes in this file related to this PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that makes sense, I think it would be good to have it in a separate PR to track it more easily
Tests/BugsnagConfigurationTests.m
Outdated
BugsnagConfiguration *config = [BugsnagConfiguration new]; | ||
[config setEndpointsForNotify:@"" sessions:@"http://sessions.example.com"]; | ||
XCTFail(); | ||
} @catch(NSException * e) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is it better to use something like XCTAssertThrowsSpecific
here?
Tests/BugsnagConfigurationTests.m
Outdated
BugsnagConfiguration *config = [BugsnagConfiguration new]; | ||
[config setEndpointsForNotify:@"http://" sessions:@"http://sessions.example.com"]; | ||
XCTFail(); | ||
} @catch(NSException * e) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is it better to use something like XCTAssertThrowsSpecific
here?
@@ -19,7 +19,7 @@ - (void)testDictDeserialisation { | |||
|
|||
NSDictionary *dict = @{ | |||
@"id": @"test", | |||
@"emailAddress": @"[email protected]", | |||
@"email": @"[email protected]", |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
@@ -249,6 +251,22 @@ - (NSDictionary *)sessionApiHeaders { | |||
}; | |||
} | |||
|
|||
- (void)setEndpointsForNotify:(NSString *_Nonnull)notify sessions:(NSString *_Nonnull)sessions { |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
- (void)testSetEmptyNotifyEndpoint { | ||
BugsnagConfiguration *config = [BugsnagConfiguration new]; | ||
XCTAssertThrowsSpecificNamed([config setEndpointsForNotify:@"" sessions:@"http://sessions.example.com"], | ||
NSException, NSInternalInconsistencyException); |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
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
I believe the outstanding issues with this PR are the question of what to do with the I found a few issues with the existing Cocoa session tracking implementation, which I've addressed in this PR and need review:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good overall, had some concerns around concurrency and the test setup.
} | ||
|
||
- (void)run { | ||
[self flushAllSessions]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you shorten the interval in tests? It sounds similar to something that I did in the tests bugsnag-node.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also did some casual time travelling https://github.com/bugsnag/bugsnag-node/blob/7f4ee1c1219216b9f19ab7ce5ea75c33e555388d/test/sessions/tracker.js#L12
@implementation AutoSessionScenario | ||
|
||
- (void)startBugsnag { | ||
self.config.shouldAutoCaptureSessions = YES; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this be unneeded since YES
is the default?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@implementation DisabledSessionTrackingScenario | ||
|
||
- (void)run { | ||
[self flushAllSessions]; |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Source/BugsnagSessionTracker.m
Outdated
} | ||
dispatch_semaphore_wait(requestSemaphore, DISPATCH_TIME_FOREVER); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed by sending a signal to the semaphore immediately in this case.
…ng rather than manual hooks
Addressed the inline feedback and added a changelog entry. I've decided to make |
Adding |
@kattrali nope - not in the example Swift project. I attempted adding:
Then ran |
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 |
@kattrali the exact error message is: For the following declaration: It seems to compile ok for Objective-C, although there is a lint warning. |
I see. And if you don’t change the |
@kattrali that seems to work, although there's another warning:
I can ignore that and proceed with the above approach if you're happy? |
Haha my happiness is compiler happiness + making usage intent clear :) Does defining the getter + making the setter deprecated resolve everything? |
@kattrali for Objective-C yes, that resolves the issues, but the deprecation notice isn't shown in Swift. |
After discussing this with @snmaynard we decided that of the options below, (2) would be the best:
I'm not aware of anything else outstanding on this PR. |
Tests/BugsnagSinkTests.m
Outdated
@@ -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"]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The notifyURL was cleared previous, but now this is being set to localhost:8000, is this correct? Also sessions endpoint is localhost:10000
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
sessionURL
andnotifyURL
to indicate they should be only used to read the value, not write.Discussion
Outstanding Questions
What should our approach be to deprecating the
notifyURL
andsessionURL
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:
For the pull request reviewer(s), this changeset has been reviewed for: