-
Notifications
You must be signed in to change notification settings - Fork 441
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
Implement gitpod auth and settings sync #337
Conversation
6bd9416
to
ccd91eb
Compare
ccd91eb
to
2e474e7
Compare
I think we can do analytics with another PR? |
Wow, code looks so well-structured ❤️ and you seem to address all important concerns. |
reload it not enough, you need to close all windows |
telemetry is done just needed a few 💄, I'll upload another vsix |
Added vsix file updated with telemetry (telemetry key added manually before creating the vsix for now, asked @jakobhero if it's Ok to push it to github or not, MS does commit it in it's extensions) Telemetry event can be viewed here |
b24c664
to
c166153
Compare
yes, that's why I wrote |
e6f8fac
to
45cc112
Compare
c166153
to
c991e1b
Compare
@akosyakov added telemetry event description at the PR description |
It seems that code validating token was dropped, why? We should make sure that if a token gets expired on server side or we modify an extension to add more scopes then token should be invalidated. |
@jeanp413 @filiptronicek Could you rephrase UI facing messages to avoid |
@jeanp413 It looks like we will produce a lot of tables with such analytics events. Can we rather have only 3 events with different properties and timestamp to indicate progress:
These events thought won't be enough still to understand whether a user is really synching with us. Is there a way to track it from VS Code extension or we need to track in server then? machine ids are propagated with sync requests as headers? |
I think there's no API to know from within the extension when it was installed the first time or uninstalled
We'll need to do it in the server, no API for that |
ac5fa95
to
cbbc8a9
Compare
fb80f0c
to
e9e47b0
Compare
Added |
e9e47b0
to
da06a18
Compare
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.
da06a18
to
b5fa034
Compare
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.
it looks good to me, besides #337 (comment)
@filiptronicek Could you double check following:
- switching between Gitpod host does not require auth again because we delete token unnecessary
- opening with VS Code Desktop still works
@filiptronicek can you test and if everything looks good to you too approve it so we can merge this and release 🚀
P.S.: updated vsix file in the PR description |
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.
It worked nicely for me. I left some comments about user facing naming casing. It does not match to how other commands are named.
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.
opening with VS Code Desktop still works
Yes it does! All looking good.
For some reason I also wanted to try installing extensions not on OpenVSX. It looks like it is behaving correctly by not removing it, just skipping it from sync in places where it is not available...
[2022-04-28 18:47:33.081] [settingssync] [info] Extensions: Skipped synchronizing extension because the extension is not found. github.copilot
adbfebf
to
607a03a
Compare
related gitpod-io/gitpod#9136
Telemetry Events:
Common properties:
login
orlogout
orlogin_cancelled
orlogin_failed
orlogout_failed
'["function:accessCodeSyncStorage","function:getGitpodTokenScopes","function:getLoggedInUser","resource:default"]'
'true'
or'false'
install