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.

Sign up for GitHub

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

Jump to bottom

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

Merged
2 commits merged into from Oct 6, 2017
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
eb729d1
Ensure analytics failures (e.g. from broken tokens) at startup don't …
pimterry Oct 5, 2017
f7c0258
v6.6.8
Oct 6, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 4 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Expand Up @@ -4,6 +4,10 @@ All notable changes to this project will be documented in this file
automatically by Versionist. DO NOT EDIT THIS FILE MANUALLY!
This project adheres to [Semantic Versioning](http://semver.org/).

## v6.6.8 - 2017-10-06

* Ensure analytics failures (e.g. from broken tokens) at startup don't break commands #675 [Tim Perry]

## v6.6.7 - 2017-09-22

* Update to resin-sync, which fixes local push on windows #666 [Tim Perry]
Expand Down
10 changes: 6 additions & 4 deletions build/app.js
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 2 additions & 2 deletions build/events.js
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

9 changes: 6 additions & 3 deletions lib/app.coffee
View file
Open in desktop
Expand Up @@ -203,9 +203,12 @@ update.notify()
plugins.register(/^resin-plugin-(.+)$/).then ->
cli = capitano.parse(process.argv)

events.trackCommand(cli).then ->
runCommand = ->
if cli.global?.help
return capitanoExecuteAsync(command: "help #{cli.command ? ''}")
capitanoExecuteAsync(cli)
capitanoExecuteAsync(command: "help #{cli.command ? ''}")
else
capitanoExecuteAsync(cli)

Promise.all([events.trackCommand(cli), runCommand()])

.catch(errors.handle)
4 changes: 3 additions & 1 deletion lib/events.coffee
View file
Open in desktop
Expand Up @@ -13,7 +13,7 @@ exports.trackCommand = (capitanoCommand) ->

return Promise.props
resinUrl: resin.settings.get('resinUrl')
username: resin.auth.whoami()
username: resin.auth.whoami().catchReturn(undefined)
mixpanel: exports.getLoggerInstance()
.then ({ username, resinUrl, mixpanel }) ->
return capitanoStateGetMatchCommandAsync(capitanoCommand.command).then (command) ->
Expand All @@ -30,3 +30,5 @@ exports.trackCommand = (capitanoCommand) ->
resinUrl: resinUrl
platform: process.platform
command: capitanoCommand
.timeout(100)
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

👍

.catchReturn()
2 changes: 1 addition & 1 deletion package.json
View file
Open in desktop
@@ -1,6 +1,6 @@
{
"name": "resin-cli",
"version": "6.6.7",
"version": "6.6.8",
"description": "The official resin.io CLI tool",
"main": "./build/actions/index.js",
"homepage": "https://github.com/resin-io/resin-cli",
Expand Down