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.

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

Merged
merged 6 commits into from
Jan 29, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions package-lock.json

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

1 change: 1 addition & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -1361,6 +1361,7 @@
"@types/chai": "^4.3.20",
"@types/debug": "^4.1.12",
"@types/glob": "^7.2.0",
"@types/lodash": "^4.17.14",
"@types/micromatch": "^4.0.9",
"@types/mkdirp": "^2.0.0",
"@types/mocha": "^8.2.3",
Expand Down
4 changes: 2 additions & 2 deletions src/connectionController.ts
Original file line number Diff line number Diff line change
Expand Up @@ -331,7 +331,7 @@ export default class ConnectionController {
}
}

public sendTelemetry(
private sendTelemetry(
newDataService: DataService,
connectionType: ConnectionTypes
): void {
Expand Down Expand Up @@ -436,7 +436,7 @@ export default class ConnectionController {
});
const browserAuthCommand = vscode.workspace
.getConfiguration('mdb')
.get('browserCommandForOIDCAuth');
.get<string>('browserCommandForOIDCAuth');
dataService = await connectionAttempt.connect({
...connectionOptions,
oidc: {
Expand Down
31 changes: 18 additions & 13 deletions src/explorer/explorerController.ts
Original file line number Diff line number Diff line change
@@ -1,38 +1,43 @@
import * as vscode from 'vscode';
import type * as vscode from 'vscode';

import type ConnectionController from '../connectionController';
import { DataServiceEventTypes } from '../connectionController';
import ExplorerTreeController from './explorerTreeController';
import { createTrackedTreeView } from '../utils/treeViewHelper';
import type { TelemetryService } from '../telemetry';

export default class ExplorerController {
private _connectionController: ConnectionController;
private _treeController: ExplorerTreeController;
private _treeView?: vscode.TreeView<vscode.TreeItem>;

constructor(connectionController: ConnectionController) {
this._connectionController = connectionController;
this._treeController = new ExplorerTreeController(connectionController);
constructor(
private _connectionController: ConnectionController,
private _telemetryService: TelemetryService
) {
this._treeController = new ExplorerTreeController(
this._connectionController,
this._telemetryService
);
}

createTreeView = (): void => {
private createTreeView = (): void => {
// Remove the listener that called this function.
this._connectionController.removeEventListener(
DataServiceEventTypes.CONNECTIONS_DID_CHANGE,
this.createTreeView
);

if (!this._treeView) {
this._treeView = vscode.window.createTreeView(
this._treeView = createTrackedTreeView(
'mongoDBConnectionExplorer',
{
treeDataProvider: this._treeController,
}
this._treeController,
this._telemetryService
);
this._treeController.activateTreeViewEventHandlers(this._treeView);
}
};

activateConnectionsTreeView(): void {
public activateConnectionsTreeView(): void {
// Listen for a change in connections to occur before we create the tree
// so that we show the `viewsWelcome` before any connections are added.
this._connectionController.addEventListener(
Expand All @@ -41,7 +46,7 @@ export default class ExplorerController {
);
}

deactivate(): void {
public deactivate(): void {
if (this._treeController) {
this._treeController.removeListeners();
}
Expand All @@ -52,7 +57,7 @@ export default class ExplorerController {
}
}

refresh(): boolean {
public refresh(): boolean {
if (this._treeController) {
return this._treeController.refresh();
}
Expand Down
50 changes: 35 additions & 15 deletions src/explorer/explorerTreeController.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,19 +9,25 @@ import { DOCUMENT_LIST_ITEM, CollectionTypes } from './documentListTreeItem';
import EXTENSION_COMMANDS from '../commands';
import { sortTreeItemsByLabel } from './treeItemUtils';
import type { LoadedConnection } from '../storage/connectionStorage';
import {
TreeItemExpandedTelemetryEvent,
type TelemetryService,
} from '../telemetry';
import type TreeItemParentInterface from './treeItemParentInterface';

const log = createLogger('explorer tree controller');

export default class ExplorerTreeController
implements vscode.TreeDataProvider<vscode.TreeItem>
{
private _connectionController: ConnectionController;
private _connectionTreeItems: { [key: string]: ConnectionTreeItem };

constructor(connectionController: ConnectionController) {
constructor(
private _connectionController: ConnectionController,
private _telemetryService: TelemetryService
) {
this._onDidChangeTreeData = new vscode.EventEmitter<void>();
this.onDidChangeTreeData = this._onDidChangeTreeData.event;
this._connectionController = connectionController;

// Subscribe to changes in the connections.
this._connectionController.addEventListener(
Expand All @@ -46,14 +52,19 @@ export default class ExplorerTreeController
activateTreeViewEventHandlers = (
treeView: vscode.TreeView<vscode.TreeItem>
): void => {
treeView.onDidCollapseElement((event: any) => {
treeView.onDidCollapseElement((event) => {
log.info('Tree item was collapsed', event.element.label);

if (event.element.onDidCollapse) {
event.element.onDidCollapse();
const treeItem = event.element as vscode.TreeItem &
TreeItemParentInterface;
if (treeItem.onDidCollapse) {
treeItem.onDidCollapse();
}

if (event.element.doesNotRequireTreeUpdate) {
if (
'doesNotRequireTreeUpdate' in treeItem &&
treeItem.doesNotRequireTreeUpdate
) {
// When the element is already loaded (synchronous), we do not need to
// fully refresh the tree.
return;
Expand All @@ -62,20 +73,29 @@ export default class ExplorerTreeController
this._onTreeItemUpdate();
});

treeView.onDidExpandElement(async (event: any): Promise<void> => {
log.info('Connection tree item was expanded', {
connectionId: event.element.connectionId,
connectionName: event.element.label,
isExpanded: event.element.isExpanded,
treeView.onDidExpandElement(async (event): Promise<void> => {
const treeItem = event.element as vscode.TreeItem &
TreeItemParentInterface;
this._telemetryService.track(
new TreeItemExpandedTelemetryEvent(treeItem)
);

log.info('Explorer tree item was expanded', {
type: treeItem.contextValue,
connectionName: treeItem.label,
isExpanded: treeItem.isExpanded,
});

if (!event.element.onDidExpand) {
if (!treeItem.onDidExpand) {
Copy link
Contributor

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.

Copy link
Contributor Author

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.

Copy link
Contributor

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.

return;
}

await event.element.onDidExpand();
await treeItem.onDidExpand();

if (event.element.doesNotRequireTreeUpdate) {
if (
'doesNotRequireTreeUpdate' in treeItem &&
treeItem.doesNotRequireTreeUpdate
) {
// When the element is already loaded (synchronous), we do not
// need to fully refresh the tree.
return;
Expand Down
17 changes: 10 additions & 7 deletions src/explorer/helpExplorer.ts
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();
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need _telemetryService in the constructor?

Copy link
Contributor

Choose a reason for hiding this comment

The 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

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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 like it was previously. I think that way is cleaner architecturally as I see it as a dependency we set once for the lifetime of the controller. Obviously, there's not a huge difference with the HelpExplorer because it's a fairly minimalistic class, but if you consider a more complex controller that has multiple methods which need to log some telemetry events, it becomes much easier to reason about how the dependencies are satisfied.

}

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
);
}
}
Expand Down
13 changes: 7 additions & 6 deletions src/explorer/playgroundsExplorer.ts
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();
Copy link
Contributor

@alenakhineika alenakhineika Jan 28, 2025

Choose a reason for hiding this comment

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

Same here. Doesn't seem we use _telemetryService in the constructor.

}

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);
}
Expand Down
9 changes: 5 additions & 4 deletions src/mdbExtensionController.ts
Original file line number Diff line number Diff line change
Expand Up @@ -103,10 +103,11 @@ export default class MDBExtensionController implements vscode.Disposable {
});
this._languageServerController = new LanguageServerController(context);
this._explorerController = new ExplorerController(
this._connectionController
this._connectionController,
this._telemetryService
);
this._helpExplorer = new HelpExplorer();
this._playgroundsExplorer = new PlaygroundsExplorer();
this._helpExplorer = new HelpExplorer(this._telemetryService);
this._playgroundsExplorer = new PlaygroundsExplorer(this._telemetryService);
this._editDocumentCodeLensProvider = new EditDocumentCodeLensProvider(
this._connectionController
);
Expand Down Expand Up @@ -175,7 +176,7 @@ export default class MDBExtensionController implements vscode.Disposable {

async activate(): Promise<void> {
this._explorerController.activateConnectionsTreeView();
this._helpExplorer.activateHelpTreeView(this._telemetryService);
this._helpExplorer.activateHelpTreeView();
this._playgroundsExplorer.activatePlaygroundsTreeView();
this._telemetryService.activateSegmentAnalytics();
this._participantController.createParticipant(this._context);
Expand Down
50 changes: 47 additions & 3 deletions src/telemetry/telemetryEvents.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 */
Expand Down Expand Up @@ -118,7 +125,7 @@ export class PlaygroundExecutedTelemetryEvent implements TelemetryEventBase {
return 'query';
}

return 'other';
return shellApiType;
}
}

Expand Down Expand Up @@ -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 SidePanelOpenedTelemetryEvent implements TelemetryEventBase {
type = 'Side Panel Opened';
properties: {};

constructor() {
this.properties = {};
}
}

/**
* Reported when a tree item from the collection explorer is expanded.
*/
export class TreeItemExpandedTelemetryEvent implements TelemetryEventBase {
Copy link
Contributor

Choose a reason for hiding this comment

The 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

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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';
Copy link
Contributor

Choose a reason for hiding this comment

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

Is Section a tree item? If yes, maybe Tree Item Expanded would be more transparent than Section Expanded.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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
Expand All @@ -649,4 +691,6 @@ export type TelemetryEvent =
| ParticipantChatOpenedFromActionTelemetryEvent
| ParticipantInputBoxSubmittedTelemetryEvent
| ParticipantResponseGeneratedTelemetryEvent
| PresetConnectionEditedTelemetryEvent;
| PresetConnectionEditedTelemetryEvent
| SidePanelOpenedTelemetryEvent
| TreeItemExpandedTelemetryEvent;
Loading
Loading