NEW @W-17915999@ Added telemetry collection for run action #1778

jfeingold35 · 2025-04-23T19:34:01Z

No description provided.

jfeingold35 · 2025-04-23T19:36:01Z

src/lib/actions/RunAction.ts

+
+	private emitEngineExecutionTelemetry(ruleSelection: RuleSelection, results: RunResults, coreEngineNames: string[]): Promise<void> {
+		const selectedEngineNames: Set<string> = new Set(ruleSelection.getEngineNames());
+		const executedEngineNames: Set<string> = new Set(results.getEngineNames());


Turns out that some of the tests started failing when I assumed that the ruleSelection and results would have the same .getEngineNames() results, so I added the separate boolean for execution.

? How could this ever be the case? Are you sure the tests are not incorrect?
Is it because the results.getEngineNames return the uninstantiated engines as well but the ruleSelection doesn't?

I'm not entirely sure. I'll look further into it.

jfeingold35 · 2025-04-23T19:38:06Z

src/lib/actions/RunAction.ts

 	pluginsFactory: EnginePluginsFactory;
 	logEventListeners: LogEventListener[];
 	progressListeners: ProgressEventListener[];
+	telemetryEmitter?: TelemetryEmitter;


I deviated from the pattern I've been using with the Listeners.

jfeingold35 · 2025-04-23T19:38:44Z

src/lib/actions/RunAction.ts

 		// LogEventListeners should start listening as soon as the Core is instantiated, since Core can start emitting
 		// events they listen for basically immediately.
 		this.dependencies.logEventListeners.forEach(listener => listener.listen(core));
+		const telemetryListener: TelemetryEventListener = new TelemetryEventListener(this.dependencies.telemetryEmitter ?? new NoOpTelemetryEmitter());


We only instantiate one listener, and we'll use either the one we were given or a NoOp one. This should help avoid some messy nullchecks.

I don't understand this line. When would telemetryEmitter ever be null?

jfeingold35 · 2025-04-23T19:39:28Z

test/commands/code-analyzer/run.test.ts

 			return Promise.resolve();
 		});
+		receivedTelemetryEmissions = [];
+		jest.spyOn(SfCliTelemetryEmitter.prototype, 'emitTelemetry').mockImplementation((source, eventName, data) => {


I know this isn't the preferred way of doing this sort of test, but it was the best option available.

stephen-carter-at-sf · 2025-04-23T21:19:01Z

src/lib/actions/RunAction.ts

 	pluginsFactory: EnginePluginsFactory;
 	logEventListeners: LogEventListener[];
 	progressListeners: ProgressEventListener[];
+	telemetryEmitter?: TelemetryEmitter;


When would you ever have this undefined?

stephen-carter-at-sf · 2025-04-23T21:19:22Z

src/lib/actions/RunAction.ts

 		// LogEventListeners should start listening as soon as the Core is instantiated, since Core can start emitting
 		// events they listen for basically immediately.
 		this.dependencies.logEventListeners.forEach(listener => listener.listen(core));
+		const telemetryListener: TelemetryEventListener = new TelemetryEventListener(this.dependencies.telemetryEmitter ?? new NoOpTelemetryEmitter());


I don't understand this line. When would telemetryEmitter ever be null?

stephen-carter-at-sf · 2025-04-23T21:22:55Z

src/lib/actions/RunAction.ts

+			engineTelemetryObject[`${coreEngineName}_selected`] = selected;
+			engineTelemetryObject[`${coreEngineName}_executed`] = executed;


I do not understand this.

Is it that the results.getEngineNames is returning the uninstantiated engines as well?

stephen-carter-at-sf · 2025-04-24T18:38:35Z

src/lib/actions/RunAction.ts

+			if (!selectedEngineNames.has(coreEngineName)) {
+				continue;
+			}


Do you really need to pass in coreEngineNames. Let's just loop over selectedEngineNames. Then back on line 81... we don't need to get the engine names from the plugins.

We have to use coreEngineNames in order to limit ourselves to the plugins that we ourselves ship. Otherwise, we're collecting telemetry on possibly-user-created engines, which is apparently a no-no.

Oh you are saying that enginePlugins which you get from the this.dependencies.pluginsFactory.create(); doesn't include the dynamic engines. OK makes sense.

stephen-carter-at-sf · 2025-04-24T18:44:08Z

src/lib/actions/RunAction.ts

+		const telemetryPromises: Promise<void>[] = [];
+		for (const selectionTelemetry of selectionTelemetryEvents) {
+			telemetryPromises.push(this.dependencies.telemetryEmitter.emitTelemetry('RunAction', Constants.TelemetryEventName, selectionTelemetry));
+		}
+		for (const executionTelemetry of executionTelemetryEvents) {
+			telemetryPromises.push(this.dependencies.telemetryEmitter.emitTelemetry('RunAction', Constants.TelemetryEventName, executionTelemetry));
+		}
+		return telemetryPromises;


If you go over into lib/Telemetry.ts
and make

export interface TelemetryEmitter { emitTelemetry(source: string, eventName: string, data: TelemetryData): Promise<void>; } export class SfCliTelemetryEmitter implements TelemetryEmitter { // istanbul ignore next - No sense in covering SF-CLI core code. public emitTelemetry(source: string, eventName: string, data: TelemetryData): Promise<void> { return Lifecycle.getInstance().emitTelemetry({ ...data, source, eventName }); } }

instead be the following (since we won't be awaiting these anyway):

export interface TelemetryEmitter { emitTelemetry(source: string, eventName: string, data: TelemetryData): void; } export class SfCliTelemetryEmitter implements TelemetryEmitter { // istanbul ignore next - No sense in covering SF-CLI core code. public emitTelemetry(source: string, eventName: string, data: TelemetryData): void { void Lifecycle.getInstance().emitTelemetry({ ...data, source, eventName }); } }

then you won't need to create on line 103 and 104 these arrays, to then fill them, to then create promise arrays, to then push them, etc.

Instead you should be able to just immediately call the emitTelemetry('RunAction', ...) on lines 109 and 115.

Yeah, that's reasonable.

@W-17915999@ Added telemetry collection for run action

29eb583

jfeingold35 commented Apr 23, 2025

View reviewed changes

jfeingold35 added 2 commits April 23, 2025 15:32

@W-17915999@ Feedback from code review

cb5b38d

@W-17915999@ Void-ing dangling promises

aad9d84

jfeingold35 changed the title ~~@W-17915999@ Added telemetry collection for run action~~ NEW @W-17915999@ Added telemetry collection for run action Apr 23, 2025

jfeingold35 marked this pull request as ready for review April 23, 2025 20:59

stephen-carter-at-sf reviewed Apr 23, 2025

View reviewed changes

jfeingold35 added 3 commits April 24, 2025 09:39

@W-17915999@ Feedback from code review

14f3df4

@W-17915999@ Fixed silly oversight.

5853c8a

@W-17915999@ Updated to latest SFGE

0ba14f8

jfeingold35 force-pushed the d/W-17915999 branch from 3444516 to 0ba14f8 Compare April 24, 2025 14:50

jfeingold35 added 2 commits April 24, 2025 11:19

@W-17915999@ Removing unnecessary conditional

9f0ba88

@W-17915999@ Splitting events

9c23689

stephen-carter-at-sf reviewed Apr 24, 2025

View reviewed changes

@W-17915999@ Feedback from code review

d5460b2

stephen-carter-at-sf approved these changes Apr 24, 2025

View reviewed changes

jfeingold35 merged commit 5618e09 into dev Apr 24, 2025
12 checks passed

		engineTelemetryObject[`${coreEngineName}_selected`] = selected;
		engineTelemetryObject[`${coreEngineName}_executed`] = executed;

NEW @W-17915999@ Added telemetry collection for run action #1778

NEW @W-17915999@ Added telemetry collection for run action #1778

Uh oh!

Conversation

jfeingold35 commented Apr 23, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants