CHANGE @W-17526120@ Telemetry for A4D events #187

jfeingold35 · 2025-03-04T22:06:15Z

No description provided.

stephen-carter-at-sf · 2025-03-04T22:25:36Z

src/lib/actions/diff-accept-action.ts

+export type DiffAcceptDependencies = {
+	callback: DiffAcceptCallback;
+	telemetryService: TelemetryService;
+};


Any advantage of having this dependencies type instead of just passing in the telemetry service and callback directly into the constructor?

The object keeps the invocation clean and means I don't have to worry about parameters. It also means we can make any of them optional without needing to reorder them or anything like that.

stephen-carter-at-sf · 2025-03-04T22:28:07Z

src/lib/actions/diff-create-action.ts

+			this.telemetryService.sendException(Constants.TELEM_DIFF_SUGGESTION_FAILED, errMsg, {
+				executedCommand: this.commandName,
+				duration: (Date.now() - startTime).toString()
+			});


Out of curiosity, what does this do? Where does the exception go?
And do we want to swallow this exception or rethrow it?

It sends a telemetry event indicating that we got an exception. I'm not entirely sure where they go, but we've already been doing this style of telemetry for when SFCA fails.
That said, you're almost certainly right that we should be rethrowing instead of swallowing, though in practice it's probably not terribly relevant because the accept/reject functionality should be pretty stable.

stephen-carter-at-sf · 2025-03-04T22:35:03Z

src/lib/actions/diff-reject-action.ts

+			await this.callback(diffHunk);
+			this.telemetryService.sendCommandEvent(Constants.TELEM_DIFF_REJECT, {
+				commandName: this.commandName,
+				commandSource: 'PLACEHOLDER', // DECIDE WHAT SHOULD GO HERE


If you look at https://salesforce.quip.com/1ZgwAbICFda1 you'll see example commandSource values from other teams. My guess is we'll want to say something like SalesforceCodeAnalyzer or SFCA or something. Check with Joni Fazo.

Also I wonder how if we are missing anything else on this event. If you haven't done so already look through all the links in that doc which show how other teams are sending in the events.

Yeah, this is one of the things I need to refine.

randi274 · 2025-03-05T15:52:36Z

src/lib/actions/diff-accept-action.ts

+				commandName: this.commandName,
+				commandSource: 'PLACEHOLDER', // DECIDE WHAT SHOULD GO HERE
+				completionNumLines: lines.toString(),
+				languageType: 'apex' // Apex is the only language A4D codegen supports at present.


Does CodeAnalyzer provide fixes for other languages? A4D supports all of the Salesforce languages today

CodeAnalyzer only supports callouts to A4D for a small handful of PMD rules, which are all Apex-based.

randi274 · 2025-03-05T17:52:58Z

src/lib/constants.ts

+export const TELEM_DIFF_SUGGESTION_FAILED = 'sfdx__eGPT_suggest_failure';
+export const TELEM_DIFF_ACCEPT = 'sfdx__eGPT_accept';
+export const TELEM_DIFF_ACCEPT_FAILED = 'sfdx__eGPT_accept_failure';
+export const TELEM_DIFF_REJECT = 'sfdx__eGPT_reject';


I believe the team uses "sfdx__eGPT_clear" to track explicit rejections like this.

Okay, I'll switch to that.

randi274 · 2025-03-05T17:54:34Z

src/lib/constants.ts

@@ -28,6 +28,12 @@ export const TELEM_FAILED_STATIC_ANALYSIS = 'sfdx__codeanalyzer_static_run_faile
 export const TELEM_SUCCESSFUL_DFA_ANALYSIS = 'sfdx__codeanalyzer_dfa_run_complete';
 export const TELEM_FAILED_DFA_ANALYSIS = 'sfdx__codeanalyzer_dfa_run_failed';
 export const TELEM_SUCCESSFUL_APEX_GURU_FILE_ANALYSIS = 'sfdx__apexguru_file_run_complete';
+export const TELEM_DIFF_SUGGESTION = 'sfdx__eGPT_suggest';
+export const TELEM_DIFF_SUGGESTION_FAILED = 'sfdx__eGPT_suggest_failure';
+export const TELEM_DIFF_ACCEPT = 'sfdx__eGPT_accept';


I would suggest we also fire out own accept events, as A4D has reasons for monitoring their telemetry, but it would be good to keep our telemetry events consistent as well

randi274 · 2025-03-05T17:55:36Z

src/lib/constants.ts

@@ -28,6 +28,12 @@ export const TELEM_FAILED_STATIC_ANALYSIS = 'sfdx__codeanalyzer_static_run_faile
 export const TELEM_SUCCESSFUL_DFA_ANALYSIS = 'sfdx__codeanalyzer_dfa_run_complete';
 export const TELEM_FAILED_DFA_ANALYSIS = 'sfdx__codeanalyzer_dfa_run_failed';
 export const TELEM_SUCCESSFUL_APEX_GURU_FILE_ANALYSIS = 'sfdx__apexguru_file_run_complete';
+export const TELEM_DIFF_SUGGESTION = 'sfdx__eGPT_suggest';
+export const TELEM_DIFF_SUGGESTION_FAILED = 'sfdx__eGPT_suggest_failure';


All of the failure states are usually bucketed under "sfdx__eGPT_error" but I'm not sure we need to inform A4D of anything other than accept events. For our own purposes, this would be good to log though!

Realistically, any of these error cases actually happening is phenomenally low. I'm just programming a little defensively here.

stephen-carter-at-sf · 2025-03-05T20:43:14Z

src/lib/actions/diff-accept-action.ts

+		try {
+			const lines: number = await this.callback(diffHunk);
+			this.telemetryService.sendCommandEvent(Constants.TELEM_DIFF_ACCEPT, {
+				commandSource: this.commandName,


Can you make this

commandSource: 'SFCA_A4D_FIX.' + this.commandName,

instead?

Ideally we should be sending the context which initiated the diff from our A4D quick fix button. That is, we should be updating this line:
https://github.com/forcedotcom/sfdx-code-analyzer-vscode/blob/dev/src/modelBasedFixers/apex-pmd-violations-fixer.ts#L86 with the 'SFCA_A4D_FIX' context to be sent to the callback of https://github.com/forcedotcom/sfdx-code-analyzer-vscode/blob/dev/src/extension.ts#L236
and from there you can pass in the 'SFCA_A4D_FIX' string to the DiffCreateAction constructor, and so forth.

stephen-carter-at-sf · 2025-03-05T20:43:24Z

src/lib/actions/diff-create-action.ts

+		try {
+			await this.callback(code, file);
+			this.telemetryService.sendCommandEvent(Constants.TELEM_DIFF_SUGGESTION, {
+				commandSource: this.commandName,


stephen-carter-at-sf · 2025-03-05T20:43:32Z

src/lib/actions/diff-reject-action.ts

+	private readonly telemetryService: TelemetryService;
+
+	public constructor(commandName: string, dependencies: DiffRejectDependencies) {
+		this.commandName = commandName;


stephen-carter-at-sf · 2025-03-05T21:00:30Z

src/modelBasedFixers/apex-pmd-violations-fixer.ts

@@ -83,12 +83,12 @@ export class ApexPmdViolationsFixer implements vscode.CodeActionProvider {

                const updatedFileContent = this.replaceCodeInFile(document.getText(), codeSnippet.trim(), diagnostic.range.start.line + 1, diagnostic.range.end.line + 1, document);
                // Update the command arguments with the resolved code snippet
-                codeAction.command.arguments = [updatedFileContent, document.uri];
+                codeAction.command.arguments = [updatedFileContent, document.uri, Constants.A4D_PREFIX];


Shouldn't Constants.A4D_PREFIX be the first in the array given the callback is:

vscode.commands.registerCommand(Constants.UNIFIED_DIFF, async (source: string, code: string, file?: string) => {

Whoops, yeah.

jfeingold35 force-pushed the d/W-17526120 branch from bf9f8be to 3764ed1 Compare March 4, 2025 22:14

stephen-carter-at-sf reviewed Mar 4, 2025

View reviewed changes

randi274 reviewed Mar 5, 2025

View reviewed changes

@W-17526120@ Added improved telemetry for A4D events

0cc20fe

jfeingold35 force-pushed the d/W-17526120 branch from ab4b34d to 0cc20fe Compare March 5, 2025 19:05

jfeingold35 added 2 commits March 5, 2025 13:37

@W-17526120@ Integrated feedback from code review

4d941eb

@W-17526120@ Implementing feedback from code review

5d5e5bd

jfeingold35 changed the title ~~Partial commit. Will rollback~~ CHANGE @W-17526120@ Telemetry for A4D events Mar 5, 2025

@W-17526120@ Fixing bad comment

e35ceaf

jfeingold35 marked this pull request as ready for review March 5, 2025 20:28

stephen-carter-at-sf requested changes Mar 5, 2025

View reviewed changes

@W-17526120@ Feedback from code review

bbdd593

stephen-carter-at-sf requested changes Mar 5, 2025

View reviewed changes

@W-17526120@ Final bugfix

dd03930

stephen-carter-at-sf approved these changes Mar 5, 2025

View reviewed changes

jfeingold35 merged commit 1cead2c into dev Mar 5, 2025
8 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CHANGE @W-17526120@ Telemetry for A4D events #187

CHANGE @W-17526120@ Telemetry for A4D events #187

jfeingold35 commented Mar 4, 2025

stephen-carter-at-sf Mar 4, 2025

jfeingold35 Mar 4, 2025

stephen-carter-at-sf Mar 4, 2025

jfeingold35 Mar 4, 2025

stephen-carter-at-sf Mar 4, 2025

jfeingold35 Mar 4, 2025

randi274 Mar 5, 2025

jfeingold35 Mar 5, 2025

randi274 Mar 5, 2025

jfeingold35 Mar 5, 2025

randi274 Mar 5, 2025

randi274 Mar 5, 2025

jfeingold35 Mar 5, 2025

stephen-carter-at-sf Mar 5, 2025

stephen-carter-at-sf Mar 5, 2025

stephen-carter-at-sf Mar 5, 2025

stephen-carter-at-sf Mar 5, 2025

jfeingold35 Mar 5, 2025

CHANGE @W-17526120@ Telemetry for A4D events #187

CHANGE @W-17526120@ Telemetry for A4D events #187

Conversation

jfeingold35 commented Mar 4, 2025

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

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

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