-
Notifications
You must be signed in to change notification settings - Fork 64
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
chore(telemetry): telemetry improvements VSCODE-673 #914
Changes from 5 commits
e53ae4d
1b54bf1
d288e97
1e6a96d
006176b
df29dac
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,23 +1,26 @@ | ||
import * as vscode from 'vscode'; | ||
import type * as vscode from 'vscode'; | ||
import HelpTree from './helpTree'; | ||
import type { TelemetryService } from '../telemetry'; | ||
import { createTrackedTreeView } from '../utils/treeViewHelper'; | ||
|
||
export default class HelpExplorer { | ||
_treeController: HelpTree; | ||
_treeView?: vscode.TreeView<vscode.TreeItem>; | ||
|
||
constructor() { | ||
constructor(private _telemetryService: TelemetryService) { | ||
this._treeController = new HelpTree(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we need There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this is meant to be a shortcut for passing it as an argument in the constructor and defining/setting it as a property of the class object which is then used in its method There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We need to inject the telemetry service and I think it's better to do it via the ctor rather than as an argument to |
||
} | ||
|
||
activateHelpTreeView(telemetryService: TelemetryService): void { | ||
activateHelpTreeView(): void { | ||
if (!this._treeView) { | ||
this._treeView = vscode.window.createTreeView('mongoDBHelpExplorer', { | ||
treeDataProvider: this._treeController, | ||
}); | ||
this._treeView = createTrackedTreeView( | ||
'mongoDBHelpExplorer', | ||
this._treeController, | ||
this._telemetryService | ||
); | ||
this._treeController.activateTreeViewEventHandlers( | ||
this._treeView, | ||
telemetryService | ||
this._telemetryService | ||
); | ||
} | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,21 +1,22 @@ | ||
import * as vscode from 'vscode'; | ||
import type * as vscode from 'vscode'; | ||
import PlaygroundsTree from './playgroundsTree'; | ||
import type { TelemetryService } from '../telemetry'; | ||
import { createTrackedTreeView } from '../utils/treeViewHelper'; | ||
|
||
export default class PlaygroundsExplorer { | ||
private _treeController: PlaygroundsTree; | ||
private _treeView?: vscode.TreeView<vscode.TreeItem>; | ||
|
||
constructor() { | ||
constructor(private _telemetryService: TelemetryService) { | ||
this._treeController = new PlaygroundsTree(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same here. Doesn't seem we use |
||
} | ||
|
||
private createPlaygroundsTreeView = (): void => { | ||
if (!this._treeView) { | ||
this._treeView = vscode.window.createTreeView( | ||
this._treeView = createTrackedTreeView( | ||
'mongoDBPlaygroundsExplorer', | ||
{ | ||
treeDataProvider: this._treeController, | ||
} | ||
this._treeController, | ||
this._telemetryService | ||
); | ||
this._treeController.activateTreeViewEventHandlers(this._treeView); | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -76,7 +76,14 @@ abstract class TelemetryEventBase { | |
export class PlaygroundExecutedTelemetryEvent implements TelemetryEventBase { | ||
type = 'Playground Code Executed'; | ||
properties: { | ||
/** The type of the executed operation, e.g. 'insert', 'update', 'delete', 'query', 'aggregation', 'other' */ | ||
/** | ||
* The type of the executed operation. Common CRUD operations are mapped to | ||
* 'insert', 'update', 'delete', 'query', 'aggregation'. Other operations return | ||
* the type of the result returned by the shell API - e.g. 'collection', 'database', | ||
* 'help', etc. for known shell types and 'string', 'number', 'undefined', etc. for | ||
* plain JS types. In the unlikely case the shell evaluator was unable to determine | ||
* a type, 'other' is returned. | ||
*/ | ||
type: string | null; | ||
|
||
/** Whether the entire script was run or just a part of it */ | ||
|
@@ -118,7 +125,7 @@ export class PlaygroundExecutedTelemetryEvent implements TelemetryEventBase { | |
return 'query'; | ||
} | ||
|
||
return 'other'; | ||
return shellApiType; | ||
} | ||
} | ||
|
||
|
@@ -627,6 +634,41 @@ export class PresetConnectionEditedTelemetryEvent | |
} | ||
} | ||
|
||
/** Reported when the extension side panel is opened. VSCode doesn't expose | ||
* a subscribable event for this, so we're inferring it by subscribing to | ||
* treeView.onDidChangeVisibility for all the extension treeviews and throttling | ||
* the events. | ||
*/ | ||
export class PanelOpenedTelemetryEvent implements TelemetryEventBase { | ||
nirinchev marked this conversation as resolved.
Show resolved
Hide resolved
|
||
type = 'Side Panel Opened'; | ||
properties: {}; | ||
|
||
constructor() { | ||
this.properties = {}; | ||
} | ||
} | ||
|
||
/** | ||
* Reported when a tree item from the collection explorer is expanded. | ||
*/ | ||
export class TreeItemExpandedTelemetryEvent implements TelemetryEventBase { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. though here this seems better as section is too vague... which maybe says more about the name of the type. I don't mind leaving it as is There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think I'd prefer to leave this as - I know there's a discrepancy between the class name and the event name, but I feel it's fine as those are intended to be consumed by different audiences. |
||
type = 'Section Expanded'; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That was the name Amanda suggested and I am inclined to keep it because the fact those are tree items is an implementation detail of the code and not terminology that non-engineers necessarily use when describing the connection tree. |
||
properties: { | ||
/** | ||
* The name of the section - e.g. database, collection, etc. This is obtained from the | ||
* `contextValue` field of the tree item. | ||
* */ | ||
section_name?: string; | ||
}; | ||
|
||
constructor(item: vscode.TreeItem) { | ||
// We suffix all tree item context values with 'TreeItem', which is redundant when sending to analytics. | ||
this.properties = { | ||
section_name: item.contextValue?.replace('TreeItem', ''), | ||
}; | ||
} | ||
} | ||
|
||
export type TelemetryEvent = | ||
| PlaygroundExecutedTelemetryEvent | ||
| LinkClickedTelemetryEvent | ||
|
@@ -649,4 +691,6 @@ export type TelemetryEvent = | |
| ParticipantChatOpenedFromActionTelemetryEvent | ||
| ParticipantInputBoxSubmittedTelemetryEvent | ||
| ParticipantResponseGeneratedTelemetryEvent | ||
| PresetConnectionEditedTelemetryEvent; | ||
| PresetConnectionEditedTelemetryEvent | ||
| PanelOpenedTelemetryEvent | ||
| TreeItemExpandedTelemetryEvent; |
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.
do I understand correctly that if we do not have an
onDidExpand
set then we also don't end up calling_onTreeItemUpdate
? Seems to be the legacy behavior so not an issue for this PR, more of not sure why this is fully returning here.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 seems to be the case - I don't know if this is incorrect and a lurking bug or there's a deeper meaning to 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.
I think this part handles three elements that don't have children. I might be mistaken, maybe Rhys knows better.