-
Notifications
You must be signed in to change notification settings - Fork 423
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
[gitpod] enable telemetry for testing #257
Conversation
0b66d21
to
4fe2a9f
Compare
2f6c762
to
5efd331
Compare
@loujaybee with this PR you can see different telemetry events instrumented by MS, could you have a look please what can be interesting for us? We can add also additional instrumentation if needed. To investigate:
|
Updated steps as I did more changes:
|
4fe2a9f
to
c53ef1f
Compare
c53ef1f
to
5107052
Compare
b8b3838
to
ccf3e6a
Compare
} | ||
|
||
log(eventName: string, data?: any): void { | ||
data = mixin(data, this._defaultData); |
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 will race with preparing api?
@@ -290,11 +293,13 @@ export class RemoteExtensionHostAgentServer extends Disposable { | |||
|
|||
let appInsightsAppender: ITelemetryAppender = NullAppender; | |||
if (supportsTelemetry(this._productService, this._environmentService)) { | |||
if (this._productService.aiConfig && this._productService.aiConfig.asimovKey) { | |||
if (this._productService.aiConfig && this._productService.aiConfig.asimovKey !== 'foo') { |
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 this left over, or genuine code? 😆
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's intended, I could remove that block of code, but this way I think it would create less merge conflicts
Wow, this is awesome! Thanks so much @jeanp413 😍 It would be good to get this foundational PR merged, which implements the "framework" for capturing these events, and an easy way to map these events into the tracking event structure (the structure is decided on a case-by-case basis) we send to Segment, which is implemented in I'd argue that this PR should only put in place that "foundational" code, and then we can specify the exact events we need to track, and what format we need to store them (for our purposes) on the other GitHub issues. e.g. In other words, we add the specifics of which telemetry events to track (and how to map them to the segment structure) on each analytics ticket (gitpod-io/gitpod#6897, gitpod-io/gitpod#6895, gitpod-io/gitpod#6981) However, for reference, some of events that look useful right now:
A few events I couldn't trigger yet:
A couple of challenges:
Thought: When we do implement the events, it would be nice if we have a simple collection of maps, that map between these telemetry events, and the analytics ones. e.g. const maps = [
(/* The teletry event */) => {
return // The analytics event
},
(/* The teletry event */) => {
return // The analytics event
}
] So that it's super easy to add new events (read: I could do it without having to bother you guys 😁 ) |
data = mixin(data, this._defaultData); | ||
data = validateTelemetryData(data); | ||
|
||
if (eventName !== 'workbenchActionExecuted') { |
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.
@jeanp413 let's start but tracking first read and write interactions, so we don't produce many data and addressing gitpod-io/gitpod#6981
We can have following event:
{
eventName: 'vscode_file_access`,
properties: {
// first `editorOpened` or `fileGet` fires `read`
// first `filePut` fires `write`
kind: 'read' | 'write',
// from supervisor info endpoint
workspaceId: string,
// from supervisor info endpoint
workspaceInstanceId: string,
// from MS telemetry event
sessionId: string,
// from MS telemetry event
timestamp: Date
}
}
It is important that we care about only first to avoid spamming segment.
We need also kind of a function which does conversion from MS telemetry event to our event similar how @loujaybee is suggested in #257 (comment) It should help later adding more events for other team members.
It will be all for the scope of PR. After that we can work one by one with concrete issues.
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 added the logic to send just the first events but they are misleading as they are triggered when the getting started page or the simple-browser is shown
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 because editorOpened
or fileGet
?
Maybe we can filter down editorOpened
for files with file
scheme?
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.
Will take a look but not sure if that is gonna be reliable enough. either way I think this PR is done and we can polish this event in following PRs related to each gitpod telemetry issue
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 ended up creating separate event for editorOpened
and filtering by 'workbench.editors.files.fileEditorInput'
fileGet
gets fired automatically by the gitpod-web extension when reading the gitpod.yml
file.
If this needs more tweaking lets address them in following PRs
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.
yeah it sounds like fileGet
and filePut
more track fs access by any source (i.e. api and user triggered), we should not rely on it.
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.
so should I delete them? and just send editorOpened
?
@jeanp413 check that by enabling telemetry for Gitpod we don't send anything to MS even from extensions like built-in for instance git or 3rd party like GHPR extensions. |
5107052
to
fbd9385
Compare
8f56e45
to
9392504
Compare
0ba1637
to
caa452b
Compare
57e5f82
to
4877cf3
Compare
I'll go ahead and merge this |
211a663
to
3714d21
Compare
super(address, protocols, { | ||
headers: { | ||
'Origin': new URL(gitpodHost).origin, | ||
'Authorization': `Bearer ${serverToken}` |
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.
@jeanp413 we need to add here User-Agent
at least. It should be a requirement for all our clients to Gitpod Server
} | ||
filterEvent.add(eventName); | ||
return { | ||
event: `vscode_${formatEventName(eventName)}`, |
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 should be vscode_file_access
with read
kind.
I think it would be better to wait for approval with this PR.
We should be careful with adding new events. Because we need to track them in these doc and make sure that they make sense for @loujaybee and @jakobhero: https://www.notion.so/gitpod/dd900d5177944fc38d22bb8c7d3d2cce?v=54cdae8ff0f54b4d8235429ab9893e49
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, sorry about that, do you want me to revert it? or just push the changes directly?
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 should be enough to make a new PR which properly collects vscode_file_access
. And merge it only after approval this time.
This PR fixes #
Logs can be found in
~/.vscode-remote/data/logs/<date>/telemetry.log