Skip to content

Commit d923bb8

Browse files
eleanorjboydeleanorjboyd
committed
Track terminal activation outcomes and confirmed state (#1822)
Distinguish shell command success, failure, timeout and unknown completion before updating activation state. Record bounded telemetry for both shell integration and unverified sendText attempts, with regression coverage for deactivation and environment switching.\n\nCo-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
1 parent bde7cf8 commit d923bb8

4 files changed

Lines changed: 301 additions & 94 deletions

File tree

‎src/common/telemetry/constants.ts‎

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -49,6 +49,11 @@ export enum EventNames {
4949
* - errorType: string (error class name, on failure only)
5050
*/
5151
ENVIRONMENT_DISCOVERY = 'ENVIRONMENT_DISCOVERY',
52+
/**
53+
* One event per terminal activation/deactivation command attempt.
54+
* Legacy sendText cannot confirm command completion and is reported as unverified.
55+
*/
56+
TERMINAL_ACTIVATION_OUTCOME = 'TERMINAL.ACTIVATION_OUTCOME',
5257
MANAGER_READY_TIMEOUT = 'MANAGER_READY.TIMEOUT',
5358
/**
5459
* Telemetry event for individual manager registration failure.
@@ -234,6 +239,23 @@ export enum EventNames {
234239

235240
// Map all events to their properties
236241
export interface IEventNamePropertyMapping {
242+
/* __GDPR__
243+
"terminal.activation_outcome": {
244+
"operation": { "classification": "SystemMetaData", "purpose": "FeatureInsight", "owner": "eleanorjboyd" },
245+
"outcome": { "classification": "SystemMetaData", "purpose": "FeatureInsight", "owner": "eleanorjboyd" },
246+
"method": { "classification": "SystemMetaData", "purpose": "FeatureInsight", "owner": "eleanorjboyd" },
247+
"shell": { "classification": "SystemMetaData", "purpose": "FeatureInsight", "owner": "eleanorjboyd" },
248+
"trigger": { "classification": "SystemMetaData", "purpose": "FeatureInsight", "owner": "eleanorjboyd" },
249+
"<duration>": { "classification": "SystemMetaData", "purpose": "FeatureInsight", "isMeasurement": true, "owner": "eleanorjboyd" }
250+
}
251+
*/
252+
[EventNames.TERMINAL_ACTIVATION_OUTCOME]: {
253+
operation: 'activate' | 'deactivate';
254+
outcome: 'succeeded' | 'failed' | 'timedOut' | 'unknown' | 'unverified' | 'noCommand';
255+
method: 'shellIntegration' | 'sendText';
256+
shell: string;
257+
trigger: 'terminalOpen' | 'preExisting' | 'explicit' | 'environmentSwitch' | 'unknown';
258+
};
237259
/* __GDPR__
238260
"extension.activation_duration": {
239261
"duration" : { "classification": "SystemMetaData", "purpose": "FeatureInsight", "owner": "eleanorjboyd" }

‎src/features/terminal/terminalActivationState.ts‎

Lines changed: 75 additions & 90 deletions
Original file line numberDiff line numberDiff line change
@@ -9,10 +9,17 @@ import {
99
} from 'vscode';
1010
import { PythonEnvironment } from '../../api';
1111
import { traceError, traceInfo, traceVerbose } from '../../common/logging';
12+
import { StopWatch } from '../../common/stopWatch';
13+
import { EventNames } from '../../common/telemetry/constants';
14+
import { sendTelemetryEvent } from '../../common/telemetry/sender';
1215
import { onDidEndTerminalShellExecution, onDidStartTerminalShellExecution } from '../../common/window.apis';
1316
import { getActivationCommand, getDeactivationCommand } from '../common/activation';
17+
import { identifyTerminalShell } from '../common/shellDetector';
1418
import { getShellIntegrationTimeout, isTaskTerminal, shouldSkipTerminalActivation } from './utils';
1519

20+
type ActivationTrigger = 'terminalOpen' | 'preExisting' | 'explicit' | 'environmentSwitch' | 'unknown';
21+
type CommandOutcome = 'succeeded' | 'failed' | 'timedOut' | 'unknown' | 'unverified' | 'noCommand';
22+
1623
export interface DidChangeTerminalActivationStateEvent {
1724
terminal: Terminal;
1825
environment: PythonEnvironment;
@@ -21,8 +28,8 @@ export interface DidChangeTerminalActivationStateEvent {
2128

2229
export interface TerminalActivation {
2330
isActivated(terminal: Terminal, environment?: PythonEnvironment): boolean;
24-
activate(terminal: Terminal, environment: PythonEnvironment): Promise<void>;
25-
deactivate(terminal: Terminal): Promise<void>;
31+
activate(terminal: Terminal, environment: PythonEnvironment, trigger?: ActivationTrigger): Promise<void>;
32+
deactivate(terminal: Terminal, trigger?: ActivationTrigger): Promise<void>;
2633
onDidChangeTerminalActivationState: Event<DidChangeTerminalActivationStateEvent>;
2734
}
2835

@@ -50,8 +57,8 @@ export class TerminalActivationImpl implements TerminalActivationInternal {
5057
private onTerminalClosed = this.onTerminalClosedEmitter.event;
5158

5259
private activatedTerminals = new Map<Terminal, PythonEnvironment>();
53-
private activatingTerminals = new Map<Terminal, Promise<void>>();
54-
private deactivatingTerminals = new Map<Terminal, Promise<void>>();
60+
private activatingTerminals = new Map<Terminal, Promise<CommandOutcome>>();
61+
private deactivatingTerminals = new Map<Terminal, Promise<CommandOutcome>>();
5562

5663
constructor() {
5764
this.disposables.push(
@@ -85,7 +92,11 @@ export class TerminalActivationImpl implements TerminalActivationInternal {
8592
return this.activatedTerminals.get(terminal);
8693
}
8794

88-
async activate(terminal: Terminal, environment: PythonEnvironment): Promise<void> {
95+
async activate(
96+
terminal: Terminal,
97+
environment: PythonEnvironment,
98+
trigger: ActivationTrigger = 'unknown',
99+
): Promise<void> {
89100
if (shouldSkipTerminalActivation(terminal)) {
90101
traceVerbose('Skipping activation for this terminal');
91102
return;
@@ -98,12 +109,14 @@ export class TerminalActivationImpl implements TerminalActivationInternal {
98109

99110
if (this.deactivatingTerminals.has(terminal)) {
100111
traceVerbose('Terminal is being deactivated, cannot activate.');
101-
return this.deactivatingTerminals.get(terminal);
112+
await this.deactivatingTerminals.get(terminal);
113+
return;
102114
}
103115

104116
if (this.activatingTerminals.has(terminal)) {
105117
traceVerbose('Terminal is being activated, skipping.');
106-
return this.activatingTerminals.get(terminal);
118+
await this.activatingTerminals.get(terminal);
119+
return;
107120
}
108121

109122
const terminalEnv = this.activatedTerminals.get(terminal);
@@ -115,50 +128,57 @@ export class TerminalActivationImpl implements TerminalActivationInternal {
115128
traceInfo(
116129
`Terminal is activated with a different environment, deactivating: ${terminalEnv.environmentPath.fsPath}`,
117130
);
118-
await this.deactivate(terminal);
131+
await this.deactivate(terminal, 'environmentSwitch');
119132
}
120133
}
121134

122135
try {
123-
const promise = this.activateInternal(terminal, environment);
136+
const promise = this.runCommand(terminal, environment, 'activate', trigger);
124137
traceVerbose(`Activating terminal: ${environment.environmentPath.fsPath}`);
125138
this.activatingTerminals.set(terminal, promise);
126-
await promise;
139+
const outcome = await promise;
127140
this.activatingTerminals.delete(terminal);
128-
this.updateActivationState(terminal, environment, true);
129-
traceInfo(`Terminal is activated: ${environment.environmentPath.fsPath}`);
141+
if (outcome === 'succeeded' || outcome === 'unverified') {
142+
// sendText has no completion signal; preserve its existing optimistic UI behavior.
143+
this.updateActivationState(terminal, environment, true);
144+
traceInfo(`Terminal activation sent: ${environment.environmentPath.fsPath} (${outcome})`);
145+
}
130146
} catch (ex) {
131147
this.activatingTerminals.delete(terminal);
132148
traceError('Failed to activate environment:\r\n', ex);
133149
}
134150
}
135151

136-
async deactivate(terminal: Terminal): Promise<void> {
152+
async deactivate(terminal: Terminal, trigger: ActivationTrigger = 'unknown'): Promise<void> {
137153
if (isTaskTerminal(terminal)) {
138154
traceVerbose('Cannot deactivate environment in a task terminal');
139155
return;
140156
}
141157

142158
if (this.activatingTerminals.has(terminal)) {
143159
traceVerbose('Terminal is being activated, cannot deactivate.');
144-
return this.activatingTerminals.get(terminal);
160+
await this.activatingTerminals.get(terminal);
161+
return;
145162
}
146163

147164
if (this.deactivatingTerminals.has(terminal)) {
148165
traceVerbose('Terminal is being deactivated, skipping.');
149-
return this.deactivatingTerminals.get(terminal);
166+
await this.deactivatingTerminals.get(terminal);
167+
return;
150168
}
151169

152170
const terminalEnv = this.activatedTerminals.get(terminal);
153171
if (terminalEnv) {
154172
try {
155-
const promise = this.deactivateInternal(terminal, terminalEnv);
173+
const promise = this.runCommand(terminal, terminalEnv, 'deactivate', trigger);
156174
traceVerbose(`Deactivating terminal: ${terminalEnv.environmentPath.fsPath}`);
157175
this.deactivatingTerminals.set(terminal, promise);
158-
await promise;
176+
const outcome = await promise;
159177
this.deactivatingTerminals.delete(terminal);
160-
this.updateActivationState(terminal, terminalEnv, false);
161-
traceInfo(`Terminal is deactivated: ${terminalEnv.environmentPath.fsPath}`);
178+
if (outcome === 'succeeded' || outcome === 'unverified') {
179+
this.updateActivationState(terminal, terminalEnv, false);
180+
traceInfo(`Terminal deactivation sent: ${terminalEnv.environmentPath.fsPath} (${outcome})`);
181+
}
162182
} catch (ex) {
163183
this.deactivatingTerminals.delete(terminal);
164184
traceError('Failed to deactivate environment:\r\n', ex);
@@ -183,93 +203,62 @@ export class TerminalActivationImpl implements TerminalActivationInternal {
183203
this.disposables.forEach((d) => d.dispose());
184204
}
185205

186-
private async activateInternal(terminal: Terminal, environment: PythonEnvironment): Promise<void> {
187-
if (terminal.shellIntegration) {
188-
await this.activateUsingShellIntegration(terminal.shellIntegration, terminal, environment);
189-
} else {
190-
this.activateLegacy(terminal, environment);
191-
}
192-
}
193-
194-
private async deactivateInternal(terminal: Terminal, environment: PythonEnvironment): Promise<void> {
195-
if (terminal.shellIntegration) {
196-
await this.deactivateUsingShellIntegration(terminal.shellIntegration, terminal, environment);
197-
} else {
198-
this.deactivateLegacy(terminal, environment);
199-
}
200-
}
201-
202-
private activateLegacy(terminal: Terminal, environment: PythonEnvironment) {
203-
const activationCommands = getActivationCommand(terminal, environment);
204-
if (activationCommands) {
205-
terminal.sendText(activationCommands);
206-
this.activatedTerminals.set(terminal, environment);
207-
}
208-
}
209-
210-
private deactivateLegacy(terminal: Terminal, environment: PythonEnvironment) {
211-
const deactivationCommands = getDeactivationCommand(terminal, environment);
212-
if (deactivationCommands) {
213-
terminal.sendText(deactivationCommands);
214-
this.activatedTerminals.delete(terminal);
215-
}
216-
}
217-
218-
private async activateUsingShellIntegration(
219-
shellIntegration: TerminalShellIntegration,
206+
private async runCommand(
220207
terminal: Terminal,
221208
environment: PythonEnvironment,
222-
): Promise<void> {
223-
const activationCommand = getActivationCommand(terminal, environment);
224-
if (activationCommand) {
225-
try {
226-
await this.executeTerminalShellCommandInternal(shellIntegration, activationCommand);
227-
this.activatedTerminals.set(terminal, environment);
228-
} catch {
229-
traceError('Failed to activate environment using shell integration');
209+
operation: 'activate' | 'deactivate',
210+
trigger: ActivationTrigger,
211+
): Promise<CommandOutcome> {
212+
const watch = new StopWatch();
213+
const method = terminal.shellIntegration ? 'shellIntegration' : 'sendText';
214+
let outcome: CommandOutcome = 'failed';
215+
try {
216+
const command =
217+
operation === 'activate'
218+
? getActivationCommand(terminal, environment)
219+
: getDeactivationCommand(terminal, environment);
220+
if (!command) {
221+
outcome = 'noCommand';
222+
} else if (terminal.shellIntegration) {
223+
outcome = await this.executeTerminalShellCommandInternal(terminal.shellIntegration, command);
224+
} else {
225+
terminal.sendText(command);
226+
outcome = 'unverified';
230227
}
231-
} else {
232-
traceVerbose('No activation commands found for terminal.');
228+
} catch (error) {
229+
traceError(`Failed to ${operation} terminal environment`, error);
233230
}
234-
}
235-
236-
private async deactivateUsingShellIntegration(
237-
shellIntegration: TerminalShellIntegration,
238-
terminal: Terminal,
239-
environment: PythonEnvironment,
240-
): Promise<void> {
241-
const deactivationCommand = getDeactivationCommand(terminal, environment);
242-
if (deactivationCommand) {
243-
try {
244-
await this.executeTerminalShellCommandInternal(shellIntegration, deactivationCommand);
245-
this.activatedTerminals.delete(terminal);
246-
} catch {
247-
traceError('Failed to deactivate environment using shell integration');
248-
}
249-
} else {
250-
traceVerbose('No deactivation commands found for terminal.');
231+
if (outcome !== 'succeeded' && outcome !== 'unverified') {
232+
traceError(`Terminal ${operation} outcome: ${outcome}`);
251233
}
234+
sendTelemetryEvent(EventNames.TERMINAL_ACTIVATION_OUTCOME, watch.elapsedTime, {
235+
operation,
236+
outcome,
237+
method,
238+
shell: identifyTerminalShell(terminal),
239+
trigger,
240+
});
241+
return outcome;
252242
}
253243

254244
private async executeTerminalShellCommandInternal(
255245
shellIntegration: TerminalShellIntegration,
256246
command: string,
257-
): Promise<boolean> {
247+
): Promise<CommandOutcome> {
258248
const execution = shellIntegration.executeCommand(command);
259249
const disposables: Disposable[] = [];
260250
const timeoutMs = getShellIntegrationTimeout();
261251

262-
const promise = new Promise<void>((resolve) => {
252+
const promise = new Promise<CommandOutcome>((resolve) => {
263253
const timer = setTimeout(() => {
264-
traceError(`Shell execution timed out: ${command}`);
265-
resolve();
254+
resolve('timedOut');
266255
}, timeoutMs);
267256

268257
disposables.push(
269258
new Disposable(() => clearTimeout(timer)),
270259
this.onTerminalShellExecutionEnd((e: TerminalShellExecutionEndEvent) => {
271260
if (e.execution === execution) {
272-
resolve();
261+
resolve(e.exitCode === 0 ? 'succeeded' : e.exitCode === undefined ? 'unknown' : 'failed');
273262
}
274263
}),
275264
this.onTerminalShellExecutionStart((e: TerminalShellExecutionStartEvent) => {
@@ -281,11 +270,7 @@ export class TerminalActivationImpl implements TerminalActivationInternal {
281270
});
282271

283272
try {
284-
await promise;
285-
return true;
286-
} catch {
287-
traceError(`Failed to execute shell command: ${command}`);
288-
return false;
273+
return await promise;
289274
} finally {
290275
disposables.forEach((d) => d.dispose());
291276
}

‎src/features/terminal/terminalManager.ts‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -251,7 +251,7 @@ export class TerminalManagerImpl implements TerminalManager {
251251
},
252252
async () => {
253253
await waitForShellIntegration(terminal);
254-
await this.activate(terminal, environment);
254+
await this.ta.activate(terminal, environment, 'terminalOpen');
255255
},
256256
);
257257
} else {
@@ -402,7 +402,7 @@ export class TerminalManagerImpl implements TerminalManager {
402402
const env = this.ta.getEnvironment(t) ?? (await getEnvironmentForTerminal(api, t));
403403

404404
if (env && isActivatableEnvironment(env)) {
405-
await this.activate(t, env);
405+
await this.ta.activate(t, env, 'preExisting');
406406
}
407407
}
408408

@@ -456,11 +456,11 @@ export class TerminalManagerImpl implements TerminalManager {
456456
}
457457

458458
public activate(terminal: Terminal, environment: PythonEnvironment): Promise<void> {
459-
return this.ta.activate(terminal, environment);
459+
return this.ta.activate(terminal, environment, 'explicit');
460460
}
461461

462462
public deactivate(terminal: Terminal): Promise<void> {
463-
return this.ta.deactivate(terminal);
463+
return this.ta.deactivate(terminal, 'explicit');
464464
}
465465

466466
isActivated(terminal: Terminal, environment?: PythonEnvironment): boolean {

0 commit comments

Comments
 (0)