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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ensure whoami failures (i.e. present but broken tokens) at startup don't break commands #675

Merged
merged 2 commits into from Oct 6, 2017

Conversation

Projects
None yet
2 participants
@pimterry
Member

pimterry commented Oct 5, 2017

Right now, if your token is invalid - i.e. expired, or for the wrong service (staging vs prod) or somehow corrupted - then every command fails, including help and login, because they try to get your username up front for analytics and that explodes. That's pretty bad. This stops everything exploding.

There's some other parts to complete this full, but once this is released in combination with resin-io/resin-sdk#414 and resin-io-modules/resin-request#105, this will solve #443 馃嵕

@pimterry pimterry requested review from thgreasi and MoranF Oct 5, 2017

@thgreasi

How about also adding a cathReturn after events.trackCommand(cli) in app.coffee? Makes sense that Tracking errors should never prevent you to do your actual task.

@pimterry

This comment has been minimized.

Show comment
Hide comment
@pimterry

pimterry Oct 5, 2017

Member

How about also adding a cathReturn after events.trackCommand(cli) in app.coffee? Makes sense that Tracking errors should never prevent you to do your actual task.

Another excellent idea 馃憤.

Done, in the definition of trackCommand itself rather than the calling code, since nobody should ever be waiting on that.

I've also added a timeout, so if it works but it's just a bit slow, we don't wait around. Thought about not waiting for the promise at all, but it'd be nice to increase the odds we'll get events for commands that run pretty quickly (resin help).

Member

pimterry commented Oct 5, 2017

How about also adding a cathReturn after events.trackCommand(cli) in app.coffee? Makes sense that Tracking errors should never prevent you to do your actual task.

Another excellent idea 馃憤.

Done, in the definition of trackCommand itself rather than the calling code, since nobody should ever be waiting on that.

I've also added a timeout, so if it works but it's just a bit slow, we don't wait around. Thought about not waiting for the promise at all, but it'd be nice to increase the odds we'll get events for commands that run pretty quickly (resin help).

@thgreasi

This comment has been minimized.

Show comment
Hide comment
@thgreasi

thgreasi Oct 5, 2017

Member

Would it make sense to do something like this?

Promise.all([
	events.trackCommand(cli)
	cli.global?.help ?
		capitanoExecuteAsync(command: "help #{cli.command ? ''}") :
		capitanoExecuteAsync(cli)
])

So that the actual command can start the processing right away?

Member

thgreasi commented Oct 5, 2017

Would it make sense to do something like this?

Promise.all([
	events.trackCommand(cli)
	cli.global?.help ?
		capitanoExecuteAsync(command: "help #{cli.command ? ''}") :
		capitanoExecuteAsync(cli)
])

So that the actual command can start the processing right away?

@pimterry

This comment has been minimized.

Show comment
Hide comment
@pimterry

pimterry Oct 5, 2017

Member

Nice, definitely, done.

Member

pimterry commented Oct 5, 2017

Nice, definitely, done.

@@ -30,3 +30,5 @@ exports.trackCommand = (capitanoCommand) ->
resinUrl: resinUrl
platform: process.platform
command: capitanoCommand
.timeout(100)

This comment has been minimized.

@thgreasi

thgreasi Oct 6, 2017

Member

Lets say that there is a command runs in 1ms and the tracking code for some reason needs 150ms (eg slow DNS or overcrowded wifi).
Is there a chance that the cli closes and everything gets disposed before the mixpanel tracking completes and we loose that stats?
Would it make any difference to increase the timeout to something a bit larger?
(I'm not sure if that makes sense, but I was thinking of it last night, for cli commands that can complete without any network requests.)

@thgreasi

thgreasi Oct 6, 2017

Member

Lets say that there is a command runs in 1ms and the tracking code for some reason needs 150ms (eg slow DNS or overcrowded wifi).
Is there a chance that the cli closes and everything gets disposed before the mixpanel tracking completes and we loose that stats?
Would it make any difference to increase the timeout to something a bit larger?
(I'm not sure if that makes sense, but I was thinking of it last night, for cli commands that can complete without any network requests.)

This comment has been minimized.

@pimterry

pimterry Oct 6, 2017

Member

Would it make any difference to increase the timeout to something a bit larger?

Maybe. I've gone for a 100ms to avoid this issue for cases even when it's super fast, but in slow conditions that can still happen. It's not too bad though: I think as long as the full request has been sent, it doesn't matter if the response hasn't come back, so events should be tracked even if we time out while waiting for the mixpanel server & response. If we time out in TCP connection setup on the other hand, yes, we're stuck.

This problem is always going to be present up to a point though - eventually we have to decide whether we want to make users wait, or lose events, and I think I'm fairly happy dropping events occasionally in those cases. We could boost the timeout... I was trying to pick a time that's going to generally be sufficient, but isn't going to ever be noticeable if users do have to wait for it.

For now I've bumped it to a 500ms max. That could still time out, but it feels like it should successfully fire the event first, and if we block this beyond that point then we're well into noticeable experience, and even if we're going to lose events, I don't want to make users wait.

@pimterry

pimterry Oct 6, 2017

Member

Would it make any difference to increase the timeout to something a bit larger?

Maybe. I've gone for a 100ms to avoid this issue for cases even when it's super fast, but in slow conditions that can still happen. It's not too bad though: I think as long as the full request has been sent, it doesn't matter if the response hasn't come back, so events should be tracked even if we time out while waiting for the mixpanel server & response. If we time out in TCP connection setup on the other hand, yes, we're stuck.

This problem is always going to be present up to a point though - eventually we have to decide whether we want to make users wait, or lose events, and I think I'm fairly happy dropping events occasionally in those cases. We could boost the timeout... I was trying to pick a time that's going to generally be sufficient, but isn't going to ever be noticeable if users do have to wait for it.

For now I've bumped it to a 500ms max. That could still time out, but it feels like it should successfully fire the event first, and if we block this beyond that point then we're well into noticeable experience, and even if we're going to lose events, I don't want to make users wait.

This comment has been minimized.

@thgreasi

thgreasi Oct 6, 2017

Member

馃憤

@thgreasi

thgreasi Oct 6, 2017

Member

馃憤

@resin-io-versionbot resin-io-versionbot bot merged commit c187c11 into master Oct 6, 2017

5 checks passed

AutoMerges PR merging is in progress
Reviewers 1/1 review approvals met
Versionist Found all required commit footer tags
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@resin-io-versionbot resin-io-versionbot bot deleted the handle-broken-token branch Oct 6, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment