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

Adding additional telemetry on CodeActions #152383

Merged
merged 15 commits into from
Jun 23, 2022

Conversation

justschen
Copy link
Contributor

Building on top of PR #152081 for issue #151140 (for code actions telemetry and refactoring)

@justschen
Copy link
Contributor Author

Results from telemetry look like this.

From lightbulb, where user cancelled selection:
telemetry results 2

From refactor, where user selected an action, and applyCodeAction was called:
telemetry results

@justschen
Copy link
Contributor Author

also currently having this issue - leads to compilation, unit, and integration tests to fail
error message

Copy link
Collaborator

@mjbvz mjbvz left a comment

Choose a reason for hiding this comment

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

Adding some initial comments. Let me know if you have any questions about them

src/vs/editor/contrib/codeAction/browser/codeActionMenu.ts Outdated Show resolved Hide resolved
src/vs/editor/contrib/codeAction/browser/codeActionMenu.ts Outdated Show resolved Hide resolved
src/vs/editor/contrib/codeAction/browser/codeActionMenu.ts Outdated Show resolved Hide resolved
};

this._telemetryService.publicLog2<ApplyCodeActionEvent, ApplyCodeEventClassification>('codeAction.applyCodeAction', {
codeActionFrom: <CodeMenuOpenedFrom>openedFromString,
Copy link
Collaborator

Choose a reason for hiding this comment

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

You should not need the cast 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.

Left the casting for now - not sure how I could set defaults from the interface (since there were a LOT of times where CodeActionTrigger were being called)

Copy link
Collaborator

Choose a reason for hiding this comment

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

This is because openedFromString is CodeMenuOpenedFrom | undefined. The cast is hiding an error here because we are always expecting a value, not undefined

src/vs/editor/contrib/codeAction/browser/codeActionMenu.ts Outdated Show resolved Hide resolved
src/vs/editor/contrib/codeAction/browser/types.ts Outdated Show resolved Hide resolved
@@ -123,6 +136,7 @@ export interface CodeActionTrigger {
readonly position: Position;
};
readonly preview?: boolean;
readonly triggerAction?: CodeMenuOpenedFrom;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think you should make this required. I just tested this and while it does cause a lot of compile errors, it also reveals cases that should have a triggerAction passed in

};

this._telemetryService.publicLog2<ApplyCodeActionEvent, ApplyCodeEventClassification>('codeAction.applyCodeAction', {
codeActionFrom: <CodeMenuOpenedFrom>openedFromString,
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is because openedFromString is CodeMenuOpenedFrom | undefined. The cast is hiding an error here because we are always expecting a value, not undefined

@justschen justschen merged commit 7cb79a6 into microsoft:main Jun 23, 2022
@github-actions github-actions bot locked and limited conversation to collaborators Aug 7, 2022
@justschen justschen deleted the justin/CodeAction-Telemetry branch August 8, 2024 21:47
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants