chore(telemetry): telemetry improvements VSCODE-673 #914

nirinchev · 2025-01-23T14:23:14Z

Description

This addresses the requested improvements tracked in this document.

Add new Side Panel Opened event when the extension side panel is activated
Add new Section Expanded event when a section is expanded in the connections tree view
~~Update Command Run with source: command_bar | shortcut~~
Update Playground Code Executed to include the type of the command when it's other
~~Update Playground Created, add source_details~~

Motivation and Context

Bugfix
New feature
Dependency update
Misc

Types of changes

Backport Needed
Patch (non-breaking change which fixes an issue)
Minor (non-breaking change which adds functionality)
Major (fix or feature that would cause existing functionality to change)

…tions

src/connectionController.ts

gagik · 2025-01-26T21:40:52Z

src/explorer/explorerTreeController.ts

      });

-      if (!event.element.onDidExpand) {
+      if (!treeItem.onDidExpand) {


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.

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.

I think this part handles three elements that don't have children. I might be mistaken, maybe Rhys knows better.

gagik

Looks good! Just minor non-blocking comment

src/telemetry/telemetryEvents.ts

gagik · 2025-01-28T08:18:27Z

src/telemetry/telemetryEvents.ts

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


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

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.

alenakhineika · 2025-01-28T11:41:34Z

src/explorer/helpExplorer.ts


 export default class HelpExplorer {
  _treeController: HelpTree;
  _treeView?: vscode.TreeView<vscode.TreeItem>;

-  constructor() {
+  constructor(private _telemetryService: TelemetryService) {


Do we need _telemetryService in the constructor?

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

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.

alenakhineika · 2025-01-28T11:42:06Z

src/explorer/playgroundsExplorer.ts


 export default class PlaygroundsExplorer {
  private _treeController: PlaygroundsTree;
  private _treeView?: vscode.TreeView<vscode.TreeItem>;

-  constructor() {
+  constructor(private _telemetryService: TelemetryService) {


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

alenakhineika · 2025-01-28T11:45:40Z

src/telemetry/telemetryEvents.ts

+ * Reported when a tree item from the collection explorer is expanded.
+ */
+export class TreeItemExpandedTelemetryEvent implements TelemetryEventBase {
+  type = 'Section Expanded';


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

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.

alenakhineika · 2025-01-28T11:50:59Z

src/telemetry/telemetryService.ts

@@ -189,4 +191,12 @@ export class TelemetryService {
      new ParticipantResponseFailedTelemetryEvent(command, errorName, errorCode)
    );
  }
+
+  trackTreeViewActivated: () => void = throttle(


Why to we want to throttle here?

Due to the way the extension works, there's no integration point we can use to be notified that the side panel is opened, which is why we're piggy-backing on the tree visibility event. However, since we have 3 trees in the side panel (connections, playgrounds, help), that event would get fired 3 times every time we open the sidepanel. Rather than deal with this in post-processing, I figured throttling would be a reasonable way to reduce the noise.

chore(telemetry): add Side Panel Opened event

e53ae4d

nirinchev requested a review from gagik January 23, 2025 14:23

nirinchev self-assigned this Jan 23, 2025

nirinchev changed the title ~~chore(telemetry): telemetry improvements~~ chore(telemetry): telemetry improvements VSCODE-673 Jan 23, 2025

nirinchev added 2 commits January 23, 2025 23:37

Report expanding tree items

1b54bf1

Report the shell API type when not mapped to one of the predefined op…

d288e97

…tions

gagik reviewed Jan 24, 2025

View reviewed changes

src/connectionController.ts Outdated Show resolved Hide resolved

fix and simplify tests

1e6a96d

nirinchev requested a review from gagik January 24, 2025 17:13

nirinchev marked this pull request as ready for review January 24, 2025 17:13

gagik reviewed Jan 26, 2025

View reviewed changes

nirinchev requested a review from gagik January 28, 2025 07:50

Merge branch 'main' into ni/telemetry-additions

006176b

nirinchev requested a review from alenakhineika January 28, 2025 08:10

gagik approved these changes Jan 28, 2025

View reviewed changes

Update class name

df29dac

alenakhineika reviewed Jan 28, 2025

View reviewed changes

nirinchev merged commit 390d75b into main Jan 29, 2025
9 checks passed

nirinchev deleted the ni/telemetry-additions branch January 29, 2025 12:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore(telemetry): telemetry improvements VSCODE-673 #914

chore(telemetry): telemetry improvements VSCODE-673 #914

nirinchev commented Jan 23, 2025 •

edited

Loading

gagik Jan 26, 2025

nirinchev Jan 27, 2025

alenakhineika Jan 28, 2025

gagik left a comment

gagik Jan 28, 2025

nirinchev Jan 28, 2025

alenakhineika Jan 28, 2025

gagik Jan 28, 2025

nirinchev Jan 28, 2025

alenakhineika Jan 28, 2025 •

edited

Loading

alenakhineika Jan 28, 2025

nirinchev Jan 28, 2025

alenakhineika Jan 28, 2025

nirinchev Jan 28, 2025

chore(telemetry): telemetry improvements VSCODE-673 #914

chore(telemetry): telemetry improvements VSCODE-673 #914

Conversation

nirinchev commented Jan 23, 2025 • edited Loading

Description

Motivation and Context

Types of changes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gagik left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alenakhineika Jan 28, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nirinchev commented Jan 23, 2025 •

edited

Loading

alenakhineika Jan 28, 2025 •

edited

Loading