-
Notifications
You must be signed in to change notification settings - Fork 51
NEW @W-19150636@ - Add telemetry to Config command and command field … #2024
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
Changes from all commits
6cf8123
6baef05
e25960a
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 |
|---|---|---|
|
|
@@ -117,12 +117,14 @@ export class RunAction { | |
| } | ||
| this.dependencies.telemetryEmitter.emitTelemetry(Constants.TelemetrySource, Constants.TelemetryEventName, { | ||
| sfcaEvent: Constants.CliTelemetryEvents.ENGINE_SELECTION, | ||
| command: Constants.CliCommands.RUN, | ||
|
Contributor
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. Will we get 2 run command telemetry events for ENGINE_SELECTION and ENGINE_EXECUTION? If only the command is reflected in the pft events we will see 2 run metrics for one execution of the command and and since no sfcaEvent is there we will not be able to distinguish b/w the one for ENGINE_SELECTION and the one for ENGINE_EXECUTION. Is this expected behaviour?
Contributor
Author
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. No, this is not a problem. The concern is based on a misunderstanding. What's actually emitted: For a single run command execution, you get 2 telemetry events per engine, and both events include BOTH the sfcaEvent and command fields: Event 1 - ENGINE_SELECTION: Event 2 - ENGINE_EXECUTION: Why you CAN distinguish them:
This is expected and correct behavior:
The sfcaEvent field has always been there to distinguish between selection and execution. The new command field just adds another dimension to identify which CLI command was used. |
||
| engine: coreEngineName, | ||
| ruleCount: ruleSelection.getRulesFor(coreEngineName).length | ||
| }); | ||
|
|
||
| this.dependencies.telemetryEmitter.emitTelemetry(Constants.TelemetrySource, Constants.TelemetryEventName, { | ||
| sfcaEvent: Constants.CliTelemetryEvents.ENGINE_EXECUTION, | ||
| command: Constants.CliCommands.RUN, | ||
| engine: coreEngineName, | ||
| violationCount: results.getEngineRunResults(coreEngineName).getViolationCount() | ||
| }); | ||
|
|
||
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.
This code flow does not seems to be in the path of the config cli command.
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.
in my understanding
This code is definitely in the execution path of the config CLI command. The ConfigAction.execute() method is the main entry point when running sf code-analyzer config, and emitEngineTelemetry() is called directly from within execute() after the rule selection
completes but before the method returns.
The config command performs rule selection internally (via userCore.selectRules()) to determine which rules will be included in the generated config file - this is the core functionality of the command. After the selection completes, we have the actual data about
which engines were selected and how many rules from each engine, so we emit telemetry at that point. This follows the same pattern as the run and rules commands - emit telemetry after you have the selection results.
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.
Makes sense, sorry I got confused here with engine selection.