-
Notifications
You must be signed in to change notification settings - Fork 2
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
Changes from all commits
0cc20fe
4d941eb
5d5e5bd
e35ceaf
bbdd593
dd03930
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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; | ||
}; | ||
|
||
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, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you make this
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: |
||
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() | ||
}) | ||
} | ||
} | ||
} |
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, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Out of curiosity, what does this do? Where does the exception go? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||
} | ||
} | ||
} |
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; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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() | ||
}) | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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'; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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! There was a problem hiding this comment. Choose a reason for hiding this commentThe 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'; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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'; | ||
|
@@ -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'; |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.