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

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

Merged
merged 6 commits into from
Mar 5, 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
47 changes: 37 additions & 10 deletions src/extension.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,9 @@ import {SfCli} from './lib/sf-cli';

import {Displayable, ProgressNotification, UxDisplay} from './lib/display';
import {DiagnosticManager, DiagnosticConvertible, DiagnosticManagerImpl} from './lib/diagnostics';
import {DiffCreateAction} from './lib/actions/diff-create-action';
import { DiffAcceptAction } from './lib/actions/diff-accept-action';
import { DiffRejectAction } from './lib/actions/diff-reject-action';
import {ScannerAction} from './lib/actions/scanner-action';
import { CliScannerV4Strategy } from './lib/scanner-strategies/v4-scanner';
import { CliScannerV5Strategy } from './lib/scanner-strategies/v5-scanner';
Expand Down Expand Up @@ -225,21 +228,33 @@ export async function activate(context: vscode.ExtensionContext): Promise<vscode
context.subscriptions.push(documentSaveListener);

telemetryService.sendExtensionActivationEvent(extensionHrStart);
setupUnifiedDiff(context, diagnosticManager);
setupUnifiedDiff(context, diagnosticManager, telemetryService);
outputChannel.appendLine(`Extension sfdx-code-analyzer-vscode activated.`);
return Promise.resolve(context);
}


function setupUnifiedDiff(context: vscode.ExtensionContext, diagnosticManager: DiagnosticManager) {
function setupUnifiedDiff(context: vscode.ExtensionContext, diagnosticManager: DiagnosticManager, telemetryService: TelemetryService) {
context.subscriptions.push(
vscode.commands.registerCommand(Constants.UNIFIED_DIFF, async (code: string, file?: string) => {
vscode.commands.registerCommand(Constants.UNIFIED_DIFF, async (source: string, code: string, file?: string) => {
await (new DiffCreateAction(`${source}.${Constants.UNIFIED_DIFF}`, {
callback: (code: string, file?: string) => VSCodeUnifiedDiff.singleton.unifiedDiff(code, file),
telemetryService
})).run(code, file);
await VSCodeUnifiedDiff.singleton.unifiedDiff(code, file);
})
);
context.subscriptions.push(
vscode.commands.registerCommand(CODEGENIE_UNIFIED_DIFF_ACCEPT, async (hunk: DiffHunk) => {
await VSCodeUnifiedDiff.singleton.unifiedDiffAccept(hunk);
// TODO: The use of the prefix shouldn't be hardcoded. Ideally, it should be passed in as an argument to the command.
// But that would require us to make changes to the underlying UnifiedDiff code that we're not currently in a position to make.
await (new DiffAcceptAction(`${Constants.A4D_PREFIX}.${CODEGENIE_UNIFIED_DIFF_ACCEPT}`, {
callback: async (diffHunk: DiffHunk) => {
await VSCodeUnifiedDiff.singleton.unifiedDiffAccept(diffHunk);
return diffHunk.lines.length;
},
telemetryService
})).run(hunk);
// For accept & accept all, it is tricky to track the diagnostics and the changed lines as multiple fixes are requested.
// Hence, we save the file and rerun the scan instead.
await vscode.window.activeTextEditor.document.save();
Expand All @@ -252,12 +267,22 @@ function setupUnifiedDiff(context: vscode.ExtensionContext, diagnosticManager: D
);
context.subscriptions.push(
vscode.commands.registerCommand(CODEGENIE_UNIFIED_DIFF_REJECT, async (hunk: DiffHunk) => {
await VSCodeUnifiedDiff.singleton.unifiedDiffReject(hunk);
// TODO: The use of the prefix shouldn't be hardcoded. Ideally, it should be passed in as an argument to the command.
// But that would require us to make changes to the underlying UnifiedDiff code that we're not currently in a position to make.
await (new DiffRejectAction(`${Constants.A4D_PREFIX}.${CODEGENIE_UNIFIED_DIFF_REJECT}`, {
callback: (diffHunk: DiffHunk) => VSCodeUnifiedDiff.singleton.unifiedDiffReject(diffHunk),
telemetryService
})).run(hunk);
})
);
context.subscriptions.push(
vscode.commands.registerCommand(CODEGENIE_UNIFIED_DIFF_ACCEPT_ALL, async () => {
await VSCodeUnifiedDiff.singleton.unifiedDiffAcceptAll();
// TODO: The use of the prefix shouldn't be hardcoded. Ideally, it should be passed in as an argument to the command.
// But that would require us to make changes to the underlying UnifiedDiff code that we're not currently in a position to make.
await (new DiffAcceptAction(`${Constants.A4D_PREFIX}.${CODEGENIE_UNIFIED_DIFF_ACCEPT_ALL}`, {
callback: () => VSCodeUnifiedDiff.singleton.unifiedDiffAcceptAll(),
telemetryService
})).run();
await vscode.window.activeTextEditor.document.save();
return _runAndDisplayScanner(Constants.COMMAND_RUN_ON_ACTIVE_FILE, [vscode.window.activeTextEditor.document.fileName], {
telemetryService,
Expand All @@ -268,7 +293,12 @@ function setupUnifiedDiff(context: vscode.ExtensionContext, diagnosticManager: D
);
context.subscriptions.push(
vscode.commands.registerCommand(CODEGENIE_UNIFIED_DIFF_REJECT_ALL, async () => {
await VSCodeUnifiedDiff.singleton.unifiedDiffRejectAll();
// TODO: The use of the prefix shouldn't be hardcoded. Ideally, it should be passed in as an argument to the command.
// But that would require us to make changes to the underlying UnifiedDiff code that we're not currently in a position to make.
await (new DiffRejectAction(`${Constants.A4D_PREFIX}.${CODEGENIE_UNIFIED_DIFF_REJECT_ALL}`, {
callback: () => VSCodeUnifiedDiff.singleton.unifiedDiffRejectAll(),
telemetryService
})).run();
})
);
VSCodeUnifiedDiff.singleton.activate(context);
Expand Down Expand Up @@ -498,7 +528,6 @@ export async function _runAndDisplayScanner(commandName: string, targets: string
});
} catch (e) {
const errMsg = e instanceof Error ? e.message : e as string;
console.log(errMsg);
telemetryService.sendException(Constants.TELEM_FAILED_STATIC_ANALYSIS, errMsg, {
executedCommand: commandName,
duration: (Date.now() - startTime).toString()
Expand Down Expand Up @@ -547,7 +576,6 @@ export async function _runAndDisplayDfa(context:vscode.ExtensionContext ,runInfo
})
} catch (e) {
const errMsg = e instanceof Error ? e.message : e as string;
console.log(errMsg);
telemetryService.sendException(Constants.TELEM_FAILED_DFA_ANALYSIS, errMsg, {
executedCommand: commandName,
duration: (Date.now() - startTime).toString()
Expand Down Expand Up @@ -600,7 +628,6 @@ export async function _clearDiagnosticsForSelectedFiles(selections: vscode.Uri[]
});
} catch (e) {
const errMsg = e instanceof Error ? e.message : e as string;
console.log(errMsg);
telemetryService.sendException(Constants.TELEM_FAILED_STATIC_ANALYSIS, errMsg, {
executedCommand: commandName,
duration: (Date.now() - startTime).toString()
Expand Down
40 changes: 40 additions & 0 deletions src/lib/actions/diff-accept-action.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
import { DiffHunk } from "../../shared/UnifiedDiff";
import { TelemetryService } from "../core-extension-service";
import * as Constants from '../constants';

export type DiffAcceptCallback = (diffHunk?: DiffHunk) => Promise<number>;

export type DiffAcceptDependencies = {
callback: DiffAcceptCallback;
telemetryService: TelemetryService;
};
Comment on lines +7 to +10
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.


export class DiffAcceptAction {
private readonly commandName: string;
private readonly callback: DiffAcceptCallback;
private readonly telemetryService: TelemetryService;

public constructor(commandName: string, dependencies: DiffAcceptDependencies) {
this.commandName = commandName;
this.callback = dependencies.callback;
this.telemetryService = dependencies.telemetryService;
}

public async run(diffHunk?: DiffHunk): Promise<void> {
const startTime = Date.now();
try {
const lines: number = await this.callback(diffHunk);
this.telemetryService.sendCommandEvent(Constants.TELEM_DIFF_ACCEPT, {
commandSource: this.commandName,
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

completionNumLines: lines.toString(),
languageType: 'apex' // The only rules that the CodeAnalyzer A4D integration supports are Apex-based
});
} catch (e) {
const errMsg = e instanceof Error ? e.message : e as string;
this.telemetryService.sendException(Constants.TELEM_DIFF_ACCEPT_FAILED, errMsg, {
executedCommand: this.commandName,
duration: (Date.now() - startTime).toString()
})
}
}
}
38 changes: 38 additions & 0 deletions src/lib/actions/diff-create-action.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
import {TelemetryService} from '../core-extension-service';
import * as Constants from '../constants';

export type DiffCreateCallback = (code: string, file?: string) => Promise<void>;

export type DiffCreateDependencies = {
callback: DiffCreateCallback;
telemetryService: TelemetryService;
};

export class DiffCreateAction {
private readonly commandName: string;
private readonly callback: DiffCreateCallback;
private readonly telemetryService: TelemetryService;

public constructor(commandName: string, dependencies: DiffCreateDependencies) {
this.commandName = commandName;
this.callback = dependencies.callback;
this.telemetryService = dependencies.telemetryService;
}

public async run(code: string, file?: string): Promise<void> {
const startTime = Date.now();
try {
await this.callback(code, file);
this.telemetryService.sendCommandEvent(Constants.TELEM_DIFF_SUGGESTION, {
commandSource: this.commandName,
Copy link
Collaborator

Choose a reason for hiding this comment

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

ditto

languageType: 'apex' // The only rules that the CodeAnalyzer A4D integration supports are Apex-based
});
} catch (e) {
const errMsg = e instanceof Error ? e.message : e as string;
this.telemetryService.sendException(Constants.TELEM_DIFF_SUGGESTION_FAILED, errMsg, {
executedCommand: this.commandName,
duration: (Date.now() - startTime).toString()
});
Comment on lines +32 to +35
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

}
}
}
39 changes: 39 additions & 0 deletions src/lib/actions/diff-reject-action.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
import { DiffHunk } from "../../shared/UnifiedDiff";
import { TelemetryService } from "../core-extension-service";
import * as Constants from '../constants';

export type DiffRejectCallback = (diffHunk?: DiffHunk) => Promise<void>;

export type DiffRejectDependencies = {
callback: DiffRejectCallback;
telemetryService: TelemetryService;
};

export class DiffRejectAction {
private readonly commandName: string;
private readonly callback: DiffRejectCallback;
private readonly telemetryService: TelemetryService;

public constructor(commandName: string, dependencies: DiffRejectDependencies) {
this.commandName = commandName;
Copy link
Collaborator

Choose a reason for hiding this comment

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

ditto

this.callback = dependencies.callback;
this.telemetryService = dependencies.telemetryService;
}

public async run(diffHunk?: DiffHunk): Promise<void> {
const startTime = Date.now();
try {
await this.callback(diffHunk);
this.telemetryService.sendCommandEvent(Constants.TELEM_DIFF_REJECT, {
commandSource: this.commandName,
languageType: 'apex' // The only rules that the CodeAnalyzer A4D integration supports are Apex-based
});
} catch (e) {
const errMsg = e instanceof Error ? e.message : e as string;
this.telemetryService.sendException(Constants.TELEM_DIFF_REJECT_FAILED, errMsg, {
executedCommand: this.commandName,
duration: (Date.now() - startTime).toString()
})
}
}
}
7 changes: 7 additions & 0 deletions src/lib/constants.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Copy link
Collaborator

Choose a reason for hiding this comment

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

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!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

export const TELEM_DIFF_ACCEPT = 'sfdx__eGPT_accept';
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

export const TELEM_DIFF_ACCEPT_FAILED = 'sfdx__eGPT_accept_failure';
export const TELEM_DIFF_REJECT = 'sfdx__eGPT_clear';
export const TELEM_DIFF_REJECT_FAILED = 'sfdx__eGPT_clear_failure';

// versioning
export const MINIMUM_REQUIRED_VERSION_CORE_EXTENSION = '58.4.1';
Expand All @@ -46,3 +52,4 @@ export const APEX_GURU_RETRY_INTERVAL_MILLIS = 1000;
export const ENABLE_A4D_INTEGRATION = false;
export const A4D_FIX_AVAILABLE_RULES = ['ApexCRUDViolation', 'ApexSharingViolations', 'EmptyCatchBlock', 'EmptyTryOrFinallyBlock', 'EmptyWhileStmt', 'EmptyIfStmt'];
export const UNIFIED_DIFF = 'unifiedDiff';
export const A4D_PREFIX = 'SFCA_A4D_FIX';
18 changes: 9 additions & 9 deletions src/modelBasedFixers/apex-pmd-violations-fixer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 = [Constants.A4D_PREFIX, updatedFileContent, document.uri];

return codeAction;
} catch (error) {
const errorMessage = '***Failed to resolve code action:***'
const detailedMessage = error instanceof Error
const detailedMessage = error instanceof Error
? error.message
: String(error);
console.error(errorMessage, error);
Expand Down Expand Up @@ -198,17 +198,17 @@ export class ApexPmdViolationsFixer implements vscode.CodeActionProvider {
if (!document) {
return replaceCode;
}

// Get the indentation of the first line in the range
const startLine = range && range.start.line > 0 ? range.start.line - 1 : 0;
const baseIndentation = this.getLineIndentation(document, startLine);

// Split the replacement code into lines
const lines = replaceCode.split(/\r?\n/);

// First, normalize the code by removing all existing indentation
const normalizedLines = lines.map(line => line.trimStart());

let indentLevel = 0;
let braceLevel = 0;
let parenLevel = 0;
Expand Down Expand Up @@ -250,10 +250,10 @@ export class ApexPmdViolationsFixer implements vscode.CodeActionProvider {

return indentation + line;
});

return formattedLines.join(document.eol === vscode.EndOfLine.CRLF ? '\r\n' : '\n');
}

// Helper to get the indentation of a specific line
private getLineIndentation(document: vscode.TextDocument, lineNumber: number): string {
const lineText = document.lineAt(lineNumber).text;
Expand All @@ -267,7 +267,7 @@ export class ApexPmdViolationsFixer implements vscode.CodeActionProvider {
// This is a known issue and we will fix this as we learn more about how the model sends the responses for other fixes.
const updatedDiagnostics = currentDiagnostics.filter(
diagnostic => (
!Constants.A4D_FIX_AVAILABLE_RULES.includes(this.extractDiagnosticCode(diagnostic)) ||
!Constants.A4D_FIX_AVAILABLE_RULES.includes(this.extractDiagnosticCode(diagnostic)) ||
(diagnostic.range.end.line < range.start.line || diagnostic.range.start.line > range.end.line )
)
);
Expand Down
8 changes: 5 additions & 3 deletions src/shared/UnifiedDiff.ts
Original file line number Diff line number Diff line change
Expand Up @@ -461,14 +461,16 @@ export class VSCodeUnifiedDiff implements vscode.CodeLensProvider, vscode.CodeAc
/**
* Accept all changes in the unified diff.
*/
public async unifiedDiffAcceptAll() {
public async unifiedDiffAcceptAll(): Promise<number> {
const editor = vscode.window.activeTextEditor;
if (!editor) return;
if (!editor) return 0;
const diff = this.unifiedDiffs.get(editor.document.uri.toString());
if (!diff) return;
if (!diff) return 0;
const diffLines: number = diff.getHunks().reduce((prev, curr) => prev + curr.lines.length, 0);
diff.setSourceCode(diff.getTargetCode());
await this.renderUnifiedDiff(editor.document);
this.checkRedundantUnifiedDiff(editor.document);
return diffLines;
}

/**
Expand Down
Loading